adriangb commented on code in PR #24345:
URL: https://github.com/apache/datafusion/pull/24345#discussion_r3780283247
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -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);
Review Comment:
```suggestion
MutableArrayData::with_capacities(vec![&original_data], false,
capacity);
```
I think we can make `use_nulls` false now. It seems to have only been true
because `try_extend_nulls` panics without it, and that call is gone. Arrow does
`use_nulls | arrays.iter().any(|a| a.null_count() > 0)` internally so this just
avoids allocating a validity buffer.
##########
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)
Review Comment:
This LargeList case has no NULL row, so the changed null branch is never
exercised for i64 offsets. The List test above it is the only thing covering
that code path. The shape is also inconsistent with its neighbours (call nested
inside values rather than in the projection).
```suggestion
query ?T
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)
```
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -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}");
Review Comment:
This can also be reached from `array_pop_front` / `array_pop_back`. Although
this is already done (also incorrectly) in `general_list_view_array_slice` so
it's a pre existing pattern that is being carried forward.
##########
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)
Review Comment:
```suggestion
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([], '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)
```
Similarly so we can cover the null case.
--
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]
