You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "neilconway (via GitHub)" <gi...@apache.org> on 2026/09/18 15:14:18 UTC
[PR] perf: Improve efficiency of `array_resize` for sliced arrays [datafusion]
neilconway opened a new pull request, #25479:
URL: https://github.com/apache/datafusion/pull/25479
## Which issue does this PR close?
- N/A
## Rationale for this change
`array_resize` prepares fill/padding values for the situations in which it needs to grow the input arrays. When (a) no fill value was provided (b) no row needs to grow, the code allocated an all-NULL array equal to the size of the underlying backing array of the to-be-resized input. This is redundant, because no fill is needed; it's also potentially expensive, because the backing array might be much larger than the actual visible portion of the input.
Benchmarks:
- Shrink 500 → 10, omitted fill: 33.79 → 10.61 µs — 3.2× faster.
- Shrink 10 → 5, omitted fill, 10,000 backing elements: 13.63 → 11.25 µs — about 17% less time.
- Shrink 10 → 5, omitted fill, 1,000,000 backing elements: 43.36 → 11.24 µs — 3.9× faster.
Other benchmark cases were within noise.
## What changes are included in this PR?
* Refactor `build_resized_list` to make the fill data `Optional`
* When no fill data is needed, don't supply any
* Add an assert that fill data is supplied if it is needed
* Add benchmark
* Add unit tests
## What is the testing strategy for this PR?
Benchmarked to confirm performance improvement. Existing tests pass. Added new unit tests to improve code coverage; they are not intended to catch the allocation change though.
## Are there any user-facing changes?
No.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Improve efficiency of `array_resize` for sliced arrays [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #25479:
URL: https://github.com/apache/datafusion/pull/25479#discussion_r4053363096
##########
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:
Thanks for the suggestion! I actually started with the chained filter but I thought writing it inline was a bit mysterious, so I'm inclined to keep the current version.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Improve efficiency of `array_resize` for sliced arrays [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #25479:
URL: https://github.com/apache/datafusion/pull/25479
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Improve efficiency of `array_resize` for sliced arrays [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #25479:
URL: https://github.com/apache/datafusion/pull/25479#discussion_r4053363096
##########
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:
Thanks for the suggestion! I actually started with the chained filter but I thought writing it inline was a bit mysterious.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Improve efficiency of `array_resize` for sliced arrays [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25479:
URL: https://github.com/apache/datafusion/pull/25479#issuecomment-5732399202
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25479?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `96.15385%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.36%. Comparing base ([`3ab72e6`](https://app.codecov.io/gh/apache/datafusion/commit/3ab72e61e38259f8c52e1937a9c90e7799d7b221?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`be2d031`](https://app.codecov.io/gh/apache/datafusion/commit/be2d031ed3c30c0127289950fb938afb94b10f71?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 10 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25479?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/functions-nested/src/resize.rs](https://app.codecov.io/gh/apache/datafusion/pull/25479?src=pr&el=tree&filepath=datafusion%2Ffunctions-nested%2Fsrc%2Fresize.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mdW5jdGlvbnMtbmVzdGVkL3NyYy9yZXNpemUucnM=) | 96.15% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25479?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25479 +/- ##
==========================================
+ Coverage 82.34% 82.36% +0.02%
==========================================
Files 1137 1137
Lines 432364 432978 +614
Branches 432364 432978 +614
==========================================
+ Hits 356020 356619 +599
Misses 54830 54830
- Partials 21514 21529 +15
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25479?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Improve efficiency of `array_resize` for sliced arrays [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #25479:
URL: https://github.com/apache/datafusion/pull/25479#discussion_r4053377416
##########
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:
Thanks, done.
##########
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:
Thanks, done.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] perf: Improve efficiency of `array_resize` for sliced arrays [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org