fix: correct list field inner type in array functions - #24345
Merged
Conversation
github-actions
Bot
added
sqllogictest
Member
Author
|
If this is approved I'll create a PR targeting |
This was referenced Aug 13, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24345 +/- ##
========================================
Coverage 81.17% 81.17%
========================================
Files 1109 1109
Lines 388033 388167 +134
Branches 388033 388167 +134
========================================
+ Hits 314985 315107 +122
- Misses 54509 54513 +4
- Partials 18539 18547 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adriangb
approved these changes
Aug 14, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me, thanks for the fix!
One finding out of scope for this PR: array_append, array_prepend and array_replace{,_n,_all} have the same defect and regressed in the same commit. Filed as #24347. They need a slightly different fix, since the appended element can be null and so the promised type is wrong at the source, not just the payload. array_concat, array_remove, array_distinct, array_union, array_intersect, array_sort and array_resize are all fine.
+1 on the branch-55 backport
Jefffrey
approved these changes
Aug 14, 2026
- Pass `use_nulls: false` to `MutableArrayData` in both `general_array_slice` and `general_list_view_array_slice`. Neither calls `try_extend_nulls` any more, and arrow ORs the flag with the child array's null count internally, so this only avoids an unnecessary validity buffer allocation. - Name the actual internal function in the `internal_err!` messages. These paths are also reachable from `array_pop_front` / `array_pop_back`, so reporting "array_slice" was misleading. - Extend the LargeList slice and pop tests to cover empty and NULL rows so the null branch is exercised for i64 offsets, not just i32.
timsaucer
enabled auto-merge
August 14, 2026 11:06
This was referenced Aug 14, 2026
timsaucer
added a commit
that referenced
this pull request
Aug 14, 2026
Theodus
pushed a commit
to Theodus/datafusion
that referenced
this pull request
Aug 14, 2026
…place (apache#24365) ## Which issue does this PR close? - Closes apache#24347. ## Rationale for this change `array_append`, `array_prepend`, `array_replace`, `array_replace_n` and `array_replace_all` promise the input list type verbatim — inner field name, nullability and metadata included — but their kernels rebuilt the output's inner field from scratch with `Field::new_list_field(..., true)`. On debug builds this trips the return-type assertion from apache#17515; on release builds it silently yields a batch whose inner field disagrees with the schema the planner recorded. This is the other half of apache#24341, whose `array_slice` part was fixed in apache#24345. The field-name symptom is a 55.0.0 regression from the same commit (5b22857, apache#20945); the non-nullable symptom is not a regression. Unlike `array_slice`, threading the input's field through is not sufficient here: the appended, prepended or replacement element can itself be null, so a promise cloned from a `List(non-null T)` input is wrong at the source and arrow rejects the array with `Non-nullable field of ListArray cannot contain nulls`. ## What changes are included in this PR? Each of the five functions now implements `return_field_from_args`, carrying the input field's name and metadata through while widening `nullable` when the new element's argument is nullable. The kernels build their output from `args.return_field` instead of deriving a field of their own, so promise and payload come from a single source. Nullability is therefore only widened when the result can genuinely contain a null: ```sql array_append(List(non-null Int64), 3) -> List(non-null Int64) array_append(List(non-null Int64), NULL) -> List(Int64) ``` Two paths beyond those listed in the issue turned out to have the same defect and are fixed too: `array_append` / `array_prepend` with a **nested** value type (which delegates to `concat_internal`), and the `LargeList` variants of all five. Since these five now implement `return_field_from_args`, their `return_type` becomes unreachable and returns `internal_err!("return_field_from_args should be used instead")`, matching the guidance on `ScalarUDFImpl::return_type` and the existing convention in `remove.rs` and `map_values.rs`. Two small cleanups while in here: - The `List`/`LargeList` inner-field extraction added to `general_array_slice` by apache#24345 is now shared as `utils::list_inner_field`, used by all three files. Its error text is unchanged. The `ListView` variant in `general_list_view_array_slice` is deliberately left alone — folding all four variants into one helper would let `general_array_slice` silently accept a `ListView` that its match currently rejects. - `array_concat` is **not** affected and its behaviour is unchanged: it derives a fresh return type via `type_union_resolution` rather than cloning an input's, so it keeps passing `None` to `concat_internal` and deriving the field from the aligned inputs. Behaviour for `Null`-typed array arguments is unchanged in all five functions (`array_replace*` return `Null`, `array_append` / `array_prepend` return `List(element)`). ## Are these changes tested? Yes — 14 new SLT tests across `array_append.slt`, `array_prepend.slt`, `array_replace.slt`, plus two guard cases in `array_concat.slt` pinning down that it is unaffected. `arrow_cast` can express a named inner field (`'List(Int64, field: ''element'')'`), so these reproduce the field-name half of the bug without needing the Spark dialect. Coverage: inner field name preserved, non-nullable inner field preserved, nullability widened only when the new element is nullable (both literal `NULL` and a nullable column), `LargeList`, nested value types, the `max <= 0` short circuit, and a `NULL` `max`. Every one of these queries fails on `main` with the return-type assertion. Also run: `cargo clippy --all-targets --all-features -- -D warnings`, the full sqllogictest suite, and the extended workspace test suite (68 test binaries, 0 failures). The `array_replace` and `array_concat` benchmarks show no regression against `main`. ## Are there any user-facing changes? The five functions now return the inner field they promise instead of a rebuilt one, which is the bug fix. As a consequence, appending or replacing with a nullable element widens the declared inner nullability of the result (`List(non-null Int64)` -> `List(Int64)`), which is required for the result to be representable at all. No breaking changes to public APIs.
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
4 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?
Rationale for this change
There is a regression from 54.1.0 to 55.0.0rc2 where the spark function
slicecan fail as demonstrated in the updated unit test, and as described in the connected issue.What changes are included in this PR?
In the nested functions, get the inner field directly from the input for array slicing operations. Also, since it is possible the inner list could be non-nullable emit an empty slice instead of a null child element for the outer lists's nulls.
Are these changes tested?
Added 6 new tests in the SLT suite.
Are there any user-facing changes?
None