andygrove opened a new pull request, #6456: URL: https://github.com/apache/datafusion-comet/pull/6456
## Which issue does this PR close? Closes #6426. The 1.1.0 regression audit (#6399) found this, and #6402 tracks it. ## Rationale for this change The native kernels for Iceberg's `years`, `months`, `days` and `hours` functions (#5638, and the `ApplyFunctionExpression` route from #5773) floor. Iceberg's `DateTimeUtil.convertMicros` doesn't quite. For a negative timestamp it builds an instant from `floorDiv(micros, 1_000_000)` seconds and `floorMod(micros + 1, 1_000_000)` microseconds, then subtracts one from the number of whole units between the epoch and that instant. When the microsecond of second is 999999, the added microsecond wraps to 0 without carrying into the seconds. So a timestamp exactly 999999 microseconds into a unit gets the unit before. Iceberg puts `1969-01-01 00:00:00.999999` in year -2, month -13, day 1968-12-31 and hour -8761, and rc1 returns -1, -12, 1969-01-01 and -8760. Iceberg's Spark functions and its partition transforms both call `DateTimeUtil`, so this is what Spark returns and also what Iceberg partitions by. I ran `DateTimeUtil` from the Iceberg 1.5.2, 1.8.1, 1.10.0 and 1.11.0 runtimes on 1.63 million values. They included every month start from 1600 to 2100, every day and hour start around 1969 with offsets on either side, and random values over the whole `i64` range. All four versions agree, and 5,919 of the values differ from a floor. ## What changes are included in this PR? - `temporal.rs`: for a negative timestamp exactly 999999 microseconds into an hour or a day, the kernels now return the unit before, as Iceberg does. `years` and `months` are computed from the day, and every month or year boundary is also a day boundary, so they follow. Dates were already right, because `convertDays` is a true floor. Non-negative timestamps are unchanged. - `iceberg_partition_value.rs`: since #6239 the native Iceberg writer computes `year` and `month` partition values with these kernels, so this fix covers them too. It computed `day` and `hour` with iceberg-rust's transforms, which floor. After the kernel change those would disagree with the sort in front of a clustered write. The writer now computes `day` and `hour` of a timestamp with the kernels as well, so all four time partitions match iceberg-java. This also fixes a separate bug in iceberg-rust's `day`. It takes the whole seconds with a truncating division and the microseconds with a flooring one, so it moves a timestamp from the last second of a day before 1969-12-31 into the next day, unless the microsecond of second is 0 or 999999. That is 10,295 of the values above. `day` of a date still goes through iceberg-rust. The native writer is off by default. - The doc comments in `iceberg_write.rs` and the contributor guide's Iceberg writes page now say which transforms go through the kernels. - `CometIcebergSystemFunctionExtensionsSuite` is a new suite, so I added it to both PR build workflows. The extra check costs about 0.2 ns per row for `days` and `hours` in a standalone micro-benchmark, 0.4 to 0.6 ns. On backporting: the writer part depends on #6239, which is not on branch-1.1, where the native writer computes all four time partitions with iceberg-rust. A backport would take the `temporal.rs` change and its tests. On that branch the native writer (off by default) would then keep iceberg-rust's partition values, which disagree with the sort for these rows. ## How are these changes tested? - Rust: `timestamps_ending_in_999999_match_iceberg_date_time_util` checks values against the JVM for year, month, day and hour boundaries before and after the epoch, and for a second that starts no unit. For each boundary it checks the microsecond before it, the boundary itself, and 999998, 999999, 1000000 and 1999999 microseconds after it. The extremes test adds the lowest `i64` that ends in 999999. `pre_epoch_timestamps_partition_like_iceberg_java` checks the writer's partition values for both timestamp types, and it pins iceberg-rust's differing values. Both tests fail without the change. I also compared the kernels with the 1.63 million JVM values (not committed), and all of them match. - Scala, from the audit's reproducers: a new test in `CometIcebergSystemFunctionSuite` runs the four functions on `TIMESTAMP` and `TIMESTAMP_NTZ` values just after each boundary, in a projection and in filters. A second new test writes those values with the native writer into a table partitioned by all four transforms, then checks `_partition` against Iceberg's functions. The new `CometIcebergSystemFunctionExtensionsSuite` installs Iceberg's SQL extensions, checks that the filters are rewritten to `ApplyFunctionExpression`, and compares the results with Spark. All three fail without the change. - I ran `cargo test` for the Iceberg tests in `datafusion-comet-spark-expr` and `datafusion-comet`, plus clippy. On the default profile I ran `CometIcebergSystemFunctionSuite`, `CometIcebergSystemFunctionExtensionsSuite`, `CometIcebergWriteActionSuite`, `CometIcebergRewriteActionSuite` and `CometIcebergResidualPushdownSuite`. I also ran the two system-function suites on spark-3.4 (Iceberg 1.5.2) and spark-3.5 (Iceberg 1.8.1). -- 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]
