andygrove opened a new pull request, #6851:
URL: https://github.com/apache/datafusion-comet/pull/6851
## Which issue does this PR close?
No issue. This lifts one of the restrictions listed in #5643: the fallback
that #6531 added for #6138.
## Rationale for this change
Since #6531, a native Iceberg write falls back to iceberg-java when the
output partition spec has a `float` or `double` field. iceberg-rust compared
float partition values with `OrderedFloat`'s equality, which treats `-0.0` and
`0.0` as one value. Both of the native writer's grouping paths then filed rows
with either value under whichever arrived first, and a read that pruned on the
other value lost rows (#6138).
apache/iceberg-rust#3327 gives `PrimitiveLiteral` hand-written `PartialEq`
and `PartialOrd` that follow Java's `Float.compare` and `Double.compare`:
`-0.0` sorts before `0.0`, and all NaNs are equal. That is how iceberg-java
compares partition keys, so with the pin past it the fallback can go.
## What changes are included in this PR?
- The `iceberg` and `iceberg-storage-opendal` pin moves from `af1da4c` to
`1f3bc34`, the merge commit of apache/iceberg-rust#3327. That is 17 commits.
`Cargo.lock` picks up apache-avro 0.22 and nine new crate versions (ouroboros,
strum 0.28, aliasable and their helpers).
- `CometIcebergNativeWrite` drops the float and double partition rule, and
`IcebergReflection` drops its helper.
- Two other upstream changes in the range need code changes:
- apache/iceberg-rust#3145 makes `FileScanTaskDeleteFile`'s fields private
behind a builder that validates on `build()`, and holds the deletion-vector
offset and size as `u64`. The planner builds delete files through the builder
and rejects a negative offset or size, the way it already rejects a negative
record count. `IcebergScanExec::fill_delete_file_sizes` sizes a delete file by
rebuilding it, as it already does for the task. The JVM serde already sends
every field the builder requires for a deletion vector.
- apache/iceberg-rust#3323 fixes iceberg-rust's `day` for pre-epoch
timestamps (apache/iceberg-rust#3315). Comet computes the time-transform
partition values of timestamps itself, so nothing it writes changes. The canary
test that pins iceberg-rust's values now expects the fixed `day`. iceberg-rust
still differs from iceberg-java for a timestamp exactly 999999 microseconds
into a unit (#6426), so Comet keeps computing these values.
- Docs: the user guide no longer lists the fallback. The contributor guide
and the Iceberg write review skill keep #6138 as an example of iceberg-rust's
value semantics, and now note the upstream fix.
The rest of the range doesn't need Comet changes:
- apache-avro 0.22 for manifest reading (apache/iceberg-rust#3354).
- Typed NULLs for nested columns added by schema evolution
(apache/iceberg-rust#2635).
- Page-index pruning for `FIXED_LEN_BYTE_ARRAY` decimals
(apache/iceberg-rust#3328).
- An Azure path fix (apache/iceberg-rust#3311).
- REST catalog sessions, which Comet doesn't use.
- Dependency and CI bumps.
The nested schema evolution guard from #6504 stays, because it waits on
apache/iceberg-rust#3255, which is still open.
## How are these changes tested?
- `CometIcebergWriteActionSuite` "signed-zero float and double identity
partitions match iceberg-java" now requires the native writer to run.
- Every row goes through one task, so the zeros meet in one writer. The
fanout writer gets them interleaved, and the clustered writer gets one row per
partition.
- It compares against iceberg-java three things: the partition
directories, the committed partition values, and the rows a pruned read returns
for each zero.
- Against a `libcomet` built from `main` with the fallback removed, the
test fails: both writers put every row under `f=-0.0/d=0.0`. With this pin it
passes.
- "float and double partition paths match iceberg-java" now checks the
native writer's directories.
- `CometIcebergWriteDetectionSuite` replaces four fallback tests with one
that checks that float, double and nested double identity partitions are
`Compatible`.
- New Rust tests cover the planner's delete-file validation and the scan's
delete-file rebuild.
- Locally, on the default Spark 4.1 profile (Iceberg 1.11), all of these
pass:
- all 718 `datafusion-comet` lib tests;
- clippy, with the same flags as CI;
- the 14 Iceberg JVM suites that don't need MinIO: 419 passed, 0 failed,
and 1 canceled (the unknown-transform test, which is canceled on this profile
on `main` too).
A pin bump can change file bytes and manifests, and the other Spark profiles
pin Iceberg 1.5.2, 1.8.1 and 1.10.0. So this PR is labelled `run-iceberg-tests`
and `run-all-spark-profiles`.
--
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]
