neilconway commented on code in PR #23775:
URL: https://github.com/apache/datafusion/pull/23775#discussion_r3632859389
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1246,49 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value. Previously the no-nulls branch unconditionally
+ // read values[start], returning the wrong element for interior empty
lists.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
+ let values: ArrayRef = Arc::new(Int32Array::from(vec![1, 2, 3]));
+ // row 0 = [1], row 1 = [] (empty, non-null), row 2 = [2, 3]
+ let offsets = OffsetBuffer::new(ScalarBuffer::from(vec![0, 1, 1, 3]));
+ let field = Arc::new(Field::new("item", DataType::Int32, true));
+
+ let list_array = ListArray::new(field, offsets, values, None);
+
+ let result = general_array_any_value(&list_array)?;
+ let result = result.as_any().downcast_ref::<Int32Array>().unwrap();
+
+ assert_eq!(result.value(0), 1);
+ assert!(result.is_null(1)); // empty list -> NULL (previously read `2`)
Review Comment:
Remove "(previously read `2`)"
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1246,49 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value. Previously the no-nulls branch unconditionally
+ // read values[start], returning the wrong element for interior empty
lists.
Review Comment:
Nit: I think comments should describe the current state of the code (which
is what future readers will care about), rather than narrate how the code has
evolved over time (that belongs in `git` commit comments).
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1236,6 +1246,49 @@ mod tests {
Ok(())
}
+ // An empty (length-0) list element that is not null must yield NULL, not
+ // the next element's value. Previously the no-nulls branch unconditionally
+ // read values[start], returning the wrong element for interior empty
lists.
+ #[test]
+ fn test_array_any_value_empty_list_element() -> Result<()> {
+ let values: ArrayRef = Arc::new(Int32Array::from(vec![1, 2, 3]));
+ // row 0 = [1], row 1 = [] (empty, non-null), row 2 = [2, 3]
+ let offsets = OffsetBuffer::new(ScalarBuffer::from(vec![0, 1, 1, 3]));
+ let field = Arc::new(Field::new("item", DataType::Int32, true));
+
+ let list_array = ListArray::new(field, offsets, values, None);
+
+ let result = general_array_any_value(&list_array)?;
+ let result = result.as_any().downcast_ref::<Int32Array>().unwrap();
+
+ assert_eq!(result.value(0), 1);
+ assert!(result.is_null(1)); // empty list -> NULL (previously read `2`)
+ assert_eq!(result.value(2), 2);
+
+ Ok(())
+ }
+
+ // A trailing empty list element has start == values.len(); the old code
did
+ // `extend(0, start, start + 1)` and panicked with an out-of-bounds slice.
Review Comment:
Remove the description of previous behavior.
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -1062,13 +1062,23 @@ where
for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
let start = offset_window[0];
+ let end = offset_window[1];
- // array is null
+ // the list element is null
if array.is_null(row_index) {
mutable.try_extend_nulls(1)?;
continue;
}
+ // the list element is empty; there is no value to take, so the result
+ // is NULL. Without this guard the no-nulls branch below would read
+ // `values[start]`, which is either the next element (wrong value) or
+ // out of bounds when `start == values.len()` (panic).
Review Comment:
Remove "Without this..."
##########
datafusion/sqllogictest/test_files/array/array_any_value.slt:
##########
@@ -145,6 +145,36 @@ select array_any_value(make_array(NULL, 1, 2, 3, 4, 5)),
array_any_value(column1
1 41
1 51
+# array_any_value with empty (length-0) list elements
+# A non-null but empty list must yield NULL, including a trailing empty element
+# whose start offset equals the values length (previously read out of bounds
+# and panicked the worker task).
Review Comment:
Remove "(previously ...)"
--
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]
