Repository navigation
perf: Optimize array_filter - #25754
Merged
Merged
Conversation
Measure a planned array_filter projection, including lambda evaluation and normalization of NULL list rows, with setup outside the timed loop. Cover common element types, predicate selectivity and NULLs, list shapes, scalar and array no-ops, captured columns, slices, and LargeList. Include both empty NULL rows and NULL rows retaining hidden elements. Check results against an independent element selection before timing each case.
Use one generic implementation for List and LargeList, including scalar predicate handling. Read original row offsets relative to the visible values instead of rebuilding an intermediate offset buffer, and return the original input when all visible values are retained. Compute adjusted row boundaries only for consumers that need them. array_first and array_any_match request them once before iterating; array_filter no longer allocates this unused vector.
Normalize NULL predicate results once, then use the same keep bitmap to count retained elements per row and select output values. Copy the bitmap only when a NULL list row stores elements, and bypass the filter kernel when no elements are retained. Keep NULL elements selected by array_filter predicates. Preserve the caller's existing NULL-row normalization and lambda evaluation behavior. Add tests for slices, NULL elements and rows, field metadata, buffer reuse, and sliced output capacity. Check physical buffer sharing through array wrappers.
github-actions
Bot
added
sqllogictest
Contributor
Author
|
This could be refactored to share some code with |
Contributor
Author
|
FYI @saadtajwar @comphead |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25754 +/- ##
==========================================
+ Coverage 82.84% 82.88% +0.03%
==========================================
Files 1151 1153 +2
Lines 455961 457297 +1336
Branches 455961 457297 +1336
==========================================
+ Hits 377760 379030 +1270
- Misses 55245 55264 +19
- Partials 22956 23003 +47 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
run benchmark array_filter baseline:
ref: "f721b9567f" |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
Contributor
|
run benchmark array_filter baseline:
ref: "f721b9567f" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/refactor-array-filter (39d794d) to f721b95 diff Run configurationrun benchmark array_filter
baseline:
ref: "f721b9567f"Results will be posted here when complete File an issue against this benchmark runner |
kosiew
reviewed
Oct 15, 2026
Contributor
There was a problem hiding this comment.
Thanks for this optimization. The changes address the issue requirements by normalizing nullable predicates, using popcount to compute result offsets, and avoiding unnecessary offset and bitmap allocations. I also checked sliced inputs, NULL list rows with hidden elements, retained NULL values, and the existing fast paths. I found no blocking issues.
I have two optional suggestions for improving test and benchmark coverage. Neither needs to block merging.
LGTM!
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing neilc/refactor-array-filter (39d794d) to f721b95 diff Run configurationrun benchmark array_filter
baseline:
ref: "f721b9567f"CPU Details (lscpu)Details
Resource Usagearray_filter — base (merge-base)
array_filter — branch
File an issue against this benchmark runner |
kosiew
approved these changes
Oct 15, 2026
…ilc/refactor-array-filter
Counting each row's kept elements with `count_set_bits_offset` has a fixed cost per call, which made lists of very short rows slower than on main. `prepare_list_filter` now reads the keep bitmap's 64-bit words once while keeping a running count of set bits, so each valid row costs one mask and one popcount. Empty NULL rows are skipped, and NULL rows that store elements subtract their set bits from the count. Also add an `int32/narrow_2/null_elements25` benchmark case (2,048 rows of 2 elements), test `prepare_list_filter` with rows that span 64-bit words, and test the counter directly. Benchmarks vs main (M4 Max): - int32/narrow_2/null_elements25, 8.38 -> 7.04 us, -16% (was +15%) - int32/keep50/null_elements25, 6.26 -> 3.55 us, -43% - int32/large_1m/null_elements25, 3120 -> 886 us, -72% - int32/keep50/null_rows50/hidden_storage, 5.29 -> 3.94 us, -26%
Contributor
Author
|
Thanks for the review, @kosiew ! |
neilconway
enabled auto-merge
October 15, 2026 15:23
Contributor
Author
|
run benchmark array_filter baseline:
ref: "f721b9567f" |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/refactor-array-filter (021c8fd) to f721b95 diff Run configurationrun benchmark array_filter
baseline:
ref: "f721b9567f"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing neilc/refactor-array-filter (021c8fd) to f721b95 diff Run configurationrun benchmark array_filter
baseline:
ref: "f721b9567f"CPU Details (lscpu)Details
Resource Usagearray_filter — base (merge-base)
array_filter — branch
File an issue against this benchmark runner |
comphead
reviewed
Oct 15, 2026
comphead
left a comment
comphead
left a comment
Contributor
There was a problem hiding this comment.
I traced prepare_list_filter by reading and found no correctness problem. I checked the word-boundary handling in SetBitCounter, the lazy keep_mask copy and hidden_count across consecutive NULL rows. I did not run the tests or benches. The comments below are about test coverage and simplification.
array_compact.rscomputes its per-row kept counts withcount_set_bits_offseton every valid row. You measured that call as slow on short rows, soSetBitCounterwould likely help there too. Fine as a follow-up.- In
bench_case,selectedandresult_lengthsstart asVec::new()although both sizes are known (values.len()andlist.len()). Setup only.
neilconway
disabled auto-merge
October 15, 2026 16:18
When a list has no NULL rows, every set bit in the keep bitmap is kept, so `prepare_list_filter` now builds each row's offset directly from the running count of set bits and returns the bitmap as is. Lists with NULL rows use the existing loop. Each branch creates its own `SetBitCounter`; sharing one made int32/keep50/null_rows50 7% slower on an M4 Max because of how the compiler assigned registers in the NULL-row loop. `OffsetBuffer::new_unchecked` stays: the checked constructor made int32/narrow_2/null_elements25 8% slower. `adjusted_row_offsets` now copies the offsets of a sliced list once instead of twice. Also test `prepare_list_filter` against a reference implementation on seeded random lists (i32 and i64 offsets, NULL rows that store elements, sliced lists, random keep bitmaps), add an array_filter test where only values stored under a NULL row pass the predicate, and size the benchmark's reference vectors up front. Benchmarks vs 021c8fd (M4 Max): - int32/narrow_2/null_elements25, 6.88 -> 4.11 us, -40% - int32/keep_all, 1.60 -> 1.22 us, -24% - int32/keep10/no_nulls, 1.90 -> 1.52 us, -20% - int32/keep50/null_elements25, 3.44 -> 3.06 us, -11% - int32/large_1m/null_elements25, 865 -> 781 us, -10% - cases with NULL rows, -2.5% to -0.2%
neilconway
enabled auto-merge
October 16, 2026 13:24
Contributor
Author
|
@comphead Thanks for the review! I fixed the bench capacity nit you mentioned. Good point about possibly using |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
After applying the predicate to the input list,
array_filterhas to compute two things: the row offsets for the result list, and the "keep" bitmap to pass to Arrow'sfilterkernel to copy the result values.When the predicate returned one or more NULLs, the previous implementation computed row offsets with a bit-by-bit loop over the result of applying the predicate, which is relatively slow. We can do better by first converting the predicate result bitmap into the "keep" bitmap (normalizing NULLs to cleared bits), and then using the keep bitmap to compute row offsets. This last step can be done in a single pass over the keep bitmap, using popcount on 64-bit words rather than a bit-by-bit loop.
Along the way, avoid creating two transient copies of the input row offset buffer. The previous code built a vector of adjusted row offsets to account for slicing of the input list; we can instead account for slicing without an additional data structure but instead by subtracting the first offset value from each row offset. This matches how sliced inputs are typically handled elsewhere.
Benchmarks: (M4 Max)
What changes are included in this PR?
ListandLargeListinto a single implementation, parameterized on offset typearray_filterWhat is the testing strategy for this PR?
Existing tests pass, test coverage expanded.
Are there any user-facing changes?
No.