timsaucer opened a new pull request, #24365:
URL: https://github.com/apache/datafusion/pull/24365
## Which issue does this PR close?
- Closes #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 #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 #24341, whose `array_slice` part was fixed in
#24345. The field-name symptom is a 55.0.0 regression from the same commit
(5b228570, #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 #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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
