Skip to content

Re-enable JoinPushTransitivePredicates optimization rule by default#18720

Merged
yashmayya merged 1 commit into
apache:masterfrom
yashmayya:reenable-join-push-transitive-predicates
Jun 10, 2026
Merged

Re-enable JoinPushTransitivePredicates optimization rule by default#18720
yashmayya merged 1 commit into
apache:masterfrom
yashmayya:reenable-join-push-transitive-predicates

Conversation

@yashmayya

Copy link
Copy Markdown
Contributor

Summary

Re-enables the JoinPushTransitivePredicates planner rule by default, reverting the default-disable introduced in #17181.

Background

#17181 disabled this rule by default because JoinPushTransitivePredicatesRule could enter an expensive, effectively looping predicate-inference path during planning (RelMdPredicates#getPredicatesRelOptPredicateList.unionpredicateConstantsgatherConstraints). In certain environments and query workloads this drove broker planning CPU from ~5% to ~70% alongside query timeouts. It was disabled as a default while remaining opt-in via the usePlannerRules query option.

What changed

Calcite was upgraded to 1.42.0 in #18658, which includes the fix for CALCITE-7302 ("Infinite loop with JoinPushTransitivePredicatesRule", apache/calcite#4649). That loop was caused by predicate-inference dedup failing once a conjunction was simplified into a SEARCH/Sarg form, so the rule kept re-inferring the same predicates. With this fixed, the rule is safe to enable by default again.

For reference, the earlier related fix CALCITE-6432 / apache/calcite#4346 (another JoinPushTransitivePredicatesRule infinite-loop fix) shipped back in Calcite 1.40.0; CALCITE-7302 is the follow-up that 1.42.0 brings in.

Behavior / blast radius

  • The rule now fires by default for multi-stage join / semi-join queries that carry a predicate on a join key, pushing the transitively-derived predicate to both sides of the join (e.g. for a.col1 = b.col1 AND a.col1 = 1, the rule also adds b.col1 = 1).
  • The Pinot carve-out that does not push predicates to the right side under a lookup-join hint is preserved (covered by QueryCompilationTest#testJoinPushTransitivePredicateLookupJoin).
  • The rule remains opt-out: per-query via skipPlannerRules='JoinPushTransitivePredicates', or cluster-wide via the pinot.broker.mse.planner.disabled.rules broker config.

Tests

  • Updated the affected plan-assertion resources (ExplainPhysicalPlans.json, JoinPlans.json, PhysicalOptimizerPlans.json) to reflect the rule firing, and removed the now-unnecessary usePlannerRules opt-in from QueryCompilationTest#testJoinPushTransitivePredicate.
  • Full pinot-query-planner suite passes (1263 tests, 0 failures).

Reverts the default-disable from apache#17181. That rule was disabled because
JoinPushTransitivePredicatesRule could spin in an expensive/looping
predicate-inference path (RelMdPredicates#getPredicates). Calcite 1.42.0
(apache#18658) fixes this via CALCITE-7302 (calcite#4649), so the rule is safe to
enable by default again. The lookup-join carve-out (no right-side pushdown)
is preserved and covered by testJoinPushTransitivePredicateLookupJoin.
@codecov-commenter

codecov-commenter commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.57%. Comparing base (474efb1) to head (5300657).
⚠️ Report is 15 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18720      +/-   ##
============================================
+ Coverage     64.48%   64.57%   +0.08%     
  Complexity     1291     1291              
============================================
  Files          3372     3372              
  Lines        208639   208666      +27     
  Branches      32590    32606      +16     
============================================
+ Hits         134550   134737     +187     
+ Misses        63287    63102     -185     
- Partials      10802    10827      +25     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 64.57% <ø> (+0.08%) ⬆️
temurin 64.57% <ø> (+0.08%) ⬆️
unittests 64.56% <ø> (+0.08%) ⬆️
unittests1 57.00% <ø> (+0.12%) ⬆️
unittests2 37.13% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yashmayya yashmayya added the multi-stage Related to the multi-stage query engine label Jun 9, 2026
@yashmayya yashmayya requested review from Jackie-Jiang and gortiz June 9, 2026 20:11

@gortiz gortiz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I miss is a regression test that verifies we don't fall again into #17181, but I guess we can assume Calcite already added that test

@yashmayya

Copy link
Copy Markdown
Contributor Author

@yashmayya yashmayya merged commit 901bdf9 into apache:master Jun 10, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

multi-stage Related to the multi-stage query engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants