Add profitability guards for join filter pushdown - #22997
Merged
rapids-bot[bot] merged 10 commits intoJul 23, 2026
Merged
Conversation
pentschev
added
feature request
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
This was referenced Jun 26, 2026
wence-
force-pushed
the
cudf-polars/join-domain-prefilter-guards
branch
from
July 22, 2026 08:54
2ab4f10 to
c9d1cda
Compare
github-actions
Bot
added
libcudf
wence-
force-pushed
the
cudf-polars/join-domain-prefilter-guards
branch
from
July 22, 2026 08:55
c9d1cda to
3476846
Compare
wence-
changed the title
wence-
force-pushed
the
cudf-polars/join-domain-prefilter-guards
branch
from
July 22, 2026 10:58
6e7309a to
55a901b
Compare
wence-
added
improvement
wence-
force-pushed
the
cudf-polars/join-domain-prefilter-guards
branch
from
July 22, 2026 11:00
55a901b to
a8cc2e6
Compare
wence-
reviewed
Jul 22, 2026
| --import-mode=importlib \ | ||
| --cache-clear \ | ||
| -x \ | ||
| -v \ |
Contributor
There was a problem hiding this comment.
The verbose test log makes github actions unusable. With fail fast we get immediate information about a failing test.
wence-
marked this pull request as ready for review
July 22, 2026 11:01
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesJoin filter pushdown
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/cudf_polars/cudf_polars/streaming/join_filter_pushdown.py (1)
748-768: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency:
source_facts[node]uses direct indexing whilerow_estimatesuses.get().
rows = facts.row_estimates.get(node)defends against a missing key, butsource = facts.source_facts[node]on the next line would raiseKeyErrorinstead of returningNoneifnodewere ever absent fromsource_facts(both maps are populated together in the sameanalyze_plantraversal, so this is currently safe, but the asymmetry is a latent footgun for future refactors).🛡️ Defensive fix
- rows = facts.row_estimates.get(node) - source = facts.source_facts[node] - if rows is None or rows <= 0 or source.cost is None: + rows = facts.row_estimates.get(node) + source = facts.source_facts.get(node) + if rows is None or rows <= 0 or source is None or source.cost is None: return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/cudf_polars/streaming/join_filter_pushdown.py` around lines 748 - 768, Update make_producer to retrieve source_facts with a missing-key-safe lookup, and return None when either the row estimate or source facts are absent. Preserve the existing checks for nonpositive rows and missing source.cost, and only construct _Producer when all required facts are available.python/cudf_polars/tests/streaming/test_join_filter_pushdown.py (1)
341-454: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the cost-model behavior; consider adding an empty/zero-row domain edge case.
Both new tests correctly exercise cheaper-domain selection and stacking avoidance with GPU/CPU result parity checks. Neither this addition nor the surrounding file appears to exercise the case where a candidate domain's estimated rows/cost fall to zero or
None(e.g., an empty or fully-filtered source), which is exactly the scenariomake_producer'srows <= 0 or source.cost is Noneguard is meant to handle gracefully.As per path instructions,
python/**/test_*.pyshould "ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/tests/streaming/test_join_filter_pushdown.py` around lines 341 - 454, Add a focused empty or fully filtered source-domain test alongside test_source_only_domain_does_not_stack_on_prefiltered_source, exercising the make_producer path where estimated rows are zero or source.cost is None. Verify optimization completes without stacking an invalid domain or raising, and retain GPU/CPU result parity assertions for the empty-result case.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf_polars/cudf_polars/streaming/join_filter_pushdown.py`:
- Around line 669-672: The composite candidate path in _composite_candidates
lacks the semi-ancestor filtering guard used by _simple_candidates. Add the
equivalent single-source semi-ancestor checks for both domain and
constraint_domain before the domain cost checks, and add a test covering
composite candidates that verifies redundant semi-joins are not generated for an
already-filtered source.
---
Nitpick comments:
In `@python/cudf_polars/cudf_polars/streaming/join_filter_pushdown.py`:
- Around line 748-768: Update make_producer to retrieve source_facts with a
missing-key-safe lookup, and return None when either the row estimate or source
facts are absent. Preserve the existing checks for nonpositive rows and missing
source.cost, and only construct _Producer when all required facts are available.
In `@python/cudf_polars/tests/streaming/test_join_filter_pushdown.py`:
- Around line 341-454: Add a focused empty or fully filtered source-domain test
alongside test_source_only_domain_does_not_stack_on_prefiltered_source,
exercising the make_producer path where estimated rows are zero or source.cost
is None. Verify optimization completes without stacking an invalid domain or
raising, and retain GPU/CPU result parity assertions for the empty-result case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 39d6d829-5e86-4237-9e11-86c835d314d0
📒 Files selected for processing (5)
ci/run_cudf_polars_polars_tests.shci/test_python_other.shci/test_wheel_cudf_polars.shpython/cudf_polars/cudf_polars/streaming/join_filter_pushdown.pypython/cudf_polars/tests/streaming/test_join_filter_pushdown.py
💤 Files with no reviewable changes (1)
- ci/test_wheel_cudf_polars.sh
This was referenced Jul 22, 2026
gforsyth
approved these changes
Jul 22, 2026
Contributor
|
I see a GPU OOM for tpch query 21 at SF 1000 on an H100 with this branch: DetailsI'm investigating more. |
Contributor
Do you also see them for |
Teach the join filter pushdown planner to estimate the source-scan cost needed to build candidate domains. Candidate selection now prefers cheaper domain producers before smaller estimated output rows, and rejects pushdown when the domain-build cost is too large relative to the target being reduced. Also prevent a single-source domain from being stacked onto a target source that is already below the filtered side of a semi join. This heuristic does a better job of keeping profitable domain choices while avoiding cases where pushing down a filter would add an additional high-cardinality source onto a join target that is already significantly filtered. Additionally, extends trace metadata with estimated target, domain, and constraint costs.
We don't need to repeat the domain cost twice since it doesn't break any ties.
Also fail fast in all cudf-polars tests since a failing test can pollute engine state in an unrecoverable way.
wence-
force-pushed
the
cudf-polars/join-domain-prefilter-guards
branch
from
July 23, 2026 12:27
a8cc2e6 to
13b937d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cudf_polars/cudf_polars/dsl/traversal.py (1)
33-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused reference-counting tests.
Add direct coverage for shared children, duplicate roots, and repeated child entries; downstream optimizer tests do not fully pin down these DAG-counting semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf_polars/cudf_polars/dsl/traversal.py` around lines 33 - 49, Add focused tests for collect_refcount covering shared children referenced by multiple parents, duplicate root nodes, and repeated entries in a node’s children sequence. Assert the exact counts for each node so DAG reference-counting semantics are validated independently of downstream optimizer tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/cudf_polars/cudf_polars/dsl/traversal.py`:
- Around line 33-49: Add focused tests for collect_refcount covering shared
children referenced by multiple parents, duplicate root nodes, and repeated
entries in a node’s children sequence. Assert the exact counts for each node so
DAG reference-counting semantics are validated independently of downstream
optimizer tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca3f22b0-a328-4c8b-bf10-34d82c6cafcb
📒 Files selected for processing (6)
ci/run_cudf_polars_polars_tests.shci/test_python_other.shci/test_wheel_cudf_polars.shpython/cudf_polars/cudf_polars/dsl/traversal.pypython/cudf_polars/cudf_polars/streaming/join_filter_pushdown.pypython/cudf_polars/tests/streaming/test_join_filter_pushdown.py
💤 Files with no reviewable changes (1)
- ci/test_wheel_cudf_polars.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- ci/test_python_other.sh
- ci/run_cudf_polars_polars_tests.sh
- python/cudf_polars/tests/streaming/test_join_filter_pushdown.py
- python/cudf_polars/cudf_polars/streaming/join_filter_pushdown.py
TomAugspurger
approved these changes
Jul 23, 2026
Contributor
|
/merge |
rapids-bot
Bot
merged commit Jul 23, 2026
9b00d88
into
rapidsai:release/26.08
261 of 264 checks passed
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
6 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.
Teach the join filter pushdown planner to estimate the source-scan
cost needed to build candidate domains. Candidate selection now prefers
cheaper domain producers before smaller estimated output rows, and rejects
pushdown when the domain-build cost is too large relative to the target
being reduced.
Also prevent a single-source domain from being stacked onto a target source
that is already below the filtered side of a semi join. This heuristic does
a better job of keeping profitable domain choices while avoiding cases
where pushing down a filter would add an additional high-cardinality source
onto a join target that is already significantly filtered.
Additionally, extends trace metadata with estimated target, domain, and
constraint costs.
Material results of this change running NDSH SF30K on 8xNVL4 nodes are (previous results come from the change in #22996):