andygrove opened a new pull request, #3323: URL: https://github.com/apache/iceberg-rust/pull/3323
## Which issue does this PR close? - Closes https://github.com/apache/iceberg-rust/issues/3315. ## What changes are included in this PR? `Day::day_timestamp_micro` and `Day::day_timestamp_nano` took the whole seconds of a timestamp with `/`, which truncates toward zero, but took the fraction of the second with `rem_euclid`, which floors. For a negative timestamp that is not on a whole second, the seconds came out one too high, so a timestamp in the last second of a day before 1969-12-31 got the next day. `Day::transform`, `Day::transform_literal` and predicate projection all go through these two functions. This PR computes the day with `div_euclid` on the length of a day, the way `Hour` already does, and drops the `Duration` arithmetic and its error path. The result now matches Iceberg Java for every input in the issue's table. Files that iceberg-rust wrote before this change can hold such timestamps in the next day's partition. Inclusive projection already widens pre-epoch `day` predicates by one day to cover such files, as Java's `ProjectionUtil.fixInclusiveTimeProjection` does, but only for `timestamp` columns. This PR applies it to `timestamptz`, `timestamp_ns` and `timestamptz_ns` as well, as Java's `Timestamps.project` does. Without that, a `<`, `<=`, `=` or `IN` scan on those types could skip a partition that iceberg-rust wrote before the fix. The issue suggested flooring only the seconds. That would also copy what Java's `DateTimeUtil` does for a pre-epoch timestamp exactly 999999 microseconds (999999999 nanoseconds) after midnight: Java puts it in the day before, so `1969-01-01T00:00:00.999999` is day 1968-12-31. I tried that and decided against it: - It makes the transform non-monotonic, because `1969-01-01T00:00:00.999999` would get an earlier day than `1969-01-01T00:00:00.999998`. Inclusive projection assumes the transform preserves order, so `ts >= '1969-01-01'` would project to `day >= 1969-01-01` and skip the partition holding that row. Java's own projection has the same gap. - `day` would disagree with `hour`, `month` and `year`, which floor. - iceberg-rust already returns the calendar day for these timestamps, as PyIceberg does, so this PR leaves them unchanged. Not changed here: - Iceberg Java writes those 999999 timestamps to the day before, and iceberg-rust's projection can miss them there, as it could before this PR. - Equality deletes are matched to data files by partition, so a delete written after this fix will not reach a row that iceberg-rust wrote one day too high before it. Deletes written by Java already miss those rows. Rewriting the affected files puts the rows in the right partition. - Strict projection has no port of Java's `fixStrictTimeProjection`. `StrictProjection` is only used in tests so far. ## Are these changes tested? Yes. The new `test_transform_days_pre_epoch` runs `Day::transform` on microsecond and nanosecond arrays, and `transform_literal` on `timestamp`, `timestamptz`, `timestamp_ns` and `timestamptz_ns` literals, for seven timestamps around pre-epoch midnights. For six of them the expected day is what `Transforms.day()` returns in Iceberg Java 1.11.0 for all four types, and in 1.5.2 for `timestamp` and `timestamptz`. The seventh is the 999999 case above, where the test expects the calendar day and notes Java's value. Without the fix, three of the seven get the next day. The new `test_projection_timestamp_types_day_negative` checks that `<`, `<=`, `=` and `IN` projections on all four timestamp types still include the next day, and it fails for `timestamptz` without the projection change. The existing unit tests in the `iceberg` crate pass unchanged. ## AI Disclosure The code, the tests and this description were drafted with Claude Code (an AI assistant) and then reviewed by me. The expected days in `test_transform_days_pre_epoch` were checked by running Iceberg Java, not taken from the Rust code. The part I'd most like a second look at is the choice not to follow Java for the 999999 timestamps. -- 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]
