Disable join filter pushdown rewrites by default - #23407
Conversation
This changes the cudf-polars default configuration to disable the join filter pushdown rewrite added in NVIDIA#22996. It can be re-enabled through an env var with CUDF_POLARS__EXECUTOR__JOIN_FILTER_PUSHDOWN=1, or through python by passing `join_filter_pushdown=JoinFilterPushdownOptions()` I'm proposing to disable this based on some slowdowns in our pds-h benchmarks at smaller scales (SF-1K on a single H100 GPU).
|
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. |
|
/ok to test 7da03a8 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe default for ChangesJoin filter pushdown configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cudf_polars/cudf_polars/utils/config.py (1)
1102-1113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign all public configuration entry points with the new opt-in default.
This fallback only changes
ConfigOptions.from_polars_engine. DirectStreamingExecutor()andConfigOptions()construction still receiveJoinFilterPushdownOptionsby default via Lines 827-829 and 1006-1008, so the rewrite remains enabled for those callers despite the documented “disabled unless explicitly enabled” contract. Make the constructor defaults consistently disabled, while preserving explicit Python and environment-variable opt-in.🤖 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/utils/config.py` around lines 1102 - 1113, Update the default construction paths in ConfigOptions and StreamingExecutor, including their JoinFilterPushdownOptions initialization, to disable join filter pushdown unless explicitly enabled. Preserve explicit Python configuration and the CUDF_POLARS__EXECUTOR__JOIN_FILTER_PUSHDOWN environment-variable opt-in, and keep ConfigOptions.from_polars_engine behavior consistent with these constructor defaults.python/cudf_polars/tests/test_config.py (1)
609-618: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the environment-based tests hermetic.
test_dynamic_planning_defaultsdoes not removeCUDF_POLARS__EXECUTOR__JOIN_FILTER_PUSHDOWN, so an ambient value of"1"can invalidate the default assertion.test_join_prefilter_options_from_envlikewise leaves the nested threshold and trace variables uncontrolled, allowing ambient values to change the expected0.5andFalseresults. Usemonkeypatch.delenv(..., raising=False)or set every relevant variable explicitly.Also applies to: 642-661
🤖 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/test_config.py` around lines 609 - 618, Make the environment-dependent tests hermetic by updating test_dynamic_planning_defaults and test_join_prefilter_options_from_env to remove or explicitly set every relevant CUDF_POLARS__EXECUTOR__ environment variable before constructing configuration, including JOIN_FILTER_PUSHDOWN, JOIN_PREFILTER_THRESHOLD, and JOIN_PREFILTER_TRACE, so assertions remain deterministic.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@python/cudf_polars/cudf_polars/utils/config.py`:
- Around line 1102-1113: Update the default construction paths in ConfigOptions
and StreamingExecutor, including their JoinFilterPushdownOptions initialization,
to disable join filter pushdown unless explicitly enabled. Preserve explicit
Python configuration and the CUDF_POLARS__EXECUTOR__JOIN_FILTER_PUSHDOWN
environment-variable opt-in, and keep ConfigOptions.from_polars_engine behavior
consistent with these constructor defaults.
In `@python/cudf_polars/tests/test_config.py`:
- Around line 609-618: Make the environment-dependent tests hermetic by updating
test_dynamic_planning_defaults and test_join_prefilter_options_from_env to
remove or explicitly set every relevant CUDF_POLARS__EXECUTOR__ environment
variable before constructing configuration, including JOIN_FILTER_PUSHDOWN,
JOIN_PREFILTER_THRESHOLD, and JOIN_PREFILTER_TRACE, so assertions remain
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e36a6fdf-7e0f-4855-a2e1-cee5d68ae6d0
📒 Files selected for processing (2)
python/cudf_polars/cudf_polars/utils/config.pypython/cudf_polars/tests/test_config.py
|
Just a note that #22997 fixes a bug, which ameliorates some of the performance issues. Now things are more or less what we'd expect: join filter pushdown might insert some extra semi-joins to do some pre-filtering. This can be great at large scales. It can be some unnecessary work at smaller scales that ultimately slows us down. We want to spend a bit more time investigating the tradeoffs and will hopefully enable this for the 26.10 release. |
|
/merge |
Description
This changes the cudf-polars default configuration to disable the join filter pushdown rewrite added in
#22996. It can be re-enabled through an env var with CUDF_POLARS__EXECUTOR__JOIN_FILTER_PUSHDOWN=1, or through python by passing
join_filter_pushdown=JoinFilterPushdownOptions()I'm proposing to disable this for the 26.08 release based on some slowdowns in our pds-h benchmarks at smaller scales (SF-1K on a single H100 GPU).
Eventually, I think that the join filter pushdown optimization should be on by default. But I'd like to understand a bit more about the slowdowns observed in that benchmark before turning it on by default.