planner: Adjust cost for semijoin and apply | tidb-test=pr/2703 - #66786
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/M
|
Hi @terry1purcell. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralized ordered-join outer-row estimation and added a probe-cost penalty for semi/anti-semi index-join variants; updated TPCH test expectations and added an Apply ordering selectivity test and related stats/DeriveStats adjustments. Changes
Sequence Diagram(s)(Skipped — changes are localized planner/cost refactors and test updates that do not introduce a new multi-component runtime flow requiring a sequence diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
|
/ok-to-test |
![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 (1)
pkg/planner/core/plan_cost_ver2.go (1)
808-810: Preferp.JoinType.IsSemiJoin()here.
pkg/planner/core/base/plan_base.goalready centralizes the four semi/anti-semi variants. Reusing it avoids duplicating join-type logic in the cost model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/plan_cost_ver2.go` around lines 808 - 810, Replace the explicit join-type checks for semi/anti-semi joins in the IndexHashJoin branch by using the centralized helper: call p.JoinType.IsSemiJoin() instead of comparing against base.SemiJoin, base.AntiSemiJoin, base.LeftOuterSemiJoin, and base.AntiLeftOuterSemiJoin; update the condition in the IndexHashJoin check (around the indexJoinType == 1 && probeRowsOne > 1 block in plan_cost_ver2.go) to use p.JoinType.IsSemiJoin() so it leverages the logic defined in plan_base.go and removes duplicated join-type logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/plan_cost_ver2.go`:
- Around line 803-813: The semi/anti-semi branch is applying probeRowsOne to an
already batch-discounted probeCost (variable probeCost), leaving semi-joins
under-costed; fix by computing the semi-join path from the non-discounted base:
for indexJoinType == 1 and p.JoinType in {base.SemiJoin, base.AntiSemiJoin,
base.LeftOuterSemiJoin, base.AntiLeftOuterSemiJoin} restore the original
(non-batched) probe cost by multiplying the current probeCost by batchRatio (the
variable used earlier) to undo the division, then apply costusage.MulCostVer2
with probeRowsOne; i.e., set probeCost =
costusage.MulCostVer2(costusage.MulCostVer2(probeCost, batchRatio),
probeRowsOne) so the penalty uses the undiscouned base.
---
Nitpick comments:
In `@pkg/planner/core/plan_cost_ver2.go`:
- Around line 808-810: Replace the explicit join-type checks for semi/anti-semi
joins in the IndexHashJoin branch by using the centralized helper: call
p.JoinType.IsSemiJoin() instead of comparing against base.SemiJoin,
base.AntiSemiJoin, base.LeftOuterSemiJoin, and base.AntiLeftOuterSemiJoin;
update the condition in the IndexHashJoin check (around the indexJoinType == 1
&& probeRowsOne > 1 block in plan_cost_ver2.go) to use p.JoinType.IsSemiJoin()
so it leverages the logic defined in plan_base.go and removes duplicated
join-type logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1d5d951-0f15-4523-b456-968e4c0e7352
📒 Files selected for processing (3)
pkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.jsonpkg/planner/core/plan_cost_ver2.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66786 +/- ##
================================================
- Coverage 77.7308% 77.4895% -0.2414%
================================================
Files 2012 1937 -75
Lines 551135 545803 -5332
================================================
- Hits 428402 422940 -5462
- Misses 121017 122832 +1815
+ Partials 1716 31 -1685
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integrationtest/r/tpch.result`:
- Line 295: The test expectations were changed while the planner cost change
wasn't applied: the semi/anti-semi over-read penalty is only coded for
indexJoinType == IndexHashJoin in function(s) handling semi/anti joins (refer to
plan_cost_ver2.go and the indexJoinType/IndexHashJoin symbols); update the cost
logic so the over-read penalty also applies to the other IndexJoin variant (or
more generally to any index-based semijoin) by broadening the conditional that
currently checks indexJoinType == IndexHashJoin (or by switching to a check
based on join strategy rather than just the IndexHashJoin constant), then re-run
the planner tests and re-record the golden outputs instead of changing
expectations first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d52641d-39c0-4290-8cdd-df5bab930468
📒 Files selected for processing (1)
tests/integrationtest/r/tpch.result
Deduplicate the OptOrderingIdxSelRatio-aware ExpectedCnt computation that was inlined in constructIndexJoinStatic into a shared helper. This avoids duplicating the same logic when other join operators (e.g. Apply) need the same calculation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ti-chi-bot
Bot
added
size/L
terry1purcell
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 258-267: calcOuterExpectedCnt currently adds the rowsToMeetFirst
ordering penalty whenever OptOrderingIdxSelRatio > 0, which incorrectly
penalizes unordered index join candidates; change calcOuterExpectedCnt to only
include the rowsToMeetFirst term when the property indicates an ordered output
(e.g., when prop.SortItems is non-empty) so unordered joins keep their original
ExpectedCnt; update the conditional around rowsToMeetFirst to check
len(prop.SortItems) > 0 (or equivalent) before computing/adding rowsToMeetFirst,
and apply the same guarded change to the analogous logic referenced elsewhere in
this diff (the other calc/return path for static IndexJoin/IndexHashJoin).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 72521dd8-e86a-4b93-9a1c-657d3e3815f0
📒 Files selected for processing (1)
pkg/planner/core/exhaust_physical_plans.go
terry1purcell
changed the title
|
/retest-required |
terry1purcell
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/cardinality/selectivity_test.go (1)
1917-1924: Minor style observation (optional).The
hasApplyhelper is appropriately scoped. Consider extracting the cost-parsing pattern (lines 1930-1931, 1936-1937, etc.) into a shared helper if more similar tests are added in the future, though this matches the existingTestOrderingIdxSelectivityRatioForJoinstyle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/cardinality/selectivity_test.go` around lines 1917 - 1924, The cost-parsing logic repeated in the test (the pattern that parses cost values from rows) should be extracted into a small helper (e.g., parsePlanCostFromRow or extractCostFromRow) and used by this test and any similar ones like TestOrderingIdxSelectivityRatioForJoin; implement a helper that accepts a row ([]interface{}) or the plan string, returns the parsed float64 cost (with error handling), replace the duplicated parsing snippets in the test and in other tests with calls to that helper, and keep the existing hasApply closure as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/cardinality/selectivity_test.go`:
- Around line 1917-1924: The cost-parsing logic repeated in the test (the
pattern that parses cost values from rows) should be extracted into a small
helper (e.g., parsePlanCostFromRow or extractCostFromRow) and used by this test
and any similar ones like TestOrderingIdxSelectivityRatioForJoin; implement a
helper that accepts a row ([]interface{}) or the plan string, returns the parsed
float64 cost (with error handling), replace the duplicated parsing snippets in
the test and in other tests with calls to that helper, and keep the existing
hasApply closure as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e55c080c-230f-47b2-8985-3390dcc9328b
📒 Files selected for processing (3)
pkg/planner/cardinality/BUILD.bazelpkg/planner/cardinality/selectivity_test.gopkg/planner/core/operator/logicalop/logical_apply.go
terry1purcell
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, qw4990 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
ti-chi-bot
Bot
added
size/L
|
/hold Other review comments - that aren't critical to merge. But are worthy of resolution. |
ti-chi-bot
Bot
added
the
do-not-merge/hold
ti-chi-bot
Bot
added
size/XL
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/retest-required |
4 participants
What problem does this PR solve?
Issue Number: ref #65556
Problem Summary:
What changed and how does it work?
Changes
This PR improves planner cost modeling for semi-join operators (EXISTS/NOT EXISTS subqueries) across IndexJoin and Apply plans, particularly when ordering is involved.
Several testcases were regenerated to reflect the default "SelectionFactor" usage for apply.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Refactor
Tests