andygrove opened a new pull request, #6239:
URL: https://github.com/apache/datafusion-comet/pull/6239
## Which issue does this PR close?
Closes #6145.
## Rationale for this change
iceberg-rust goes through `chrono`, whose calendar ends at year 262142. A
Spark `DATE` reaches year 5881580 and a `TIMESTAMP` year 294247, and
iceberg-java handles the whole range. Past year 262142 the native writer went
wrong in two ways, both reproduced on main:
- **Panic.** Rendering the directory of a `days(...)` partition, or of an
identity partition on a `DATE` column, dies in iceberg-rust with `` `DateTime +
TimeDelta` overflowed ``. The task fails where iceberg-java succeeds.
- **NULL partition values, committed without an error.** iceberg-rust
computes `year` and `month` with Arrow's `date_part`, which returns NULL past
chrono's range. A table partitioned by `years(d), months(ts)` and written with
`date_from_unix_date(100000000)` and `timestamp_micros(9000000000000000000)`
got partition `{null, null}` in `d_year=null/ts_month=null`. iceberg-java
writes `{273790, 3422383}` in `d_year=275760/ts_month=287168-08`.
Two corrections to the issue:
- `hours(...)` never panicked. An `i32` hour count only spans about 245,000
years either side of 1970, which is inside chrono's calendar, and iceberg-rust
already renders it the way iceberg-java does.
- Identity dates also diverged well inside chrono's range. Outside years
0-9999 iceberg-rust spelled them `+10000-01-01` and `-0001-12-31`.
iceberg-java's `TransformUtil.humanDay` writes `10000-01-01` and `-001-12-31`.
## What changes are included in this PR?
- `iceberg_partition_path.rs` renders `date` partition values, for both the
`day` transform and identity on a date, with a total equivalent of
`TransformUtil.humanDay`. It reuses the module's existing `civil_from_days`.
- The new `iceberg_partition_value.rs` adds a Comet
`PartitionValueCalculator` to replace iceberg-rust's.
- `year` and `month` over `date`, `timestamp` and `timestamptz` go through
the `iceberg_years` / `iceberg_months` kernels from
`datafusion-comet-spark-expr`. The sort in front of a clustered write already
uses them, and their own tests pin them against iceberg-java over the whole
domain.
- Every other transform stays on iceberg-rust, and so do the V3 nanosecond
timestamp types.
- Wherever chrono can represent the date the two agree, so no value the
native writer computed before changes.
- In `iceberg_write.rs`, `ClusteredBatchSplitter` becomes
`PartitionSplitter`, and both partitioned writers use it.
- The fanout path no longer goes through iceberg-rust's
`RecordBatchPartitionSplitter`, which computes values with iceberg-rust's
transforms.
- The new `split_groups` emits partitions in first-appearance order
instead of `HashMap` order. Like the upstream splitter, it hands a
single-partition batch on whole.
- It groups on the same `Struct` equality as before, so #6138 (`-0.0` and
`0.0` merging) is unchanged.
- `SparkIcebergTemporalTransform::transform` is a new array-level entry
point to the kernel.
- The contributor guide's Iceberg writes page lists the new adaptation, and
adds chrono's range to the list of pitfalls.
## How are these changes tested?
The expected values come from iceberg-java 1.11 itself (`Transform#bind`,
`toHumanString` and `PartitionSpec#partitionToPath` on a JDK 17 JVM), not from
Comet's own arithmetic.
- Rust:
- Rendering covers `date` values past chrono's range on both sides, and
the delegated `year` / `month` / `hour` ordinals at their extremes.
- Calculator tests take values one step past chrono's range on both sides,
the values from the issue, and both ends of Spark's date and timestamp domains.
They also check that iceberg-rust's own calculator returns NULL for those
values, that the two calculators agree for every transform wherever chrono can
represent the date, and that the nanosecond types stay on iceberg-rust.
- `split_groups` tests cover row grouping and order, the whole-batch fast
path, and empty batches.
- An end-to-end write goes through both the fanout and the clustered
writer. It checks directories, `DataFile` partition values and the transport
manifest.
- Removing each fix makes its tests fail. That covers routing
`year`/`month` back to iceberg-rust, dropping the `date` rendering arm (which
reproduces the panic), misassigning rows in `split_groups`, and disabling its
single-partition fast path.
- JVM:
- The new `CometIcebergWriteActionSuite` test "native acceleration:
partitions past year 262142 match iceberg-java" writes the same rows through
the native writer and iceberg-java, clustered and fanout. It compares
directories, committed partition values, and rows read back with Comet on and
off.
- It fails on main on Spark 4.1 with the panic. With the fix it passes on
Spark 3.4 (Iceberg 1.5.2), 3.5 (1.8.1), 4.0 (1.10.0) and 4.1 (1.11.0).
- The full `CometIcebergWriteActionSuite` (75 tests) and
`CometIcebergRewriteActionSuite` pass on Spark 4.1. So do all
`datafusion-comet` lib tests, and `cargo clippy --all-targets --workspace -- -D
warnings` and `cargo fmt` are clean.
Iceberg's own Spark SQL tests have not run. Since this changes the native
writer, it should get `run-iceberg-tests` before it leaves draft.
--
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]
