TimestampFromV7 should return the unix timestamp in milliseconds

[jaredLunde]

Apologies if I am missing something, but it appears the current implementation of TimestampFromV7() uses the Gregorian epoch timestamp as opposed to unix:

https://github.com/gofrs/uuid/blob/22c52c268bc0dcc0569793f5b1433db423f5a9c6/uuid.go#L156 https://github.com/gofrs/uuid/blob/22c52c268bc0dcc0569793f5b1433db423f5a9c6/generator.go#L37-L39

The draft states:

unix_ts_ms: 48 bit big-endian unsigned number of Unix epoch timestamp as per Section 6.1.

So shouldn't this be:

func TimestampFromV7(u UUID) (Timestamp, error) {
    if u.Version() != 7 {
        return 0, fmt.Errorf("uuid: %s is version %d, not version 6", u, u.Version())
    }

    t := 0 |
        (int64(u[0]) << 40) |
        (int64(u[1]) << 32) |
        (int64(u[2]) << 24) |
        (int64(u[3]) << 16) |
        (int64(u[4]) << 8) |
        int64(u[5])

    return Timestamp(t), nil
}

Where tests would be as straightforward as:

    tests := []struct {
        u       UUID
        want    Timestamp
        wanterr bool
    }{
        {u: Must(FromString("00000000-0000-7000-0000-000000000000")), want: Timestamp(0x000000000000)},
        {u: Must(FromString("018a8fec-3ced-7164-995f-93c80cbdc575")), want: Timestamp(0x018a8fec3ced)},
        {u: Must(FromString("ffffffff-ffff-7fff-ffff-ffffffffffff")), want: Timestamp(0xffffffffffff)},
       }

[dylan-bourque]

I didn't write the existing implementation, but it looks like it's reusing the Timestamp type and adjusting the value to match that type's documented constraints here.

[jaredLunde]

Ahh, should we expect each parser to return its draft-compliant timestamp though? Happy to draft that PR if you think so!

[dylan-bourque]

That would imply creating a separate TimestampVN type for each version, which doesn't feel like a good path. 🤔

[jaredLunde]

Or separate by epoch

• UnixTime, GregTime
• UnixTimestamp, GregTimestamp etc.

[dylan-bourque]

That would be a breaking change to everything currently using Timestamp, though.

I'm certainly not the final arbiter but I'd argue for leaving this as is.

[jaredLunde]

I mean you could mark Timestamp as deprecated to avoid shipping a new major version. That said, I totally understand not wanting to change it. As a user, I was confused given this in the draft: https://www.ietf.org/archive/id/draft-peabody-dispatch-new-uuid-format-04.html#name-example-of-a-uuidv7-value

-------------------------------
field      bits    value
-------------------------------
unix_ts_ms   48    0x17F22E279B0
var           4    0x7
rand_a       12    0xCC3
var           2    b10
rand_b       62    0x18C4DC0C0C07398F
-------------------------------
total       128
-------------------------------
final: 017F22E2-79B0-7CC3-98C4-DC0C0C07398F

I expected that parsing a UUID v7 would yield a Unix timestamp w/ millisecond precision, as this was a deliberate design choice by the UUID authors.

[kohenkatz]

Might I suggest having two "from v7" functions here?

• The existing TimestampFromV7 implementation that returns a uuid.Timestamp object.
• A new NativeTimeFromV7 (or GoTimeFromV7; TimeFromV7, etc.) that returns a Go native time.Time object.

Thoughts?

[cameracker]

Hi, chiming in. I think it's a better developer experience to offer a neutral, consistent timestamp type and allow the developer to convert as needed. Understood it's not compliant to spec in a technical sense. I'll close the issue but please feel free to continue discussing. Thanks for the feedback!

[kohenkatz]

For some users, there may be a performance reason to have a native time.Time conversion. I did an experiment here - adding a TimeFromV7 method and a benchmark that compares TimeFromV7() and TimestampFromV7() + Time() - in 10 benchmark runs, converting to a native timestamp averages 33% faster than converting to this package's Timestamp and then to the native timestamp, at least on my machine:

> go test -benchmem -run=^$ -bench '^(BenchmarkGetTimeFromTimestamp|BenchmarkGetNativeTime)$' -count 10 github.com/gofrs/uuid/v5

goos: windows
goarch: amd64
pkg: github.com/gofrs/uuid/v5
cpu: 12th Gen Intel(R) Core(TM) i7-1280P
BenchmarkGetTimeFromTimestamp-20        301292166                4.005 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        304036849                3.908 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        261100980                4.021 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        308387047                4.134 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        298532562                4.177 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        277410657                3.922 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        248709250                5.145 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        245341626                5.171 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        240928731                5.481 ns/op           0 B/op          0 allocs/op
BenchmarkGetTimeFromTimestamp-20        283031826                4.415 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               352612831                3.094 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               395081556                3.225 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               379098888                3.022 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               377483012                3.257 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               400233602                3.217 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               392246847                3.037 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               389608377                3.169 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               345799002                3.361 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               391484430                3.576 ns/op           0 B/op          0 allocs/op
BenchmarkGetNativeTime-20               303667467                4.294 ns/op           0 B/op          0 allocs/op
PASS
ok      github.com/gofrs/uuid/v5        35.777s

Admittedly, for most of us the difference between 3.022ns (fastest native run) and 5.481ns (slowest two-step run) is not going to be noticeable, but I am sure there are some people for whom it makes a difference.