Repository navigation
fix: array_concat panics or fails on non-default nested lists - #25970
Conversation
array_concat planned nested list types from the leaf element type with default inner fields, while execution kept the input fields. LargeList or renamed inner lists failed a type assertion, all-NULL arguments of different depths returned the wrong depth, and non-nullable inner elements panicked. Plan from the dimension-aligned argument types and build the output with the planned type. Signed-off-by: 1fanwang <1fannnw@gmail.com>
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25970 +/- ##
==========================================
+ Coverage 82.59% 82.61% +0.01%
==========================================
Files 1145 1147 +2
Lines 444194 444775 +581
Branches 444194 444775 +581
==========================================
+ Hits 366897 367445 +548
+ Misses 55029 55006 -23
- Partials 22268 22324 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Hasnaathussain
left a comment
Hasnaathussain
left a comment
There was a problem hiding this comment.
Thanks, 1fanwang. I compared fourteen SQL cases on fca52aa59 and c3fcddbc2 under WSL/Rust 1.98.1: nine fail on the parent and all pass here, including mixed-depth List/LargeList, nested nulls and FixedSizeList inputs. The probe checks actual/planned type agreement and values against typed SQL literals; this isn't a full-suite result.
One contract detail: the one-dimensional output type changes too when the input fields already match. This query returns List(Int64) on the parent, but List(non-null Int64, field: 'element') here; the value remains [1]:
SELECT arrow_typeof(array_concat(
arrow_cast([1], 'List(non-null Int64, field: ''element'')')
));The same happens with two inputs carrying that field. The description says one-dimensional concatenation is unchanged, and the SLT comment still describes a default nullable item field for all one-dimensional inputs. If retaining this metadata is intentional, could you call it out in the description and add a small SLT case for identical named/non-nullable inputs? This is a contract/test clarification, not a value regression.
One-dimensional inputs that share a named or non-nullable field kept that field in the result, which changed the result type from main. Plan one-dimensional results from the unified element type with the default nullable field again, and pin it in array_concat.slt. Signed-off-by: 1fanwang <1fannnw@gmail.com>
@Hasnaathussain It wasn't intentional. 07cf61a restores main's default item field for one-dimensional results, and array_concat.slt now pins List(Int64) for that case. Nested results still keep their inner fields, which the issue needs. |
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Which issue does this PR close?
array_concaton nested lists panics or fails with type mismatch errors #25948.Rationale for this change
array_concat on nested lists crashes or fails in cases that work with default inner fields. Concatenating a list whose inner lists have non-nullable elements with a one-dimensional array panics the process with "Arrays with inconsistent types passed to MutableArrayData". Nested lists whose inner list is a LargeList or has a non-default field name, and all-NULL arguments of different nesting depths, fail with an internal error saying array_concat returned a different type than the one promised at planning time.
Planning built the result type from the leaf element type with default nullable inner fields, while execution kept the inner fields of the inputs, so the planned and executed types disagreed, and a non-nullable inner field met a nullable one in the same array builder.
What changes are included in this PR?
array_concat now plans its result from the argument types aligned to the deepest argument and unified with the same type union as before, so inner LargeList lists, field names and nullability carry through. Each argument is coerced to that type or to the matching lower-dimensional part of it, and execution builds the output, including an all-NULL result, with the planned type. Concatenating one-dimensional arrays is unchanged.
What is the testing strategy for this PR?
New cases in array/array_concat.slt cover each failure from the issue: a nested LargeList, an inner list with a non-default field name, all-NULL arguments of different depths, and non-nullable inner elements concatenated with a one-dimensional array. Another case pins the one-dimensional result type for inputs that share a named, non-nullable field.
Testing Done
The issue's queries in datafusion-cli, built with cargo build --profile ci -p datafusion-cli on main (416002a) and on this branch. The cargo registry path in the panic message is shortened.
On main, the first three queries fail with internal type errors and the fourth panics:
On this branch, all four queries return results:
One-dimensional results keep the types they have on main, including inputs that share a named or non-nullable field:
On main:
On this branch:
Reproducer source: repro.sql
Reproducer source: one-dimensional.sql
Are there any user-facing changes?
Yes. These array_concat calls now return results instead of panicking or failing with internal errors, and nested results keep the inner list types of their inputs. No API changes.