Repository navigation
planner, executor: plan ordered window for ordered input - #68520
hawkingrei wants to merge 20 commits into
Conversation
ti-chi-bot
Bot
added
release-note-none
|
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 a PhysicalOrderedWindow operator and wires it through planner generation, executor builder, task attachment, shuffle optimization, plan stringification, and test fixtures/expectations. ChangesOrdered Window Operator Optimization
Sequence Diagram(s)sequenceDiagram
participant LogicalPlanner
participant ExhaustPhysicalPlans4LogicalWindow
participant PhysicalOrderedWindow
participant ExecutorBuilder
participant windowexec_BuildOrdered
LogicalPlanner->>ExhaustPhysicalPlans4LogicalWindow: request physical plans for LogicalWindow
ExhaustPhysicalPlans4LogicalWindow->>PhysicalOrderedWindow: emit OrderedWindow when CanUseOrderedWindow
PhysicalOrderedWindow->>ExecutorBuilder: builder.build routes to buildOrderedWindow
ExecutorBuilder->>windowexec_BuildOrdered: build ordered-window executor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 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 `@pkg/planner/core/casetest/windows/window_push_down_test.go`:
- Around line 41-42: The test currently treats queries starting only with
"select" or "explain" as query-path verifiable and sends everything else
(including CTEs starting with "with") to tk.MustExec, which skips plan/warning
checks; update the condition that inspects stmt so that it also recognizes
"with" (case-insensitive, trim leading whitespace) as a query that should go
through the normal plan/warning verification path instead of calling
tk.MustExec(tt). Locate the stmt variable and the branch that calls
tk.MustExec(tt) in window_push_down_test.go (affecting tests like
TestWindowWithOuterJoinAndCTE) and modify the prefix check to include "with" (or
detect leading CTEs and then the underlying "select"/"explain") so CTE queries
are classified correctly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43fa2aff-fb42-4eb6-93d0-ce7f07d7a3d6
📒 Files selected for processing (12)
.agents/skills/tidb-test-guidelines/references/planner-case-map.mdpkg/executor/builder.gopkg/planner/core/casetest/windows/testdata/window_push_down_suite_in.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_out.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_xut.jsonpkg/planner/core/casetest/windows/window_push_down_test.gopkg/planner/core/operator/physicalop/base_physical_plan.gopkg/planner/core/operator/physicalop/physical_window.gopkg/planner/core/optimizer.gopkg/planner/core/plan.gopkg/planner/core/stringer.gopkg/planner/core/task.go
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 (1)
pkg/planner/core/casetest/windows/testdata/window_push_down_suite_in.json (1)
22-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the result-case deterministic with an explicit outer ORDER BY.
This case asserts exact row sequence but the query has no final
ORDER BY, so output order is not guaranteed by SQL semantics and can become flaky with future plan changes.Suggested change
- "select a, b, row_number() over(partition by a order by b) as rn from ordered_window_t", + "select a, b, row_number() over(partition by a order by b) as rn from ordered_window_t order by a, b",🤖 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 `@pkg/planner/core/casetest/windows/testdata/window_push_down_suite_in.json` at line 22, The test SQL lacks a final ordering so its asserted row sequence can be flaky; update the query string "select a, b, row_number() over(partition by a order by b) as rn from ordered_window_t" to include an explicit outer ORDER BY (for example ORDER BY a, b or ORDER BY rn) so the result order is deterministic; locate the string in window_push_down_suite_in.json and append the chosen ORDER BY clause to the query.
🤖 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 `@pkg/planner/core/casetest/windows/testdata/window_push_down_suite_in.json`:
- Line 22: The test SQL lacks a final ordering so its asserted row sequence can
be flaky; update the query string "select a, b, row_number() over(partition by a
order by b) as rn from ordered_window_t" to include an explicit outer ORDER BY
(for example ORDER BY a, b or ORDER BY rn) so the result order is deterministic;
locate the string in window_push_down_suite_in.json and append the chosen ORDER
BY clause to the query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d240250-6358-4bef-8eb7-ddd543309e57
📒 Files selected for processing (3)
pkg/planner/core/casetest/windows/testdata/window_push_down_suite_in.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_out.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_xut.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/operator/physicalop/physical_window.go (1)
69-75: ⚡ Quick winUse a
plancodecconstant for the type string.
PhysicalWindow.Initusesplancodec.TypeWindow(line 63), butPhysicalOrderedWindow.Inituses the raw string"OrderedWindow"(line 71). This inconsistency may affect plan hashing and serialization logic that uses the TP() value.Consider adding
TypeOrderedWindow = "OrderedWindow"to the plancodec package and using it here to maintain consistency with other plan 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 `@pkg/planner/core/operator/physicalop/physical_window.go` around lines 69 - 75, PhysicalOrderedWindow.Init currently calls NewBasePhysicalPlan with the raw string "OrderedWindow" causing inconsistency with other plan types that use plancodec constants (e.g., PhysicalWindow.Init uses plancodec.TypeWindow); add a new constant in the plancodec package (e.g., TypeOrderedWindow = "OrderedWindow") and replace the literal in PhysicalOrderedWindow.Init so it calls NewBasePhysicalPlan(ctx, plancodec.TypeOrderedWindow, &p, offset), preserving consistent TP() values for hashing/serialization.
🤖 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 `@pkg/planner/core/operator/physicalop/physical_window.go`:
- Around line 69-75: PhysicalOrderedWindow.Init currently calls
NewBasePhysicalPlan with the raw string "OrderedWindow" causing inconsistency
with other plan types that use plancodec constants (e.g., PhysicalWindow.Init
uses plancodec.TypeWindow); add a new constant in the plancodec package (e.g.,
TypeOrderedWindow = "OrderedWindow") and replace the literal in
PhysicalOrderedWindow.Init so it calls NewBasePhysicalPlan(ctx,
plancodec.TypeOrderedWindow, &p, offset), preserving consistent TP() values for
hashing/serialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e0bb654-d577-4133-bf4f-94222d7add73
📒 Files selected for processing (6)
pkg/planner/core/casetest/windows/testdata/window_push_down_suite_in.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_out.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_xut.jsonpkg/planner/core/casetest/windows/window_push_down_test.gopkg/planner/core/operator/physicalop/physical_window.gotests/integrationtest/r/planner/core/casetest/rule/rule_derive_topn_from_window.result
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68520 +/- ##
================================================
- Coverage 76.6358% 75.3814% -1.2545%
================================================
Files 2027 2021 -6
Lines 560954 569416 +8462
================================================
- Hits 429892 429234 -658
- Misses 130142 139859 +9717
+ Partials 920 323 -597
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XXL
|
/retest |
winoros
left a comment
winoros
left a comment
There was a problem hiding this comment.
So many tests changed.
Is this expected?
And I don't see there's a cost formula for the new operator.
ti-chi-bot
Bot
added
size/XL
ti-chi-bot
Bot
removed
the
size/XL
ti-chi-bot
Bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
I've made a lot of updates. Now it's minimal changes, with many newly added tests. The cost part still uses the Windows part. This is currently to let Windows make the most of indexes, and plans that use indexes inherently have lower cost than those that don't. |
What problem does this PR solve?
Issue Number: ref #67989
Problem Summary:
PR #68480 added the executor-side ordered window builder but left planner enumeration out of scope. This PR lets the optimizer choose an ordered-input window when the child naturally provides the required partition/order property, including index-join paths without join hints.
What changed and how does it work?
PhysicalOrderedWindowfor supportedrow_number()windows whose child already satisfies the window property. If aSortenforcer is required, the plan stays on the regularWindowpath.partition by ... order by ... descon indexes such as(partition_key, order_time).Example:
The main regression query joins a filtered probe table to a derived inner table and keeps only the latest row per partition key. The query itself has no join hint:
Before this PR, the window side had to be fully materialized and sorted:
After this PR, the optimizer can use
IndexHashJoinand build the window side from the ordered(partition_key, order_time)index. The backward index range scan provides the required order, so theSortand full inner-table scan disappear:The partition TopN pruning part from PR #67696 is not included here and should stay in a follow-up PR.
Check List
Tests
Commands:
./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/windows -run TestWindowPushDownPlans -count=1./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/join -run TestIndexJoinWithOrderedWindowInner -record./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/join -run TestIndexJoinWithOrderedWindowInner./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/join -count=1./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/windows -run 'TestWindow(FunctionDescCanPushDown|PlanWithOtherOperators)$' -count=1./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/windows -run 'TestWindow(PushDownPlans|WithOuterJoinAndCTE)$' -count=1./tools/check/failpoint-go-test.sh pkg/planner/core/casetest/rule -run TestOuter2InnerSuite -count=1./tools/check/failpoint-go-test.sh pkg/planner/core/operator/physicalop -run '^$' -count=0(cd tests/integrationtest && ./run-tests.sh -r planner/core/indexjoin)(cd tests/integrationtest && ./run-tests.sh -r planner/core/casetest/rule/rule_derive_topn_from_window)make lintAttempted but blocked locally:
make bazel_preparewas retried, but the latest run failed while fetchingcloud.google.com/go/storage@v1.39.1fromgoproxy.cnwithunexpected EOF. The generatedDEPS.bzlnoise from the failed run was restored; the relevant joinBUILD.bazeltestdata metadata is included.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.