andygrove commented on code in PR #6456:
URL: https://github.com/apache/datafusion-comet/pull/6456#discussion_r4149556748
##########
native/core/src/execution/operators/iceberg_partition_value.rs:
##########
@@ -35,20 +35,31 @@ use iceberg::{Error, ErrorKind, Result};
/// Computes a batch's partition values: one row of the partition struct per
input row.
///
-/// Matches iceberg-rust's `PartitionValueCalculator` except for `year` and
`month` over a `date`,
-/// `timestamp`, or `timestamptz` source. iceberg-rust splits the calendar
with Arrow's `date_part`,
-/// which returns NULL for anything `chrono` cannot represent -- past year
262142 -- whereas
-/// iceberg-java's `DateTimeUtil` goes through `LocalDate` and covers every
Spark date (to year
-/// 5881580) and timestamp (to year 294247). The NULL did not fail the write:
the data file was
-/// committed claiming a NULL partition for rows whose source value is not NULL
-/// (apache/datafusion-comet#6145). Those two transforms go through Comet's
`iceberg_years` /
-/// `iceberg_months` kernels instead, the ones the sort in front of a
clustered write runs, which
-/// are pinned against iceberg-java over the whole domain. Wherever `chrono`
can represent the date
-/// the two implementations agree, so every value iceberg-rust could compute
is unchanged.
+/// Matches iceberg-rust's `PartitionValueCalculator` except for the time
transforms of a `date`,
+/// `timestamp`, or `timestamptz` source, which go through Comet's
`iceberg_years` /
+/// `iceberg_months` / `iceberg_days` / `iceberg_hours` kernels instead: the
ones the sort in front
+/// of a clustered write runs, pinned against iceberg-java's `DateTimeUtil`
over the whole domain.
+/// iceberg-rust's transforms differ from iceberg-java's in three ways:
///
-/// `day` and `hour` stay on iceberg-rust: they are floor divisions of the
epoch value and never
-/// consult the calendar. So do the nanosecond timestamp types, whose `i64`
range (years 1677 to
-/// 2262) lies inside `chrono`'s and which Comet's kernels do not accept.
+/// - `year` and `month` split the calendar with Arrow's `date_part`, which
returns NULL for
+/// anything `chrono` cannot represent -- past year 262142 -- whereas
iceberg-java goes through
+/// `LocalDate` and covers every Spark date (to year 5881580) and timestamp
(to year 294247). The
+/// NULL did not fail the write: the data file was committed claiming a NULL
partition for rows
+/// whose source value is not NULL (apache/datafusion-comet#6145).
+/// - All four floor a pre-epoch timestamp that lies exactly 999999
microseconds into a unit, which
+/// iceberg-java puts in the unit before, so `1969-01-01T00:00:00.999999`
belongs in the 1968
+/// partitions (apache/datafusion-comet#6426).
+/// - `day` moves a timestamp from the last second of a day before 1969-12-31
into the next day,
Review Comment:
No, there isn't one. apache/iceberg-rust#3022 even says `Day` already aligns
with Java, but `day_timestamp_micro` still truncates the seconds toward zero.
I've written one up with a reproducer and a one-line fix, and I'll link it here
once it's filed.
--
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]
