andygrove opened a new pull request, #7249: URL: https://github.com/apache/datafusion-comet/pull/7249
## Which issue does this PR close? Part of #6410, which tracks the regressions this PR finds. The all-NaN Iceberg bound regression is tracked in #7242. This is a long-lived draft that follows DataFusion main until DataFusion 56.0.0 is released, and it is not meant to merge before then. Related: #5477, whose Arrow 60 items it unblocks. This replaces #6404, moving the branch from a fork to `datafusion-56` in this repository. ## Rationale for this change DataFusion main is at `350ca76` (2026-10-26). Since 55.1.0 it has moved to arrow/parquet 60.0.0 and object_store 0.14.2 (apache/datafusion#25335), and it has a number of API changes (see its [56.0.0 upgrade guide](https://github.com/apache/datafusion/blob/350ca76505c9513d16e7dd9910bcd9bde2c911ed/docs/source/library-user-guide/upgrading/56.0.0.md)). Building Comet against main now surfaces the breaks early and lets CI run the Comet suites against it, rather than finding everything at release time. The branch is one commit on top of current `apache/main`. ## What changes are included in this PR? **Dependencies** - `datafusion`, `datafusion-datasource`, `datafusion-functions`, `datafusion-physical-expr-adapter` and `datafusion-spark` in the workspace now use `git = "https://github.com/apache/datafusion";, rev = "350ca76..."`. So do the `datafusion-functions-nested` dev-dependency in core and `datafusion` in both contrib crates. - `arrow`, `arrow-data`, `arrow-select` and `parquet` go from 59.2.0 to 60.0.0. - `object_store` goes from 0.13.2 to 0.14.2. `object_store_opendal` goes from 0.58.0 to 0.59.0, the first release on object_store 0.14. That release keeps opendal on 0.58, which is what iceberg-rust uses. - `iceberg` and `iceberg-storage-opendal` move to `8c8cf19`, a branch on the [andygrove/iceberg-rust](https://github.com/andygrove/iceberg-rust/tree/comet-df56-arrow60) fork. - iceberg-rust main is still on arrow 59, and Comet hands arrow types to iceberg-rust directly, so the two must agree. apache/iceberg-rust#3257 ("Bump to Arrow 60") is the fix, and it is still open. - Its head (`0d5eb12`) is now 42 commits behind the main revision Comet builds against (`1f3bc34`), and Comet's Iceberg code uses APIs from those commits (`FileMetadata` from `close`, the public `FileScanTaskDeleteFile` fields, `OPENDAL_IO_TIMEOUT_MS`). So the fork branch is #3257's head with `1f3bc34` merged in. The merge had no conflicts. - Move back to an upstream main revision once #3257 merges. The Cargo.toml comment says so. - Lockfiles: `native/Cargo.lock` and `contrib/delta/native/Cargo.lock` are regenerated. **Source changes (all forced by the upgrade)** - Arrow 60 made the `FFI_ArrowArrayStream` callbacks private, so the decimal-realignment test in `scan.rs` can no longer build a hand-rolled C stream. It now exports a struct array and imports it with `from_ffi_and_data_type`, the call `ArrowArrayStreamReader::next` makes. `AlignedArrowStreamReader` was already gone on main. - Parquet 60 moved the page index behind `ParquetMetaData::page_index()`, a `PageIndexProvider` with per-row-group access. - `with_spark_arrow_schema` and the rebase footer rewrite carry it over with `set_page_index`. - `page_selection_bound_selectors` in `delta_dv.rs` walks the row groups through `page_index_for_row_group`. - The tests check `page_index()` instead of `column_index()`/`offset_index()`. - DataFusion's `coerce_int96` and `coerce_int96_tz` are now typed (`DFTimeUnit`, `ConfigTimeZone`) instead of strings. - object_store 0.14 added `GetResult::extensions`, which `ScanIoObjectStore::get_opts` now forwards. - `OffsetBuffer::first()`/`last()` now return values instead of `Option`s. - Field and schema metadata are now arrow's `Metadata` type. The datetime rebase resolution reads it directly, and the shuffle schema cache's size estimate uses `len()` instead of `capacity()`. - `fb_to_schema` is deprecated. The shuffle decoder uses `try_fb_to_schema`, so an invalid schema message now returns a decode error where it used to panic. - `FFI_ArrowSchema::with_metadata` is now `unsafe`. The call in `move_to_spark` is sound because the schema was just built by `TryFrom`. - Four DataFusion APIs are deprecated, and each call stays under `#[allow(deprecated)]`: - `HashJoinExec::with_dynamic_filter_expr` and `SortExec::with_dynamic_filter_expr`, both with no replacement. Upstream installs a dynamic filter only from `handle_child_pushdown_result` (joins) or `try_from_proto` (sorts). `DynamicFilterJoinExec` and the runtime top-K wiring set it directly. The deprecation note asks for an issue if you have a use case. Comet has one, so it is worth filing before the setters are removed. - `ExecutionPlan::with_preserve_order`, which DataFusion no longer calls. `MetricOwningExec` keeps forwarding it. - `ParquetAccessPlan::into_overall_row_selection`, in three `delta_dv.rs` tests that model the reader's file-level selection peak. DataFusion now keeps row-group-local entries, so the deletion-vector admission bound should be rechecked against the new reader. - `normalize_spark_empty_key_metadata_rejects_other_malformed_encodings`: Arrow 60 accepts empty entries in unsorted Variant dictionaries (apache/arrow-rs#10352). - Two of the five "malformed" dictionaries in the test were rejected only because of their empty entry: the one with a repeated entry and the one with trailing bytes. - Both now pass through unchanged. That matches Spark, which checks only the offsets of the keys it looks up, so the test now asserts it. - `make_map_c` in `comet-test-udfs`: arrow 60's `MapBuilder` names the map children `key`/`value` by default, so the test UDF now names them `keys`/`values` explicitly to keep exercising non-canonical names. - `datafusion-spark`'s `SparkXxhash64` now ignores the hidden children of a NULL struct (apache/datafusion#25774) and keeps the running hash for nested dictionaries (apache/datafusion#25813). Three tests in `xxhash64_diff.rs` that pinned those differences now assert equality. Comet still keeps its own kernel for those types, so widening the routing is a possible follow-up. **Known issue: all-NaN float and double columns in Iceberg manifests** Parquet 60 writes NaN min and max statistics for a column chunk that holds only NaN, and iceberg-rust copies them into the manifest bounds, where iceberg-java records none. `CometIcebergWriteActionSuite` "native acceleration: NaN float/double manifest metrics match the JVM writer" caught it, so this PR ignores that test, links #7242 and lists the issue under the Iceberg known issues and in a new 1.3.0 section of the upgrade guide. Re-enable the test once iceberg-rust skips NaN bounds. **DataFusion behavior changes checked against Comet** - The `floor`/`ceil` return types and the result of `map_extract` for absent keys changed. Comet registers its own `floor`, `ceil` and `map_extract`, so neither change reaches Comet. - Missing Parquet null counts are now treated as unknown instead of zero. This is a correctness fix. It can disable some pruning on files written by parquet-rs before 53.1. **Not included (possible follow-ups)** - #5477: Arrow 60 contains apache/arrow-rs#10810 and apache/arrow-rs#10352, so the encoded-metadata decoding and the empty-key retry (`canonicalize_spark_empty_key_metadata`) can now be removed. - Move the iceberg pin back to a main revision once apache/iceberg-rust#3257 merges. - Wire `stack` once apache/datafusion#26266 is in the pinned revision. It is now (`6489d15`). ## How are these changes tested? Run locally on macOS aarch64, against the lockfile in this PR: - `cargo fmt --check`, `cargo clippy --all-targets --workspace -- -D warnings`, the `python-udf` and `jemalloc` clippy runs, `cargo check --benches`, `cargo check -p datafusion-comet --all-targets --features contrib-delta,contrib-lance` and `cargo machete` are all clean. - `cargo nextest run` in `native/`: 2666 tests run, 2661 passed, 5 failed. All five are `parquet::objectstore::http_metrics` tests that bind a loopback socket, which this machine refuses (`Operation not permitted`), so CI is the check for them. - `cargo nextest run -p datafusion-comet --lib --features jemalloc alloc_accounting`: 5 passed. Not run: - The `python-udf` tests, because pyarrow could not be installed here. - The JVM suites. The previous revision of this PR passed 1388 tests on the default Spark 4.1 profile, but the branch has been reset onto current main since, so they need a fresh run. - Spark's own SQL tests, the Iceberg Spark tests and the other Spark profiles. Those need the `run-all-spark-profiles`, `run-spark-4.1-tests` and `run-iceberg-tests` labels. -- 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]
