fix: preserve the input list's inner field in array_append/prepend/replace

[timsaucer]

Which issue does this PR close?

• Closes array_append / array_prepend / array_replace* discard the input list's inner field, contradicting their promised return type #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 (5b22857, #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:

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 fix: correct list field inner type in array functions #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.

[timsaucer]

This is important because if I append a nullable element to a list of non-nullable elements then the output should be a list of nullable elements.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 82.58427% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.19%. Comparing base (9f377c4) to head (1575a5c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/functions-nested/src/replace.rs	79.78%	15 Missing and 4 partials ⚠️
datafusion/functions-nested/src/concat.rs	83.87%	6 Missing and 4 partials ⚠️
datafusion/functions-nested/src/extract.rs	0.00%	0 Missing and 1 partial ⚠️
datafusion/functions-nested/src/utils.rs	95.23%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #24365    +/-   ##
========================================
  Coverage   81.18%   81.19%            
========================================
  Files        1109     1110     +1     
  Lines      388167   388737   +570     
  Branches   388167   388737   +570     
========================================
+ Hits       315118   315618   +500     
- Misses      54507    54530    +23     
- Partials    18542    18589    +47     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR fixes a type/Arrow-schema mismatch in the nested array functions array_append, array_prepend, array_replace, array_replace_n, and array_replace_all by ensuring their kernels construct output list arrays using the same inner FieldRef (name, metadata, and nullability) that is promised during planning, widening the inner-field nullability only when the appended/replacement element can be null.

Changes:

• Implement return_field_from_args for the five functions to preserve the input list’s inner field and widen element nullability only when required.
• Update the corresponding kernels to build GenericListArray using the promised return field (rather than recreating Field::new_list_field(..., true)).
• Add shared helpers (list_inner_field, list_type_with_element) and expand SLT coverage (including LargeList, nested value types, and guard cases for unaffected array_concat).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
datafusion/functions-nested/src/utils.rs	Adds helpers to extract/preserve list inner fields and to widen inner nullability when needed.
datafusion/functions-nested/src/concat.rs	Uses return_field_from_args for append/prepend and threads promised inner field into kernels; keeps array_concat behavior unchanged.
datafusion/functions-nested/src/replace.rs	Uses return_field_from_args for replace* and constructs output arrays using the promised inner field (including short-circuit/null paths).
datafusion/functions-nested/src/extract.rs	Refactors array_slice inner-field extraction to reuse the new list_inner_field helper.
datafusion/sqllogictest/test_files/array/array_append.slt	Adds regression tests for field-name/metadata preservation and inner-nullability widening behavior.
datafusion/sqllogictest/test_files/array/array_prepend.slt	Adds regression tests mirroring append coverage (including LargeList and nullable element cases).
datafusion/sqllogictest/test_files/array/array_replace.slt	Adds regression tests for replace* variants (including max short-circuit and NULL max).
datafusion/sqllogictest/test_files/array/array_concat.slt	Adds guard tests confirming array_concat still derives the default inner field regardless of inputs.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[alamb]

LOooks good to me (and I had claude code review it too)

[alamb]

Is the idea that if from is null, then it wouldn't match anything anyways (and leave the array_field) unchanged? So thus the _from_field nullability is ignored?

[timsaucer]

Yes, exactly. from is only used to find the replacement match. It cannot impact the output. The one place this may seem non-obvious is the case where you have an array that is not nullable, and you have a from that is null and a to that is not nullable. The output remains a non-nullable list.

> set datafusion.format.types_info = true;
0 row(s) fetched.
Elapsed 0.002 seconds.

> select array_replace(arrow_cast(column1, 'List(non-null Int64)'), NULL, 3) from values (make_array(1, 2, 2));
+-------------------------------------------------------------------------------+
| array_replace(arrow_cast(column1,Utf8("List(non-null Int64)")),NULL,Int64(3)) |
| List(non-null Int64)                                                          |
+-------------------------------------------------------------------------------+
| [1, 2, 2]                                                                     |
+-------------------------------------------------------------------------------+
1 row(s) fetched.

[alamb]

Thank you @timsaucer