perf: deduplicate hash join InList pushdown values - #24236
Draft
adriangb wants to merge 2 commits into
Draft
Conversation
The hash join InList pushdown gates on the build side's *distinct* key count (`hash_join_inlist_pushdown_max_distinct_values`, default 150) but then ships the raw build-side key arrays, which have one entry per build row. A partition with 150 distinct keys and 16k build rows produced a 16k-entry `IN` list. Deduplicate the values (arrow row-format encoding + a hash set, so it works for multi-column struct keys and dictionaries alike) before wrapping them in `PushdownStrategy::InList`. An `IN` list is a set, so this cannot change any result; NULLs collapse to a single NULL, which is likewise indistinguishable under three-valued logic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a sqllogictest where the build side has 6 rows but only 2 distinct keys, asserting the pushed-down dynamic filter is `IN (SET) ([11, 22])` and the derived `required_guarantees` is `[id in (11, 22)]`. Before the previous commit both listed all 6 build rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
run benchmarks |
Contributor
Author
|
show benchmark queue |
|
Hi @adriangb, you asked to view the benchmark queue (#24236 (comment)).
File an issue against this benchmark runner |
github-actions
Bot
added
sqllogictest
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/dedup-inlist-pushdown (d56edfd) to 33ad1cc (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/dedup-inlist-pushdown (d56edfd) to 33ad1cc (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/dedup-inlist-pushdown (d56edfd) to 33ad1cc (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/dedup-inlist-pushdown (d56edfd) to 33ad1cc (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/dedup-inlist-pushdown (d56edfd) to 33ad1cc (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/dedup-inlist-pushdown (d56edfd) to 33ad1cc (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24236 +/- ##
==========================================
- Coverage 80.99% 80.98% -0.01%
==========================================
Files 1106 1106
Lines 383352 383458 +106
Branches 383352 383458 +106
==========================================
+ Hits 310488 310554 +66
- Misses 54544 54575 +31
- Partials 18320 18329 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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
The hash join InList filter pushdown is gated on the build side's distinct key count:
but the values it actually ships are the raw build-side key arrays — one entry per build row, never deduplicated. So a partition that passes the 150-distinct-value gate can still emit a list with thousands of entries, bounded only by
hash_join_inlist_pushdown_max_size(default 128 KiB).On TPC-H q17 at SF=1 (12 partitions) the pushed-down filter on the
lineitemscan looked like this before the change:6088 list entries in total across the 12 partitions, for 204 distinct keys.
That length is paid for repeatedly and mostly discarded:
InListExpr::try_new_from_arraymaterialises oneScalarValue+ oneLiteralexpression per entry, so thelistfield is ~30× larger than it needs to be.LiteralGuarantee, whichPruningPredicate::prunere-materialises per row group wherever the pushed-down filter reaches a pruning predicate.RowGroupPruningStatistics::contained()indatafusion/datasource-parquet/src/row_group_filter.rsreturnsNoneunconditionally, so that guarantee can never prune anything — the work is provably discarded.max_in_list_size(default 20) blocks the InList→OR rewrite, so an inflated list yields no pruning benefit on that path either.The duplicates carry no information:
INis a set membership test.What changes are included in this PR?
build_struct_inlist_valuesnow deduplicates the value array before it is wrapped inPushdownStrategy::InList.Deduplication is done on the arrow array itself: encode it once with
RowConverterand keep first occurrences via aHashSet<Row>, then a singletake. This is one pass and two allocations, rather than oneScalarValue::try_from_arrayper row, and it handles both shapes the pushdown produces — a single key column (possibly dictionary-encoded) and a multi-columnStructArray— without special-casing either.Semantics are preserved exactly, and the transform is a set-preserving no-op:
IN/NOT INresults are unchanged.takepreserves dictionary encoding, and the array is returned untouched when it has <2 rows, contains no duplicates, or has a typeRowConvertercannot encode.Are these changes tested?
Yes.
inlist_builder.rscover: single-column dedup, an already-distinct array (identity), NULL collapsing, multi-column tuple dedup, distinct tuples built from individually-duplicated columns, and dictionary arrays.push_down_filter_parquet.sltcovers it end to end: a build side with 6 rows and 2 distinct keys now pushesIN (SET) ([11, 22])withrequired_guarantees=[id in (11, 22)], wheremainpushed all 6 rows. It also asserts the join output is unchanged (still one row per matching build row).IN (SET)snapshots inpush_down_filter_parquet.sltandfilter_pushdown.rsare byte-identical — their build sides were already distinct, which is the expected no-op.cargo test -p datafusion-physical-plan --lib, the full sqllogictest suite,cargo clippy --all-targets -- -D warnings(CI feature set) andcargo docare all green.Measured effect
Counts first, since they are exact rather than timings. TPC-H SF=1, parquet, from
dfbench --debug, summing theIN (SET)list lengths across the 12 partitions of the q17 dynamic filter:Only q17 has a build side with duplicate join keys at SF=1; everywhere else the build keys are already distinct (group-by outputs,
nation/regionkeys) and the change is a literal no-op. That is the expected shape: this only helps when the build side repeats keys.Timings: both binaries built from the same worktree and target dir, differing only by this commit. 20 rounds × 6 iterations, A/B order counterbalanced per round (A,B on odd rounds; B,A on even), per-round median over iterations 1–5 (cold iteration dropped), then median across rounds. TPC-H SF=1 parquet on a machine that was not idle.
Honest reading of that table. q1 and q6 have no joins, so this change cannot touch them; whatever they show is the noise floor, and here that floor is up to ±2.4%. q17's −3.8% is only just clear of it. The reason I believe it is real rather than noise is the sign test, not the magnitude: 18 of 20 counterbalanced rounds favoured the change, both orderings agree in sign and size, and the controls sit at 6/20 and 10/20 as they should. q18's −2.3% is inside the floor and 12/20 is a coin flip — and its list length does not change at all, so that column is noise by construction, which is a useful internal check on the method.
Per-round spread is wide (individual q17 rounds ranged −44% to +22%), which is why only the 20-round medians and the sign test are quoted.
A first, lower-powered pass (12 rounds × 5 iterations over q17/q18/q3/q5/q9 + q1/q6) produced a ±9% control floor and showed nothing above it in either direction. It is reported here for completeness, not as evidence.
I would also not over-claim the pruning half of the mechanism from this benchmark: at SF=1 each table is a single file and the dynamic filter arrives after row-group pruning has already run (
row_groups_pruned_dynamic_filter=0,statistics_eval_time=24nson that scan), so on this dataset the discardedLiteralGuaranteework is not actually being paid. The saving I measured at SF=1 is the ~5900ScalarValue/Literalallocations that no longer happen. The guarantee argument should matter more on datasets with many row groups per scan, where the filter is live before pruning — I have not measured that here.If reviewers consider −3.8% on one query too thin to justify the change on performance grounds alone, the correctness-of-effort argument still stands on its own: the deduplicated list is semantically identical, strictly smaller, and the work it removes is work that could never have produced a result.
Are there any user-facing changes?
No API changes and no behaviour changes. The only visible difference is in
EXPLAIN ANALYZEoutput, where the pushed-downIN (SET) (...)list and therequired_guaranteesderived from it no longer repeat duplicate build-side keys.