laskoviymishka commented on code in PR #1586:
URL: https://github.com/apache/iceberg-go/pull/1586#discussion_r3680745308
##########
literals_test.go:
##########
@@ -201,8 +201,8 @@ func TestTimestampNanoLiteralConversions(t *testing.T) {
{
name: "negative timestamp",
nanoLit:
iceberg.TimestampNsLiteral(-1234567890123456789),
- expectedMicro: iceberg.Timestamp(-1234567890123456),
- expectedRoundTrip:
iceberg.TimestampNano(-1234567890123456000),
+ expectedMicro: iceberg.Timestamp(-1234567890123457),
+ expectedRoundTrip:
iceberg.TimestampNano(-1234567890123457000),
Review Comment:
The new value is right — `floor(-1234567890123456.789 µs) =
-1234567890123457`, then `*1000`.
I'd add a one-line comment here though. This case was silently wrong for
years before the PR, and the asymmetry with the positive case (where we just
trim the last 3 digits) is exactly the shape a future reader "corrects" back to
the intuitive-but-wrong value. Something like `// floor(-…456.789 µs) = -…457
µs, then *1000` would save that trip. wdyt?
##########
types.go:
##########
@@ -799,7 +799,7 @@ func (t TimestampNano) ToTime() time.Time {
}
func (t TimestampNano) ToMicros() Timestamp {
- return Timestamp(int64(t) / 1000)
+ return Timestamp(internal.FloorDiv(int64(t), 1000))
Review Comment:
This is the right change and it's the last raw-truncation site, so no
objection.
Optional: since this exported method now has a specific rounding contract, a
one-line godoc noting it floors toward negative infinity (matching Java's
`Math.floorDiv`) would keep someone from "simplifying" it back to `/ 1000`
later.
##########
literals_test.go:
##########
@@ -235,6 +235,36 @@ func TestTimestampNanoLiteralConversions(t *testing.T) {
}
}
+func TestTimestampNanoToMicrosFloorsNegativeValues(t *testing.T) {
+ tests := []struct {
+ nanos iceberg.TimestampNano
+ micros iceberg.Timestamp
+ }{
+ {nanos: -1001, micros: -2},
+ {nanos: -1000, micros: -1},
+ {nanos: -999, micros: -1},
+ {nanos: -1, micros: -1},
+ {nanos: 0, micros: 0},
+ {nanos: 1, micros: 0},
+ {nanos: 999, micros: 0},
+ {nanos: 1000, micros: 1},
+ {nanos: 1001, micros: 1},
+ }
+
+ for _, tt := range tests {
Review Comment:
Small thing: this table drops the `name` field and `t.Run` that the other
table tests in this file use, so a failing case just points at the assert line
with no hint of which `{nanos, micros}` pair broke.
It also mixes `assert` (non-fatal) with the `require.NoError` a few lines
down — and since `require` kills the goroutine, an error out of the `To()` path
aborts the whole test before the later cases ever run. Wrapping the loop body
in `t.Run(tt.name, ...)` fixes both at once (`require` inside a subtest only
fails that subtest). Not blocking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
