Skip to content

Conversation

@lyne7-sc
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

The current implementation of general_remove is based on filter + concat, which creates intermediate arrays for each list row and can be relatively expensive.

This PR introduces an alternative implementation based on MutableArrayData, which copies contiguous ranges from the original values buffer directly into the output array. The new approach is semantically equivalent to the existing implementation but reduces intermediate allocations and per-element overhead.

What changes are included in this PR?

  • Replaced general_remove's filter-based implementation with MutableArrayData for more efficient memory usage.
  • Optimized the removal process by adding fast paths for rows where no matching elements need removal.

Benchmark

group                                        after                                  before
-----                                        -----                                  ------
array_remove_binary/remove/10                1.00      4.6±0.14ms        ? ?/sec    2.41     11.0±0.34ms        ? ?/sec
array_remove_binary/remove/100               1.00      8.5±0.19ms        ? ?/sec    1.95     16.6±0.42ms        ? ?/sec
array_remove_binary/remove/500               1.00     35.9±0.78ms        ? ?/sec    1.43     51.4±1.10ms        ? ?/sec
array_remove_boolean/remove/10               1.00      3.7±0.05ms        ? ?/sec    3.23     11.8±0.30ms        ? ?/sec
array_remove_boolean/remove/100              1.00      8.1±0.15ms        ? ?/sec    2.18     17.6±0.35ms        ? ?/sec
array_remove_boolean/remove/500              1.00     26.6±0.43ms        ? ?/sec    1.52     40.3±0.81ms        ? ?/sec
array_remove_decimal64/remove/10             1.00      3.9±0.07ms        ? ?/sec    2.41      9.4±0.18ms        ? ?/sec
array_remove_decimal64/remove/100            1.00      6.7±0.19ms        ? ?/sec    2.12     14.2±0.34ms        ? ?/sec
array_remove_decimal64/remove/500            1.00     40.3±0.75ms        ? ?/sec    1.52     61.1±1.46ms        ? ?/sec
array_remove_f64/remove/10                   1.00      3.8±0.10ms        ? ?/sec    1.32      5.0±0.16ms        ? ?/sec
array_remove_f64/remove/100                  1.00      4.8±0.34ms        ? ?/sec    1.24      5.9±0.18ms        ? ?/sec
array_remove_f64/remove/500                  1.00     22.3±0.68ms        ? ?/sec    1.15     25.5±0.86ms        ? ?/sec
array_remove_fixed_size_binary/remove/10     1.00      4.7±0.09ms        ? ?/sec    1.52      7.2±0.26ms        ? ?/sec
array_remove_fixed_size_binary/remove/100    1.00      8.0±0.32ms        ? ?/sec    1.40     11.1±0.40ms        ? ?/sec
array_remove_fixed_size_binary/remove/500    1.00     45.4±0.97ms        ? ?/sec    1.16     52.6±1.43ms        ? ?/sec
array_remove_int64/remove/10                 1.00      3.9±0.11ms        ? ?/sec    2.24      8.8±0.24ms        ? ?/sec
array_remove_int64/remove/100                1.00      5.5±0.18ms        ? ?/sec    2.32     12.8±0.44ms        ? ?/sec
array_remove_int64/remove/500                1.00     25.5±1.06ms        ? ?/sec    1.61     40.9±1.25ms        ? ?/sec
array_remove_strings/remove/10               1.00      4.5±0.10ms        ? ?/sec    2.41     10.9±0.28ms        ? ?/sec
array_remove_strings/remove/100              1.00      8.5±0.37ms        ? ?/sec    2.00     17.0±0.71ms        ? ?/sec
array_remove_strings/remove/500              1.00     35.9±0.84ms        ? ?/sec    1.48     53.1±1.91ms        ? ?/sec

Are these changes tested?

Yes. Existing SLT for array continue to pass without modification. Benchmarks were added.

Are there any user-facing changes?

No.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @lyne7-sc this is great PR, PTAL on minors


fn criterion_benchmark(c: &mut Criterion) {
// Test array_remove with different data types and array sizes
bench_array_remove_int64(c);
Copy link
Contributor

Choose a reason for hiding this comment

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

great, would be also nice to see performance for nested datatypes, but it can be done in the following PR, for now please mention a TODO here

@lyne7-sc
Copy link
Contributor Author

Thanks for the review.
Added a TODO, will validate performance for nested datatypes later on.

@comphead comphead added this pull request to the merge queue Jan 26, 2026
Merged via the queue into apache:main with commit 27abe5a Jan 26, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants