chore: upgrade to DataFusion main + arrow/parquet 59.1.0 [WIP] - #4888
Draft
andygrove wants to merge 4 commits into
Draft
chore: upgrade to DataFusion main + arrow/parquet 59.1.0 [WIP]#4888andygrove wants to merge 4 commits into
andygrove wants to merge 4 commits into
Conversation
2 tasks
Contributor
|
related to #4865 |
Work through the compile breakage surfaced by pinning DataFusion to main: - Drop opt_filter from GroupsAccumulator::merge_batch impls and their delegating call sites (avg, avg_decimal, correlation, covariance, percentile, stddev, sum_decimal, sum_int, variance, merge_as_partial) to match the upstream signature change. - Migrate MutableArrayData::extend/extend_nulls to try_extend/ try_extend_nulls across array_funcs and copy.rs (arrow 59 returns a Result instead of panicking). - Rewire shuffle spill to the new DiskManager API, where create_tmp_file returns Arc<dyn SpillFile> and path() returns Option<&Path>. - Replace deprecated TableSchema::from_file_schema/with_table_partition_cols with the TableSchema builder. - Use FixedSizeBinaryArray::try_from (arrow 59 dropped the From impl). The native iceberg scan remains broken: iceberg-rust still depends on arrow 58, so its RecordBatch does not unify with Comet's arrow 59 type. This is blocked on iceberg-rust upgrading to arrow 59.
Bump the DataFusion pin to main @ 179b32c9b and refresh the lockfile. arrow/parquet stay at 59.1.0 and object_store at 0.13.2, which is what DataFusion main still uses. Conflict resolutions: - native/Cargo.toml, native/core/Cargo.toml: keep the git pins, advanced to 179b32c9b. - native/Cargo.lock: regenerated from main's lockfile via `cargo update`. - native/spark-expr/src/array_funcs/array_compact.rs: take main's deletion (main switched to DataFusion's `array_compact` in apache#4741). - native/core/src/parquet/parquet_exec.rs: keep both sides' imports (`FieldRef` from this branch, `ParquetOptions` from main). Adaptations to newer DataFusion main: - `GroupsAccumulator::convert_to_state` is now a required method and `supports_convert_to_state` is gone. That gate is what previously kept skip-partial-aggregation away from Comet's accumulators, so returning `not_impl_err` would now fail at runtime once the probe fires. Implement it for all 13 accumulators instead, via a shared `convert_to_state_per_row` helper that accumulates each row into its own group on a fresh accumulator, so the state columns match `state()` by construction. `avg` builds its state directly because it holds a non-cloneable `avg_fn`. - hll_plus_plus (new on main) drops `opt_filter` from `merge_batch`. - `MergeAsPartialGroupsAccumulator::convert_to_state` returns the input verbatim: its input is already the inner accumulator's intermediate state. - `FileMetadataCache` is now a type alias, so `Arc<dyn FileMetadataCache>` becomes `Arc<FileMetadataCache>`. - `UnnestOptions::preserve_nulls` was replaced by `NullHandling`. Use the backward-compatible `with_preserve_nulls` setter to keep behavior identical. - `arrow::ipc::writer::CompressionContext` is deprecated in favor of `IpcWriteContext`. The native Iceberg scan is now behind a new off-by-default `iceberg-scan` feature. iceberg-rust (including its main branch) is still on arrow 58, so its `RecordBatch` does not unify with the arrow 59 that DataFusion main requires, and that single error was keeping the whole `datafusion-comet` crate from compiling -- which stopped CI from surfacing any other breakage. With the feature off the workspace builds clean; with it on, that one type mismatch is the only remaining error, so re-enabling is a one-flag change once iceberg-rust upgrades. Planning an IcebergScan operator in a build without the feature returns an explanatory error. `cargo check --workspace --all-targets` and `cargo clippy --workspace --all-targets -- -D warnings` are clean. Native tests pass except 3 pre-existing failures in datafusion-comet-jni-bridge's panic-handling tests, which fail identically on apache/main.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WIP draft to track the upgrade to DataFusion main (currently pinned to
179b32c9b60103d9c4e6a4364f10f6286c963904, dated 2026-08-04). Bumps arrow/parquet from 58.4.0 to 59.1.0 to match what DataFusion main uses.object_storestays at 0.13.2 (DF main is still on that).Not ready for merge, opened as a draft so CI runs against latest DF main and breaking changes are surfaced continuously. The pin is bumped periodically to keep pace with main.
Changes
native/Cargo.toml: swapdatafusion,datafusion-datasource,datafusion-physical-expr-adapter,datafusion-sparkfrom crates.io54.1.0togit = "https://github.com/apache/datafusion", rev = "..."native/core/Cargo.toml: same treatment for the dev-dependencydatafusion-functions-nestedarrowandparquetworkspace pins to59.1.0iceberg-scanfeature (see below)Adaptations made
Compile breakage from pinning to DataFusion main + arrow 59 has been worked through:
GroupsAccumulator::convert_to_stateis now a required trait method andsupports_convert_to_state()has been removed. That gate is what previously kept skip-partial-aggregation away from Comet's accumulators, so stubbingconvert_to_statewithnot_impl_errwould now fail at runtime once the probe fires — a behavior regression, not just a compile fix. It is instead implemented for all 13 accumulators (avg, avg_decimal, correlation, covariance, hll_plus_plus, percentile, stddev, sum_decimal, sum_int ×3, variance, merge_as_partial), via a sharedconvert_to_state_per_rowhelper that accumulates each row into its own group on a freshly created accumulator, so the emitted state columns matchstate()by construction.avgbuilds its state arrays directly because it holds a non-cloneableavg_fn.MergeAsPartialGroupsAccumulatorreturns its input verbatim, since that input is already the inner accumulator's intermediate state.opt_filterfromGroupsAccumulator::merge_batchimpls and their delegating call sites to match the upstream signature change (avg, avg_decimal, correlation, covariance, hll_plus_plus, percentile, stddev, sum_decimal, sum_int, variance, merge_as_partial).MutableArrayData::extend/extend_nullstotry_extend/try_extend_nullsacrossarray_funcsandcopy.rs(arrow 59 returns aResultinstead of panicking).DiskManagerAPI, wherecreate_tmp_filereturnsArc<dyn SpillFile>andpath()returnsOption<&Path>.TableSchema::from_file_schema/with_table_partition_colswith theTableSchemabuilder.FixedSizeBinaryArray::try_from(arrow 59 dropped theFromimpl).FileMetadataCacheis now a type alias, soArc<dyn FileMetadataCache>becomesArc<FileMetadataCache>.UnnestOptions::preserve_nullswas replaced by aNullHandlingenum. Uses the backward-compatiblewith_preserve_nullssetter so behavior is unchanged.arrow::ipc::writer::CompressionContextis deprecated in favor ofIpcWriteContext.Native Iceberg scan is feature-gated
iceberg-rust — including its
mainbranch — is still on arrow 58, so itsRecordBatchdoes not unify with the arrow 59 that DataFusion main requires. That single type mismatch was enough to stop the wholedatafusion-cometcrate from compiling, which in turn prevented CI from surfacing any other breakage as the DF pin advanced — defeating the purpose of this branch.The native Iceberg scan is therefore behind a new
iceberg-scancargo feature, off by default:RecordBatchmismatch iniceberg_scan.rsis the only remaining error, so re-enabling is a one-flag change once iceberg-rust upgrades to arrow 59.IcebergScanoperator in a build without the feature returns an explanatory error rather than failing obscurely.iceberg/iceberg-storage-opendalare now optional dependencies. The gating is confined to the module declaration, the Iceberg-only planner helpers, the Iceberg half of the S3 credential bridge, and the three Iceberg planner tests.This must be reverted (feature made default again) before this PR is mergeable.
Test plan
cd native && cargo check --workspace --all-targetscompiles cleancd native && cargo clippy --workspace --all-targets -- -D warningscleanmake test-rustpasses — except 3 pre-existing failures indatafusion-comet-jni-bridge's panic-handling tests (error_from_panic,jlong_panic_exception,jint_array_panic_exception), which fail identically onapache/mainand are unrelated to this branchmake test-jvmpasses on default profileNotes for reviewers
While adapting
UnnestOptions, I noticed thatexplode_outermaps toNullHandling::Preserve, which produces no output rows for an empty array, whereas Spark'sexplode_outeremits a null row for empty arrays too. DataFusion main now expresses that withNullHandling::PreserveAndExpandEmpty. This is a pre-existing gap onmainrather than upgrade fallout, so behavior is left unchanged here; it likely deserves its own issue.