Uh oh!
There was an error while loading. Please reload this page.
Refactor InListExpr into static-filter modules - #21649
Merged
Merged
Conversation
adriangbforce-pushed
the
perf/in_list_static_filter
branch
from
April 16, 2026 18:33
b3fb108 to
f03a2d1Comparealamb
commented
Apr 16, 2026
Contributor
run benchmark in_list |
adriangbot
commented
Apr 16, 2026
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/in_list_static_filter (f03a2d1) to 93ae1b8 (merge-base) diff File an issue against this benchmark runner |
adriangbot
commented
Apr 16, 2026
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
alamb
commented
Apr 16, 2026
Contributor
WAT -- thsoe are some nice numbers. I wil try and find time to review this PR |
ContributorAuthor
That's unexpected, it was supposed to be a pure refactor PR. I suspect it's doing more than what it says, I'll review it more carefully. EDIT: I’ve rebased this PR since then. It’s now just the single refactor commit, so it should be reviewed as a mechanical/module-structure cleanup. |
geoffreyclaudeforce-pushed
the
perf/in_list_static_filter
branch
from
April 17, 2026 13:24
f03a2d1 to
b96f00dComparegeoffreyclaudeforce-pushed
the
perf/in_list_static_filter
branch
from
April 17, 2026 13:29
b96f00d to
3fa6673Comparegeoffreyclaude
commented
Apr 17, 2026
ContributorAuthor
run benchmark in_list |
geoffreyclaude
commented
Apr 17, 2026
ContributorAuthor
run benchmark in_list_strategy |
adriangbot
commented
Apr 17, 2026
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/in_list_static_filter (3fa6673) to 5a427cb (merge-base) diff File an issue against this benchmark runner |
adriangbot
commented
Apr 17, 2026
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/in_list_static_filter (3fa6673) to 5a427cb (merge-base) diff File an issue against this benchmark runner |
geoffreyclaudeforce-pushed
the
perf/in_list_static_filter
branch
from
April 17, 2026 13:38
3fa6673 to
fc15088Compareadriangbot
commented
Apr 17, 2026
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Uh oh!
There was an error while loading. Please reload this page.
geoffreyclaudeforce-pushed
the
perf/in_list_static_filter
branch
from
April 17, 2026 13:47
fc15088 to
539f998Compareadriangbot
commented
Apr 17, 2026
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
geoffreyclaudeforce-pushed
the
perf/in_list_static_filter
branch
from
April 22, 2026 13:37
539f998 to
a904b9dCompareMoves the existing primitive and generic static-filter implementations into dedicated modules without changing the underlying algorithms. Keeps ArrayStaticFilter as the generic fallback and preserves the pre-existing result construction path.
geoffreyclaudeforce-pushed
the
perf/in_list_static_filter
branch
from
April 22, 2026 13:52
a904b9d to
f6bfa39Comparegeoffreyclaude
commented
Apr 22, 2026
ContributorAuthor
run benchmark in_list_strategy |
adriangbot
commented
Apr 22, 2026
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/in_list_static_filter (f6bfa39) to 5a427cb (merge-base) diff File an issue against this benchmark runner |
adriangbot
commented
Apr 22, 2026
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
geoffreyclaude
commented
Apr 22, 2026
ContributorAuthor
run benchmark in_list |
adriangbot
commented
Apr 22, 2026
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/in_list_static_filter (f6bfa39) to 5a427cb (merge-base) diff File an issue against this benchmark runner |
adriangbot
commented
Apr 22, 2026
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)DetailsResource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
geoffreyclaude
commented
Apr 23, 2026
ContributorAuthor
alamb
approved these changes
Apr 27, 2026
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thanks @geoffreyclaude -- this looks like a great change to me and I reviewed that it just splits the code into modules. Since it isn't a behavior change I'll plan to merge it once CI passes
| }; | ||
| } | ||
| // Generate specialized filters for all integer primitive types |
Contributor
There was a problem hiding this comment.
I know this PR didn't do it, but it is quite unfortunate that the float static filter code is the same as the primitive static filter (try searching for truth table -- the code is copy/paste)
ContributorAuthor
There was a problem hiding this comment.
Agreed. I kept this PR as close to move/extract-only as possible, but I opened #21932 to address this separately.
Uh oh!
There was an error while loading. Please reload this page.
geoffreyclaude
commented
Apr 27, 2026
ContributorAuthor
Thanks! Now we can start the real perf optims 😀 |
This was referenced Apr 29, 2026
philippemnoel pushed a commit
to paradedb/datafusion
that referenced
this pull request
Apr 29, 2026
## Which issue does this PR close? - Part of apache#19241. - Addresses the duplication called out in apache#21649 (comment). ## Rationale for this change The existing primitive `IN LIST` static filters have separate integer and float macros. The float version exists only to wrap values in `OrderedFloat*` for hash/equality semantics, but the rest of the implementation is copied from the integer path, including the SQL three-valued-logic result construction. This PR removes that duplication independently from the larger and more controversial direct-probe optimization in apache#19390. ## What changes are included in this PR? - Generalizes the existing `primitive_static_filter!` macro so it can optionally use a different stored `HashSet` value type and conversion function. - Keeps the original two-argument `primitive_static_filter!(..., ...)` form for integer filters. - Keeps the original `float_static_filter!(..., ..., OrderedFloat*)` call sites, but makes `float_static_filter!` delegate to `primitive_static_filter!` instead of duplicating the full implementation. - Preserves the existing `HashSet` lookup strategy and `OrderedFloat*` bit-pattern handling for `Float32` / `Float64`. ## Are these changes tested? Yes. ## Are there any user-facing changes? No. This is an internal refactor only.
saadtajwar pushed a commit
to saadtajwar/datafusion
that referenced
this pull request
Jun 22, 2026
## Which issue does this PR close? - Part of apache#19241. - This PR was originally proposed as the first commit in the broader `IN LIST` optimization series in apache#19390. - This PR builds on the refactor extracted in apache#21649. ## Rationale for this change After apache#21649, non-primitive constant `IN LIST` evaluation still uses the extracted `ArrayStaticFilter` fallback path. That path relies on comparator checks for each input row. This PR replaces that fallback lookup with a precomputed hash table and shared result construction so generic constant-list evaluation is cheaper before the later specialized primitive and string optimizations from apache#19390. ## What changes are included in this PR? The PR is split so reviewers can separate mechanical cleanup from the behavior/performance changes: 1. `Refactor generic InList static filter helpers` Pure refactoring. This moves the existing generic static-filter construction and probe loop into helper methods inside `ArrayStaticFilter`, without changing the lookup data structure or result semantics. 2. `Build InList results from bitmaps` Changes how the generic path materializes `BooleanArray` results after membership has been computed. Instead of mixing membership checks and SQL three-valued null handling in the row loop, this builds a contains bitmap first and applies the null/negation rules with bitmap operations. This keeps the same `IN` / `NOT IN` semantics, including the `NULL` cases. 3. `Optimize generic InList static filtering` Replaces the fallback lookup storage from a unit-valued raw-entry `HashMap` to `hashbrown::HashTable<usize>`. The table still stores indices into the constant list and still uses Arrow hashing plus `make_comparator` for equality, but avoids the extra map value bookkeeping. The existing specialized primitive filters and dictionary handling are intentionally left out of scope. ## Are these changes tested? Yes. ## Are there any user-facing changes? No. This is an internal performance optimization only. <!-- codex-benchmark-start --> ## Local benchmark snapshot Benchmark command: ```bash cargo bench -p datafusion-physical-expr --profile release-nonlto --bench in_list_strategy -- --save-baseline <name> ``` Method: compare adjacent saved baselines using raw Criterion sample minima (`min(time / iters)`). Lower is better; changes within +/-5% are treated as noise. Compared baselines: merge-base -> [apache#21927](apache#21927) Relevant scope: generic fallback string/view/binary rows. Summary: 62 relevant rows, 61 faster, 0 slower, 1 within +/-5%. Largest relevant deltas: | Benchmark | Before | After | Change | |---|---:|---:|---:| | `utf8view/short_8b/list=64/match=0%` | 45.55 us | 21.85 us | -52.0% (2.08x faster) | | `utf8view/short_8b/list=256/match=0%` | 44.34 us | 21.71 us | -51.0% (2.04x faster) | | `utf8view/short_8b/list=16/match=0%` | 44.03 us | 21.60 us | -50.9% (2.04x faster) | | `utf8view/short_8b/list=4/match=0%` | 41.54 us | 20.52 us | -50.6% (2.02x faster) | | `utf8view/len_12b/list=16/match=0%` | 41.43 us | 20.55 us | -50.4% (2.02x faster) | | `utf8view/len_12b/list=64/match=0%` | 41.59 us | 21.00 us | -49.5% (1.98x faster) | | `fixed_size_binary/fsb16/list=10000/match=0%` | 58.11 us | 29.36 us | -49.5% (1.98x faster) | | `fixed_size_binary/fsb16/list=256/match=0%` | 55.49 us | 28.57 us | -48.5% (1.94x faster) | | `utf8view/shared_prefix/pfx=8/list=16/match=0%` | 57.54 us | 32.07 us | -44.3% (1.79x faster) | | `utf8view/mixed_len/list=16/match=0%` | 62.86 us | 35.25 us | -43.9% (1.78x faster) | | `fixed_size_binary/fsb16/list=4/match=0%` | 47.62 us | 27.20 us | -42.9% (1.75x faster) | | `fixed_size_binary/fsb16/list=64/match=0%` | 47.85 us | 27.45 us | -42.6% (1.74x faster) | | `utf8view/mixed_len/list=64/match=0%` | 66.09 us | 38.00 us | -42.5% (1.74x faster) | | `utf8/short_8b/list=256/match=0%` | 52.09 us | 30.49 us | -41.5% (1.71x faster) | | `utf8view/shared_prefix/pfx=12/list=32/match=0%` | 70.61 us | 42.33 us | -40.1% (1.67x faster) | <details> <summary>Full relevant table (62 rows)</summary> | Benchmark | Before | After | Change | |---|---:|---:|---:| | `fixed_size_binary/fsb16/list=10000/match=0%` | 58.11 us | 29.36 us | -49.5% (1.98x faster) | | `fixed_size_binary/fsb16/list=10000/match=50%` | 98.77 us | 81.20 us | -17.8% (1.22x faster) | | `fixed_size_binary/fsb16/list=256/match=0%` | 55.49 us | 28.57 us | -48.5% (1.94x faster) | | `fixed_size_binary/fsb16/list=256/match=50%` | 96.40 us | 79.32 us | -17.7% (1.22x faster) | | `fixed_size_binary/fsb16/list=4/match=0%` | 47.62 us | 27.20 us | -42.9% (1.75x faster) | | `fixed_size_binary/fsb16/list=4/match=50%` | 93.08 us | 75.58 us | -18.8% (1.23x faster) | | `fixed_size_binary/fsb16/list=64/match=0%` | 47.85 us | 27.45 us | -42.6% (1.74x faster) | | `fixed_size_binary/fsb16/list=64/match=50%` | 95.20 us | 74.96 us | -21.3% (1.27x faster) | | `nulls/utf8/long_24b/list=16/match=50%/nulls=20%` | 85.74 us | 74.79 us | -12.8% (1.15x faster) | | `nulls/utf8/short_8b/list=16/match=50%/nulls=20%` | 80.01 us | 77.30 us | -3.4% (within +/-5%) | | `nulls/utf8view/long_24b/list=16/match=50%/nulls=20%` | 110.19 us | 96.52 us | -12.4% (1.14x faster) | | `nulls/utf8view/short_8b/list=16/match=50%/nulls=20%` | 74.78 us | 62.92 us | -15.9% (1.19x faster) | | `nulls/utf8view/short_8b/list=16/match=50%/nulls=20%/NOT_IN` | 71.24 us | 63.51 us | -10.9% (1.12x faster) | | `nulls/utf8view/short_8b/list=16/match=50%/nulls=50%` | 83.84 us | 62.11 us | -25.9% (1.35x faster) | | `utf8/long_24b/list=256/match=0%` | 58.79 us | 37.57 us | -36.1% (1.56x faster) | | `utf8/long_24b/list=256/match=50%` | 107.85 us | 74.62 us | -30.8% (1.45x faster) | | `utf8/long_24b/list=4/match=0%` | 56.68 us | 37.64 us | -33.6% (1.51x faster) | | `utf8/long_24b/list=4/match=50%` | 100.40 us | 79.11 us | -21.2% (1.27x faster) | | `utf8/long_24b/list=64/match=0%` | 59.39 us | 35.95 us | -39.5% (1.65x faster) | | `utf8/long_24b/list=64/match=50%` | 101.26 us | 79.59 us | -21.4% (1.27x faster) | | `utf8/mixed_len/list=16/match=0%` | 60.51 us | 49.06 us | -18.9% (1.23x faster) | | `utf8/mixed_len/list=16/match=50%` | 154.00 us | 139.13 us | -9.7% (1.11x faster) | | `utf8/mixed_len/list=64/match=0%` | 63.46 us | 49.87 us | -21.4% (1.27x faster) | | `utf8/mixed_len/list=64/match=50%` | 154.01 us | 134.01 us | -13.0% (1.15x faster) | | `utf8/shared_prefix/pfx=12/list=32/match=50%` | 98.73 us | 76.64 us | -22.4% (1.29x faster) | | `utf8/short_8b/list=16/match=50%/NOT_IN` | 96.18 us | 72.15 us | -25.0% (1.33x faster) | | `utf8/short_8b/list=256/match=0%` | 52.09 us | 30.49 us | -41.5% (1.71x faster) | | `utf8/short_8b/list=256/match=50%` | 94.56 us | 74.39 us | -21.3% (1.27x faster) | | `utf8/short_8b/list=4/match=0%` | 51.95 us | 32.27 us | -37.9% (1.61x faster) | | `utf8/short_8b/list=4/match=50%` | 95.05 us | 78.47 us | -17.4% (1.21x faster) | | `utf8/short_8b/list=64/match=0%` | 53.60 us | 33.34 us | -37.8% (1.61x faster) | | `utf8/short_8b/list=64/match=50%` | 96.35 us | 80.95 us | -16.0% (1.19x faster) | | `utf8view/len_12b/list=16/match=0%` | 41.43 us | 20.55 us | -50.4% (2.02x faster) | | `utf8view/len_12b/list=16/match=50%` | 73.07 us | 50.49 us | -30.9% (1.45x faster) | | `utf8view/len_12b/list=64/match=0%` | 41.59 us | 21.00 us | -49.5% (1.98x faster) | | `utf8view/len_12b/list=64/match=50%` | 75.23 us | 50.25 us | -33.2% (1.50x faster) | | `utf8view/long_24b/list=16/match=0%` | 58.48 us | 38.22 us | -34.7% (1.53x faster) | | `utf8view/long_24b/list=16/match=50%` | 109.63 us | 87.32 us | -20.4% (1.26x faster) | | `utf8view/long_24b/list=256/match=0%` | 61.12 us | 38.40 us | -37.2% (1.59x faster) | | `utf8view/long_24b/list=256/match=50%` | 113.25 us | 91.61 us | -19.1% (1.24x faster) | | `utf8view/long_24b/list=4/match=0%` | 58.43 us | 39.48 us | -32.4% (1.48x faster) | | `utf8view/long_24b/list=4/match=50%` | 112.73 us | 90.14 us | -20.0% (1.25x faster) | | `utf8view/long_24b/list=64/match=0%` | 62.17 us | 38.48 us | -38.1% (1.62x faster) | | `utf8view/long_24b/list=64/match=50%` | 109.35 us | 87.64 us | -19.8% (1.25x faster) | | `utf8view/mixed_len/list=16/match=0%` | 62.86 us | 35.25 us | -43.9% (1.78x faster) | | `utf8view/mixed_len/list=16/match=50%` | 126.60 us | 103.97 us | -17.9% (1.22x faster) | | `utf8view/mixed_len/list=64/match=0%` | 66.09 us | 38.00 us | -42.5% (1.74x faster) | | `utf8view/mixed_len/list=64/match=50%` | 137.76 us | 112.23 us | -18.5% (1.23x faster) | | `utf8view/shared_prefix/pfx=12/list=32/match=0%` | 70.61 us | 42.33 us | -40.1% (1.67x faster) | | `utf8view/shared_prefix/pfx=12/list=32/match=50%` | 115.15 us | 94.27 us | -18.1% (1.22x faster) | | `utf8view/shared_prefix/pfx=16/list=64/match=0%` | 63.47 us | 40.67 us | -35.9% (1.56x faster) | | `utf8view/shared_prefix/pfx=16/list=64/match=50%` | 112.27 us | 91.32 us | -18.7% (1.23x faster) | | `utf8view/shared_prefix/pfx=8/list=16/match=0%` | 57.54 us | 32.07 us | -44.3% (1.79x faster) | | `utf8view/shared_prefix/pfx=8/list=16/match=50%` | 100.47 us | 82.69 us | -17.7% (1.21x faster) | | `utf8view/short_8b/list=16/match=0%` | 44.03 us | 21.60 us | -50.9% (2.04x faster) | | `utf8view/short_8b/list=16/match=50%` | 72.92 us | 49.10 us | -32.7% (1.49x faster) | | `utf8view/short_8b/list=256/match=0%` | 44.34 us | 21.71 us | -51.0% (2.04x faster) | | `utf8view/short_8b/list=256/match=50%` | 72.43 us | 51.58 us | -28.8% (1.40x faster) | | `utf8view/short_8b/list=4/match=0%` | 41.54 us | 20.52 us | -50.6% (2.02x faster) | | `utf8view/short_8b/list=4/match=50%` | 72.50 us | 48.46 us | -33.2% (1.50x faster) | | `utf8view/short_8b/list=64/match=0%` | 45.55 us | 21.85 us | -52.0% (2.08x faster) | | `utf8view/short_8b/list=64/match=50%` | 73.14 us | 50.92 us | -30.4% (1.44x faster) | </details> <!-- codex-benchmark-end -->
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
This PR is a preparatory refactor for #19390.
InListExprcurrently mixes expression evaluation, constant-list filter construction, primitive-vs-fallback dispatch, and generic fallback execution in one file.This PR extracts the existing static-filter code into smaller internal modules while preserving the current behavior.
Review guide
This PR is intended to be a move/extract-only refactor. The main review question is whether the existing
InListExprstatic-filter behavior has been preserved while making the internal structure easier to follow.In particular:
in_list.rsstill ownsInListExprconstruction and evaluationprimitive_filter.rscontains the existing primitive numeric static-filter implementations moved out ofin_list.rsarray_static_filter.rscontains the existingArrayStaticFilterfallback moved out ofin_list.rsstrategy.rscontains the existing dispatch between primitive filters andArrayStaticFilterstatic_filter.rsdefines the shared internal trait used by those extracted piecesNo user-facing behavior or API changes are intended in this PR.
Are these changes tested?
Yes. I validated this PR with:
cargo fmt --allcargo clippy -p datafusion-physical-expr --all-targets --all-features -- -D warningscargo test -p datafusion-physical-expr --all-featuresAre there any user-facing changes?
No. This is an internal refactor only.