jayzhan211 commented on code in PR #25479:
URL: https://github.com/apache/datafusion/pull/25479#discussion_r4052089179
##########
datafusion/functions-nested/src/resize.rs:
##########
@@ -335,6 +338,10 @@ where
let count = O::usize_as(count);
let start = offset_window[0];
if start + count > offset_window[1] {
+ debug_assert!(
Review Comment:
`debug_assert!` is compiled out in release; a violated invariant then
becomes an index-OOB panic inside `MutableArrayData::try_extend(1, ..)`. Return
an internal error instead:
```diff
- debug_assert!(
- default_value_data.is_some(),
- "fill values are required when growing a list"
- );
+ if default_value_data.is_none() {
+ return internal_err!(
+ "array_resize: fill values are required when growing a
list"
+ );
+ }
```
##########
datafusion/functions-nested/src/resize.rs:
##########
@@ -269,27 +269,26 @@ fn general_list_resize<O: OffsetSizeTrait + TryInto<i64>>(
count_array,
field,
&original_data,
- &default_value_data,
+ Some(&default_value_data),
output_values_len,
|mutable, _, extra_count| Ok(mutable.try_extend(1, 0,
extra_count)?),
)
} else {
- // Slow path: rows may need different fill values, so append from the
- // corresponding slot in the input fill array for each grown element.
- let fill_values = match default_element {
- Some(fill_values) => fill_values,
- None => {
- let null_scalar = ScalarValue::try_from(&data_type)?;
- null_scalar.to_array_of_size(original_data.len())?
- }
+ // Handle batches with no growth or with different fill values per row.
+ // Growing rows repeat the fill value from their corresponding input
slot.
+ // Growth without a fill argument always takes the bulk path above, so
+ // `default_element` is present whenever a row grows here.
+ let default_value_data = if max_extra > 0 {
+ default_element.map(|fill_values| fill_values.to_data())
+ } else {
+ None
Review Comment:
```suggestion
let default_value_data = default_element
.filter(|_| max_extra > 0)
.map(|fill_values| fill_values.to_data());
```
##########
datafusion/functions-nested/benches/array_resize.rs:
##########
@@ -80,6 +80,36 @@ fn criterion_benchmark(c: &mut Criterion) {
&config_options,
);
+ bench_case(
+ &mut group,
+ "shrink_default_null_fill_500_to_10",
+ &[
+ ColumnarValue::Array(create_int64_list_array(NUM_ROWS, 500)),
+ ColumnarValue::Array(repeated_int64_array(10)),
+ ],
+ &two_arg_fields,
+ &return_field,
+ &config_options,
+ );
+
+ // Keep the visible rows and output size fixed while varying the backing
+ // child size. Slicing a ListArray retains the entire child array.
+ for backing_rows in [NUM_ROWS, NUM_ROWS * 100] {
+ let array = create_int64_list_array(backing_rows, 10)
+ .slice((backing_rows - NUM_ROWS) / 2, NUM_ROWS);
+ bench_case(
+ &mut group,
+
&format!("shrink_default_null_fill_sliced_10_to_5_backing_{backing_rows}"),
Review Comment:
Label is backing list rows (1000 / 100000), but the comment and PR
description refer to backing child size (10k / 1M elements). Make them agree:
```diff
-
&format!("shrink_default_null_fill_sliced_10_to_5_backing_{backing_rows}"),
+ &format!(
+ "shrink_default_null_fill_sliced_10_to_5_backing_values_{}",
+ backing_rows * 10
+ ),
```
--
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]
