fix: nullability mismatch errors in aggregation when input batches are stricter than the plan schema - #24237
Closed
alamb wants to merge 3 commits into
Closed
fix: nullability mismatch errors in aggregation when input batches are stricter than the plan schema#24237alamb wants to merge 3 commits into
alamb wants to merge 3 commits into
Conversation
… stricter than the plan schema DataFusion allows input batches to carry data types that are stricter than the plan schema declares (see `Field::contains`), most commonly a nested struct field that is non-nullable even though the schema declares it nullable. Aggregate output and state columns are derived from the actual input data, so that stricter type propagated into the emitted columns and failed the exact type equality check in `RecordBatch::try_new` against the statically computed output/spill schema, e.g. for `array_agg` / `array_agg(DISTINCT)` over such structs on the spill path. * Add `new_batch_conforming_schema`, which casts columns whose type is contained by (but not equal to) the declared field type -- a metadata-only change -- and use it wherever aggregation assembles output or spill batches. * `ArrayAggGroupsAccumulator` now builds its `ListArray` item field from the actual values' data type instead of the planned type, which panicked in `ListArray::new` for the same reason. Closes apache#24069 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
functions
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24237 +/- ##
==========================================
- Coverage 80.99% 80.99% -0.01%
==========================================
Files 1106 1106
Lines 383344 383488 +144
Branches 383344 383488 +144
==========================================
+ Hits 310482 310596 +114
- Misses 54543 54556 +13
- Partials 18319 18336 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… fix This reverts commit 83ebe75f757a4b9430bc9e35de8b71bcbe825fa7. Review feedback on apache#24237: tolerating stricter column types at each aggregate batch-assembly site treats the symptom. An end-to-end reproducer (next commit) shows the same schema divergence also breaks `GroupValues`' row converter and `interleave` inside accumulators, so the fix needs to address the root cause instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eclared schema DataFusion accepts batches whose data types are stricter than the declared schema (`MemTable::try_new` validates with `Schema::contains`, which permits a non-nullable field where the schema declares a nullable one, including nested fields). Aggregating a struct column from such batches fails in several independent places; see apache#24069. The new tests register a `MemTable` whose declared schema marks a nested struct field nullable while the batches carry a non-nullable one, and run `array_agg` / `array_agg(DISTINCT)` over it via SQL, with and without a memory limit that forces spilling. They currently fail (this commit is the reproducer only): * `array_agg`, with and without spilling: panic in `ListArray::new` (`ArrayAggGroupsAccumulator::evaluate` builds the list item field from the planner-declared type, the values from the actual data) * `array_agg(DISTINCT)` without spilling: `RowConverter column schema mismatch` (the `single_distinct_to_groupby` rewrite turns the struct into a GROUP BY key; `GroupValues`' row converter is built from the declared schema but fed the stricter runtime arrays) * `array_agg(DISTINCT)` with spilling passes on current main, showing how configuration-dependent the failures are Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
core
Contributor
Author
|
This is slop code, we should do the right thing starting with the end to end test |
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
2 participants
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.
Which issue does this PR close?
GroupedHashAggregateStream::emitthrows ArrowError: column types must match schema types #24069.Rationale for this change
As reported in #24069 (found via DataFusion Comet, apache/datafusion-comet#5239),
collect_list/collect_set(array_agg/array_agg(DISTINCT)) over a struct column fails during aggregation withThe trigger is input batches whose data types are stricter than the plan schema declares — most commonly a nested struct field that is non-nullable even though the schema declares it nullable. This is explicitly allowed by the Arrow schema containment contract —
Field::contains(“if nullability doesn't match, self needs to be nullable”) andSchema::contains(“any record that conforms toothershould also conform toself”), which is also the checkRecordBatch::with_schemauses to accept a stricter batch under a wider schema — and happens in practice when batches come from FFI / engines like Comet, whose runtime arrays can be stricter than the declared plan schema.What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
Queries that previously failed (or panicked) with the above error now work. No API changes.