From 447fd92b85845d2d9a6dbc822ff4b5dd5d04d50c Mon Sep 17 00:00:00 2001 From: Tim Saucer Date: Thu, 13 Aug 2026 15:50:52 -0400 Subject: [PATCH 1/2] Add correction for list inner field --- datafusion/functions-nested/src/extract.rs | 22 ++++++++++----- .../test_files/array/array_pop.slt | 27 +++++++++++++++++++ .../test_files/array/array_slice.slt | 25 +++++++++++++++++ .../test_files/spark/array/slice.slt | 6 +++++ 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/datafusion/functions-nested/src/extract.rs b/datafusion/functions-nested/src/extract.rs index 9d367f0161fdd..ba5abd3d44c9a 100644 --- a/datafusion/functions-nested/src/extract.rs +++ b/datafusion/functions-nested/src/extract.rs @@ -23,9 +23,8 @@ use arrow::array::{ }; use arrow::buffer::{NullBuffer, OffsetBuffer, ScalarBuffer}; use arrow::datatypes::DataType; -use arrow::datatypes::{ - DataType::{FixedSizeList, LargeList, LargeListView, List, ListView, Null}, - Field, +use arrow::datatypes::DataType::{ + FixedSizeList, LargeList, LargeListView, List, ListView, Null, }; use datafusion_common::cast::as_large_list_array; use datafusion_common::cast::as_list_array; @@ -622,6 +621,15 @@ where let values = array.values(); let original_data = values.to_data(); let capacity = Capacities::Array(original_data.len()); + // Carry the input's list field through to the output so that the returned + // type matches the one promised by `return_type` / `return_field_from_args`, + // including the field name, nullability and metadata. + let field = match array.data_type() { + List(field) | LargeList(field) => Arc::clone(field), + other => { + return internal_err!("array_slice got unexpected data type: {other}"); + } + }; let mut mutable = MutableArrayData::with_capacities(vec![&original_data], true, capacity); @@ -638,9 +646,11 @@ where let end = offset_window[1]; let len = end - start; + // The row is null, so its contents are never observed. Emit an empty + // slice rather than a null child element: the input's list field may be + // non-nullable, in which case a null child would be invalid. if nulls.as_ref().is_some_and(|n| n.is_null(row_index)) { - mutable.try_extend_nulls(1)?; - offsets.push(offsets[row_index] + O::usize_as(1)); + offsets.push(offsets[row_index]); continue; } @@ -682,7 +692,7 @@ where let data = mutable.freeze(); Ok(Arc::new(GenericListArray::::try_new( - Arc::new(Field::new_list_field(array.value_type(), true)), + field, OffsetBuffer::::new(offsets.into()), arrow::array::make_array(data), nulls, diff --git a/datafusion/sqllogictest/test_files/array/array_pop.slt b/datafusion/sqllogictest/test_files/array/array_pop.slt index a72e566b9e7ab..4702dc9261d8b 100644 --- a/datafusion/sqllogictest/test_files/array/array_pop.slt +++ b/datafusion/sqllogictest/test_files/array/array_pop.slt @@ -318,5 +318,32 @@ select array_pop_front(arrow_cast([1, 2], 'LargeListView(Int64)')); ---- [2] +# maintains inner nullability +query ??TT +select + array_pop_front(column1), + array_pop_back(column1), + arrow_typeof(array_pop_front(column1)), + arrow_typeof(array_pop_back(column1)) +from values + (arrow_cast([], 'List(non-null Int32)')), + (arrow_cast(NULL, 'List(non-null Int32)')), + (arrow_cast([1, 3, 5, -5], 'List(non-null Int32)')) +; +---- +[] [] List(non-null Int32) List(non-null Int32) +NULL NULL List(non-null Int32) List(non-null Int32) +[3, 5, -5] [1, 3, 5] List(non-null Int32) List(non-null Int32) + +query ??TT +select + array_pop_front(column1), + array_pop_back(column1), + arrow_typeof(array_pop_front(column1)), + arrow_typeof(array_pop_back(column1)) +from values (arrow_cast([1, 3, 5, -5], 'LargeList(non-null Int32)')); +---- +[3, 5, -5] [1, 3, 5] LargeList(non-null Int32) LargeList(non-null Int32) + include ./cleanup.slt.part diff --git a/datafusion/sqllogictest/test_files/array/array_slice.slt b/datafusion/sqllogictest/test_files/array/array_slice.slt index 14587a50b2266..abd5304788e9e 100644 --- a/datafusion/sqllogictest/test_files/array/array_slice.slt +++ b/datafusion/sqllogictest/test_files/array/array_slice.slt @@ -450,6 +450,31 @@ NULL NULL [1, 3, 5] +# maintains inner nullability +query ?T +select array_slice(column1, 2, 3), arrow_typeof(array_slice(column1, 2, 3)) +from values + (arrow_cast([], 'List(non-null Int32)')), + (arrow_cast(NULL, 'List(non-null Int32)')), + (arrow_cast([1, 3, 5, -5], 'List(non-null Int32)')) +; +---- +[] List(non-null Int32) +NULL List(non-null Int32) +[3, 5] List(non-null Int32) + +query ?T +select column1, arrow_typeof(column1) +from values (array_slice(arrow_cast([1, 3, 5, -5], 'LargeList(non-null Int32)'), 2, 3)); +---- +[3, 5] LargeList(non-null Int32) + +query ?T +select array_slice(column1, 2, 3, 2), arrow_typeof(array_slice(column1, 2, 3, 2)) +from values (arrow_cast([1, 3, 5, -5], 'List(non-null Int32)')); +---- +[3] List(non-null Int32) + # Testing with empty arguments should result in an error query error DataFusion error: Error during planning: 'array_slice' does not support zero arguments select array_slice(); diff --git a/datafusion/sqllogictest/test_files/spark/array/slice.slt b/datafusion/sqllogictest/test_files/spark/array/slice.slt index aaf4aa4909dfd..f6fb431a0769b 100644 --- a/datafusion/sqllogictest/test_files/spark/array/slice.slt +++ b/datafusion/sqllogictest/test_files/spark/array/slice.slt @@ -152,3 +152,9 @@ query ? SELECT slice(make_array(1), 3, 4) ---- [] + +# the inner field name of the input list is preserved +query ?T +SELECT slice(array(1, 2, 3, 4), 2, 2), arrow_typeof(slice(array(1, 2, 3, 4), 2, 2)); +---- +[2, 3] List(Int64, field: 'element') From 7bd8af61f4088f806246bd59d02346cc777d4ee0 Mon Sep 17 00:00:00 2001 From: Tim Saucer Date: Fri, 14 Aug 2026 07:00:49 -0400 Subject: [PATCH 2/2] Address PR review feedback on list slice fix - 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. --- datafusion/functions-nested/src/extract.rs | 16 ++++++++++++---- .../sqllogictest/test_files/array/array_pop.slt | 8 +++++++- .../test_files/array/array_slice.slt | 10 ++++++++-- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/datafusion/functions-nested/src/extract.rs b/datafusion/functions-nested/src/extract.rs index ba5abd3d44c9a..8f2ea1f40dcb2 100644 --- a/datafusion/functions-nested/src/extract.rs +++ b/datafusion/functions-nested/src/extract.rs @@ -627,12 +627,17 @@ where let field = match array.data_type() { List(field) | LargeList(field) => Arc::clone(field), other => { - return internal_err!("array_slice got unexpected data type: {other}"); + return internal_err!( + "general_array_slice got unexpected data type: {other}" + ); } }; + // `use_nulls` is false because we never call `try_extend_nulls`: null rows are + // emitted as empty slices. Arrow still allocates a validity buffer on its own + // if the child array has nulls. let mut mutable = - MutableArrayData::with_capacities(vec![&original_data], true, capacity); + MutableArrayData::with_capacities(vec![&original_data], false, capacity); // We have the slice syntax compatible with DuckDB v0.8.1. // The rule `adjusted_from_index` and `adjusted_to_index` follows the rule of array_slice in duckdb. @@ -714,12 +719,15 @@ where let field = match array.data_type() { ListView(field) | LargeListView(field) => Arc::clone(field), other => { - return internal_err!("array_slice got unexpected data type: {}", other); + return internal_err!( + "general_list_view_array_slice got unexpected data type: {other}" + ); } }; + // See the note on `use_nulls` in `general_array_slice`. let mut mutable = - MutableArrayData::with_capacities(vec![&original_data], true, capacity); + MutableArrayData::with_capacities(vec![&original_data], false, capacity); // We must build `offsets` and `sizes` buffers manually as ListView does not enforce // monotonically increasing offsets. diff --git a/datafusion/sqllogictest/test_files/array/array_pop.slt b/datafusion/sqllogictest/test_files/array/array_pop.slt index 4702dc9261d8b..0b7ebf75f4b2b 100644 --- a/datafusion/sqllogictest/test_files/array/array_pop.slt +++ b/datafusion/sqllogictest/test_files/array/array_pop.slt @@ -341,8 +341,14 @@ select array_pop_back(column1), arrow_typeof(array_pop_front(column1)), arrow_typeof(array_pop_back(column1)) -from values (arrow_cast([1, 3, 5, -5], 'LargeList(non-null Int32)')); +from values + (arrow_cast([], 'LargeList(non-null Int32)')), + (arrow_cast(NULL, 'LargeList(non-null Int32)')), + (arrow_cast([1, 3, 5, -5], 'LargeList(non-null Int32)')) +; ---- +[] [] LargeList(non-null Int32) LargeList(non-null Int32) +NULL NULL LargeList(non-null Int32) LargeList(non-null Int32) [3, 5, -5] [1, 3, 5] LargeList(non-null Int32) LargeList(non-null Int32) diff --git a/datafusion/sqllogictest/test_files/array/array_slice.slt b/datafusion/sqllogictest/test_files/array/array_slice.slt index abd5304788e9e..76b81b28efc58 100644 --- a/datafusion/sqllogictest/test_files/array/array_slice.slt +++ b/datafusion/sqllogictest/test_files/array/array_slice.slt @@ -464,9 +464,15 @@ NULL List(non-null Int32) [3, 5] List(non-null Int32) query ?T -select column1, arrow_typeof(column1) -from values (array_slice(arrow_cast([1, 3, 5, -5], 'LargeList(non-null Int32)'), 2, 3)); +select array_slice(column1, 2, 3), arrow_typeof(array_slice(column1, 2, 3)) +from values + (arrow_cast([], 'LargeList(non-null Int32)')), + (arrow_cast(NULL, 'LargeList(non-null Int32)')), + (arrow_cast([1, 3, 5, -5], 'LargeList(non-null Int32)')) +; ---- +[] LargeList(non-null Int32) +NULL LargeList(non-null Int32) [3, 5] LargeList(non-null Int32) query ?T