TimestampFromV7 should return the unix timestamp in milliseconds
jaredLunde opened this issue · 9 comments
Apologies if I am missing something, but it appears the current implementation of TimestampFromV7() uses the Gregorian epoch timestamp as opposed to unix:
Line 156 in 22c52c2
Lines 37 to 39 in 22c52c2
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)},
}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.
Ahh, should we expect each parser to return its draft-compliant timestamp though? Happy to draft that PR if you think so!
That would imply creating a separate TimestampVN type for each version, which doesn't feel like a good path. 🤔
Or separate by epoch
UnixTime,GregTimeUnixTimestamp,GregTimestamp
etc.
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.
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.
Might I suggest having two "from v7" functions here?
- The existing
TimestampFromV7implementation that returns auuid.Timestampobject. - A new
NativeTimeFromV7(orGoTimeFromV7;TimeFromV7, etc.) that returns a Go nativetime.Timeobject.
Thoughts?
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!
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.