Skip to content

perf: Fix quadratic behavior of to_array_of_size - #20459

Merged
alamb merged 9 commits into
apache:mainfrom
neilconway:neilc/optimize-to-array-of-size
Feb 24, 2026
Merged

perf: Fix quadratic behavior of to_array_of_size#20459
alamb merged 9 commits into
apache:mainfrom
neilconway:neilc/optimize-to-array-of-size

Conversation

@neilconway

@neilconwayneilconway commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

When array_to_size(n) was called on a List-like object containing a StringViewArray with b data buffers, the previous implementation returned a list containing a StringViewArray with n*b buffers, which results in catastrophically bad performance if b grows even somewhat large.

This issue was previously noticed causing poor nested loop join performance. #18161 adjusted the NLJ code to avoid calling to_array_of_size for this reason, but didn't attempt to fix the underlying issue in to_array_of_size. This PR doesn't attempt to revert the change to the NLJ code: the special-case code added in #18161 is still slightly faster than to_array_of_size after this optimization. It might be possible to address that in a future PR.

What changes are included in this PR?

  • Instead of using repeat_n + concat to merge together n copies of the StringViewArray, we instead use take, which preserves the same number of buffers as the input StringViewArray.
  • Add a new benchmark for this situation
  • Add more unit tests for to_array_of_size

Are these changes tested?

Yes and benchmarked.

Are there any user-facing changes?

No.

AI usage

Iterated on the problem with Claude Code; I understand the problem and the solution.

@github-actionsgithub-actionsBot added the common Related to common crate label Feb 20, 2026
@neilconway

Copy link
Copy Markdown
ContributorAuthor

Benchmarks:

group array-of-size-opt array-of-size-vanilla
----- ----------------- ---------------------
list_to_array_of_size/1_buffer/1024 1.00 1870.0±30.72µs ? ?/sec 3.77 7.1±0.05ms ? ?/sec
list_to_array_of_size/1_buffer/256 1.00 337.9±4.16µs ? ?/sec 1.77 599.8±5.17µs ? ?/sec
list_to_array_of_size/50_buffers/1024 1.00 1945.4±23.28µs ? ?/sec 65.93 128.3±1.06ms ? ?/sec
list_to_array_of_size/50_buffers/256 1.00 364.7±4.31µs ? ?/sec 22.76 8.3±0.07ms ? ?/sec

@neilconway

Copy link
Copy Markdown
ContributorAuthor

I see #18159 already exists for this issue; I'll be optimistic and claim this PR closes it... 😅

@neilconway

neilconway commented Feb 21, 2026

Copy link
Copy Markdown
ContributorAuthor

We could consider backing out the special-case logic in NLJ that was introduced in #18161, but that will require some consideration and benchmarking first. I tried a quick hack to remove the NLJ workaround but it hurt performance on a microbenchmark by ~15%

@comphead

Copy link
Copy Markdown
Contributor

@Dandandan FYI

@compheadcomphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @neilconway the solution is elegant and makes a lot of sense

Comment threaddatafusion/common/src/scalar/mod.rs
@github-actionsgithub-actionsBot added the physical-plan Changes to the physical-plan crate label Feb 22, 2026
// the result is `[[A, B, C], [A, B, C], [A, B, C]]`.
//
// Given `arr = [[A, B], [C]]` and `size = 2`, `indices = [0, 1, 0, 1]` and the
// result is `[[A, B], [C], [A, B], [C]]`. (But in practice, we are always called

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to explicitly check that arr.len() == 1 so we don't need to cover edge cases that aren't meant to occur? (Which can lead to a bit simpler/readable code here)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm inclined to leave it as-is: with the comment giving some examples, the behavior seems reasonably clear to me, and it's not more code to handle the arr.len() > 1 case. But lmk if you disagree.

@alamb

Copy link
Copy Markdown
Contributor

Looks good to me -- thanks @neilconway and @Jefffrey

@alamb
alamb added this pull request to the merge queueFeb 24, 2026
Merged via the queue into apache:main with commit 9c85ac6Feb 24, 2026
35 checks passed
@neilconway
neilconway deleted the neilc/optimize-to-array-of-size branch February 25, 2026 01:19
de-bgunter pushed a commit to de-bgunter/datafusion that referenced this pull request Mar 24, 2026
## Which issue does this PR close?
- Closesapache#20458.
- Closesapache#18159.
## Rationale for this change
When `array_to_size(n)` was called on a `List`-like object containing a
`StringViewArray` with `b` data buffers, the previous implementation
returned a list containing a `StringViewArray` with `n*b` buffers, which
results in catastrophically bad performance if `b` grows even somewhat
large.
This issue was previously noticed causing poor nested loop join
performance. apache#18161 adjusted the NLJ code to avoid calling
`to_array_of_size` for this reason, but didn't attempt to fix the
underlying issue in `to_array_of_size`. This PR doesn't attempt to
revert the change to the NLJ code: the special-case code added in apache#18161
is still slightly faster than `to_array_of_size` after this
optimization. It might be possible to address that in a future PR.
## What changes are included in this PR?
* Instead of using `repeat_n` + `concat` to merge together `n` copies of
the `StringViewArray`, we instead use `take`, which preserves the same
number of buffers as the input `StringViewArray`.
* Add a new benchmark for this situation
* Add more unit tests for `to_array_of_size`
## Are these changes tested?
Yes and benchmarked.
## Are there any user-facing changes?
No.
## AI usage
Iterated on the problem with Claude Code; I understand the problem and
the solution.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commonRelated to common cratephysical-planChanges to the physical-plan crate

Projects

None yet

4 participants

@neilconway@comphead@alamb@Jefffrey