pkg/planner: add order-aware logical join reorder rule#67305
Conversation
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughAdds an order-aware join-reorder optimization: new OrderAwareJoinReorder rule, join-order annotation (ordered-leading choice) and internal hint fields, new planner flag, build/test updates, and casetest suites validating ordering-preserving join reorders. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Optimizer as Optimizer/RuleRunner
participant Rule as OrderAwareJoinReorder
participant JoinOrder as joinorder.AnnotateOrderedLeading
participant PlannerNodes as LogicalPlan
Client->>Optimizer: request plan build (root)
Optimizer->>Rule: OrderAwareJoinReorder.Optimize(root)
Rule->>PlannerNodes: traverse nodes (TopN/Sort/Proj/Limit/Selection/Join)
alt ordering columns found
Rule->>JoinOrder: FindOrderedLeadingChoice(root/group, orderingCols)
JoinOrder-->>Rule: choice or nil
alt choice found
Rule->>PlannerNodes: annotate anchor join with internal leading hint
end
end
Rule-->>Optimizer: return (plan, changed)
Optimizer-->>Client: optimized plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
|
Hi @AilinKid. 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. |
|
/ok-to-test |
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/planner/core/joinorder/ordered_leading.go (1)
77-99: Consider handling GenerateQBName error more explicitly.The error from
GenerateQBNameis silently swallowed, leavingqbNameas an emptyCIStr. While this may work (the hint can still match without a QBName), consider logging or documenting this fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/joinorder/ordered_leading.go` around lines 77 - 99, The function buildSingleTableLeadingHint currently swallows errors from hint.GenerateQBName leaving qbName empty; update buildSingleTableLeadingHint to handle the error explicitly by checking the returned err and logging or recording it (e.g., using the package logger or standard log) when err != nil, so callers can see why a QBName wasn't generated; keep the fallback behavior of using an empty ast.CIStr but add a clear log message referencing GenerateQBName, qbName and buildSingleTableLeadingHint so the failure is discoverable during debugging.plans/order-aware-cdc-join-reorder-rule.md (1)
9-9: Minor: Hyphenate compound adjective.Static analysis flagged "CD-C based" should be "CD-C-based" when used as a compound adjective.
📝 Suggested fix
-and feed that preference into the new CD-C based join reorder implementation as a separate logical rule. +and feed that preference into the new CD-C-based join reorder implementation as a separate logical rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/order-aware-cdc-join-reorder-rule.md` at line 9, The phrase "CD-C based" is a compound adjective and should be hyphenated as "CD-C-based"; update the text where it appears (context: the sentence mentioning "CD-C based join reorder implementation" and references to JoinReOrderSolver and join group) to use "CD-C-based" so the compound modifier is correct and consistent with surrounding wording.
🤖 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/rule_order_aware_join_reorder.go`:
- Around line 92-95: The tests enable the failpoint "enableCDCJoinReorder" but
production lacks a failpoint hook, so update shouldUseCDCBasedJoinReorder (or an
appropriate initializer in the joinorder module) to honor that failpoint: add a
failpoint.Inject("enableCDCJoinReorder", ...) branch that forces a true/false
return or toggles the session var checks so the function returns the intended
value during tests; ensure the injected branch short-circuits or sets
vars.TiDBOptEnableAdvancedJoinReorder / vars.TiDBOptJoinReorderThreshold
equivalently so existing callers of shouldUseCDCBasedJoinReorder behave as tests
expect.
---
Nitpick comments:
In `@pkg/planner/core/joinorder/ordered_leading.go`:
- Around line 77-99: The function buildSingleTableLeadingHint currently swallows
errors from hint.GenerateQBName leaving qbName empty; update
buildSingleTableLeadingHint to handle the error explicitly by checking the
returned err and logging or recording it (e.g., using the package logger or
standard log) when err != nil, so callers can see why a QBName wasn't generated;
keep the fallback behavior of using an empty ast.CIStr but add a clear log
message referencing GenerateQBName, qbName and buildSingleTableLeadingHint so
the failure is discoverable during debugging.
In `@plans/order-aware-cdc-join-reorder-rule.md`:
- Line 9: The phrase "CD-C based" is a compound adjective and should be
hyphenated as "CD-C-based"; update the text where it appears (context: the
sentence mentioning "CD-C based join reorder implementation" and references to
JoinReOrderSolver and join group) to use "CD-C-based" so the compound modifier
is correct and consistent with surrounding wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 09990315-15d5-452d-bd22-f499c87a18b2
📒 Files selected for processing (13)
pkg/planner/core/BUILD.bazelpkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/main_test.gopkg/planner/core/casetest/rule/rule_cdc_join_reorder_test.gopkg/planner/core/casetest/rule/testdata/order_aware_join_reorder_suite_in.jsonpkg/planner/core/casetest/rule/testdata/order_aware_join_reorder_suite_out.jsonpkg/planner/core/casetest/rule/testdata/order_aware_join_reorder_suite_xut.jsonpkg/planner/core/joinorder/BUILD.bazelpkg/planner/core/joinorder/ordered_leading.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/optimizer.gopkg/planner/core/rule_order_aware_join_reorder.goplans/order-aware-cdc-join-reorder-rule.md
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, 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 |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest-required |
4 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
What problem does this PR solve?
Issue Number: ref #65208
Problem Summary:
The order-aware logic for the new CD-C join reorder path should live in a separate logical rule instead of being mixed into the generic join reorder solver.
What changed and how does it work?
This PR adds a new logical rule that inspects join groups with propagated TopN / ORDER BY columns, reuses the CD-C
extractJoinGrouplogic, and injects an internalLEADINGpreference when one ordered leaf can preserve the required ordering with compatible equality filters.The change also adds dedicated planner rule tests for the new order-aware path.
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
New Features
Behavior
Tests
Chores