apache / apache/datafusion-comet
[EPIC] DataFusion 55 upgrade follow-ups
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 190
Description
### What is the problem the feature request solves?
#5262 bumps DataFusion to 55.0 and Arrow/Parquet to 59.2. It is a large branch that has accumulated review findings which are not blockers for the bump itself, but which should not be lost when it merges. Several had no issue at all, and the ones that did were not linked to each other.
This epic collects everything raised in review of #5262 that is deliberately **not** being fixed on the branch, so the branch can merge on the strength of the bump alone.
**What is already done on the branch**, and so is not listed below:
- Compiler migrations for the new DataFusion/arrow APIs (`GroupsAccumulator`, `IpcWriteContext`, `FixedSizeBinaryArray::try_from`, `ExecutionPlan::apply_expressions`, the iceberg-rust builder/getter APIs).
- `spark_sqrt`, needed because DataFusion 55 added `validate_sqrt_input`, so `sqrt(-1)` now errors where DataFusion 54 returned NaN.
- #4859 nested Parquet leaf pruning, via `is_pure_structural_narrowing`.
- The Celeborn map-key assertion for Arrow 59.2 (#5697).
- Iceberg partition spec/value/source-column consistency, which was failing 10 tests across the two Iceberg CI jobs.
### Describe the potential solution
Each item is a linked issue; check it here when it closes.
**Correctness**
- [ ] #5701 `array_distinct` and `array_union` diverge from Spark on `-0.0` for Spark versions without SPARK-54918. Needs a runtime, patch-level version check: the fix versions are 4.2.0, 4.1.4 and 4.0.5, so a compile-time shim keyed on the minor version is not enough, and the existing string comparison of `SPARK_VERSION` breaks at 4.0.10.
- [ ] #5707 Nested Parquet cast retention accepts an ambiguous case-insensitive field match, so a query Spark rejects can silently return one of two candidate fields.
**Metrics and memory accounting**
- [ ] #5703 Re-enable the two ignored `CometAggregateSuite` metric tests after the DataFusion 55 `peak_mem_used` change (apache/datafusion#22710).
- [ ] #5704 Establish whether the TPC-H off-heap bump from 2g to 4g is a real requirement, and changelog it if so.
**Error handling**
- [x] #5705 `SpillWriter::path()` conflates "nothing spilled" with "backend has no local path", so a pathless spill backend would silently drop spilled data.
- [x] #5706 `copy_array` panics on offset overflow instead of returning an error, aborting the JVM rather than failing the task.
**Performance**
- [ ] #5708 Slice arrays before normalizing, to avoid allocating over the whole backing buffer.
**Test hygiene**
- [ ] #5702 Signed-zero array fixtures claim the literal case agrees with Spark when it does not.
Moved initial followups from #4865
- [ ] #4680
- [x] #4131
- [ ] #5040
- [ ] https://github.com/apache/datafusion-comet/pull/4587
- [ ] https://github.com/apache/datafusion-comet/issues/5210
### Additional context
Review threads on #5262 from @sunchao, @andygrove, @comphead and @ajsquared. The threads opened by `ziting-openai` were withdrawn as the output of a misconfigured agent; where a finding in them was independently confirmed it is captured above, and the rest are not carried forward.
Contributor guide
Research direction
Start by choosing one unchecked follow-up, such as #5701, #5702, #5703, #5704, #5707, or #5708, and read that issue alongside the review context in #5262. Run the tests or benchmarks named by the selected issue; done means that follow-up is resolved and its checkbox is checked in this epic.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, scala
- Domain
- data-engineering, performance, testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100