You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/28 22:58:05 UTC
[PR] fix(types): floor negative nanosecond timestamps [iceberg-go]
fallintoplace opened a new pull request, #1586:
URL: https://github.com/apache/iceberg-go/pull/1586
## What changed
Use signed floor division when converting nanosecond timestamps to microseconds. Add boundary coverage around zero and verify casts to both timestamp variants.
## Why
Go integer division truncates toward zero. For pre-epoch timestamps with sub-microsecond precision, that could move the converted value forward by one microsecond and even cross the Unix epoch.
## Testing
- `go test .`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(types): floor negative nanosecond timestamps [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1586:
URL: https://github.com/apache/iceberg-go/pull/1586
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(types): floor negative nanosecond timestamps [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org