executor: move window executors into windows subpackage#67993
Merged
ti-chi-bot[bot] merged 2 commits intoApr 23, 2026
Conversation
ti-chi-bot
Bot
added
do-not-merge/release-note-label-needed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMoves window executor code into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Builder
participant Session as sessionctx.Context
participant Child as ChildExecutor
participant WindowsPkg as windows.Build
participant Pipelined as PipelinedWindowExec
participant NonPipelined as WindowExec/Processor
Builder->>Session: read settings (EnablePipelinedWindowExec)
Builder->>Child: provide child executor
Builder->>WindowsPkg: Build(plan, Child, forcePipelined)
WindowsPkg->>WindowsPkg: compute partition/order cols\nbuild agg funcs & result slots
alt pipelined chosen
WindowsPkg->>Pipelined: construct PipelinedWindowExec\nconfigure sliding-window metadata
WindowsPkg-->>Builder: return PipelinedWindowExec
else non-pipelined chosen
WindowsPkg->>NonPipelined: select processor (agg/row/range)\nconfigure frame compare cols
WindowsPkg-->>Builder: return WindowExec wrapping processor
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
hawkingrei
changed the title
hawkingrei
force-pushed
the
executor-windows-package-only
branch
from
f75f9fc to
5bb21f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/executor/windows/BUILD.bazel`:
- Around line 28-38: The go_test target "windows_test" is not depending on the
windows library so it won't verify that the :windows package compiles; update
the go_test rule (name "windows_test", srcs "window_test.go") to either add
embed = [":windows"] or add ":windows" to deps so the test target depends on the
windows library and the build will fail if that package doesn't compile.
In `@pkg/executor/windows/pipelined_window.go`:
- Around line 103-108: OpenSelf currently resets many counters but leaves
PipelinedWindowExec.partialResults and PipelinedWindowExec.emptyFrame from
previous runs, causing stale aggregate state to be reused; update OpenSelf to
reinitialize partialResults to an empty slice (or nil) and set emptyFrame to
true (or its initial empty-state value) so each reuse starts with a fresh
partial result; modify the PipelinedWindowExec.OpenSelf implementation to
explicitly reset the fields partialResults and emptyFrame along with the other
initialized fields.
In `@pkg/executor/windows/window_test.go`:
- Around line 25-71: The tests currently drive SQL through testkit and therefore
exercise the existing SQL executor path instead of the new executor package;
update the tests to directly exercise the new builders/executors by invoking
windows.Build and instantiating/validating WindowExec, PipelinedWindowExec and
PartitionTopNWindowExec (or else mark these SQL-level tests to be skipped until
pkg/executor/builder.go is wired to pkg/executor/windows). Specifically, replace
the testkit-based assertions with unit tests that construct the logical window
plan (or use the package-level plan helpers), call windows.Build to produce
physical executors, and assert the executors produce the expected rows and
column nullable attributes for the functions tested
(sum/count/row_number/rank/dense_rank); include checks that both pipelined and
non-pipelined code paths are exercised.
In `@pkg/executor/windows/window.go`:
- Around line 287-317: The code path that handles empty ROWS frames (start >=
end) reuses stale p.partialResults from the previous non-empty frame; before
calling windowFunc.AppendFinalResult2Chunk you must reset the aggregate state
for each aggregate so the empty-frame semantics are produced. Locate
rowFrameWindowProcessor.appendResult2Chunk and, in the start >= end branch (and
the analogous RANGE branch mentioned around lines 417-453), clear or
reinitialize p.partialResults[i] for each windowFunc (and if a
slidingWindowAggFunc exists, reset its sliding state or mark
initializedSlidingWindow=false) so that AppendFinalResult2Chunk receives an
empty-frame partial result rather than the prior aggregation state.
- Around line 50-72: WindowExec retains mutable state across Open/Next calls
(fields executed, resultChunks, remainingRowsInChunk and internal processor
cursors) but never resets them, causing stale results on reuse; add/override an
Open method on WindowExec that calls BaseExecutor.Open(...) and then resets
executed=false, clears and nils resultChunks and remainingRowsInChunk, and
reinitializes any internal processor cursor/index fields used by consumeOneGroup
(e.g. partition/peer cursors) so the executor starts fresh on reopen. Ensure the
new Open signature matches the executor lifecycle and preserves the
BaseExecutor.Open call before clearing WindowExec-specific state.
🪄 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: fe819d90-0d10-453e-b0ce-3b07ca70398a
📒 Files selected for processing (6)
pkg/executor/windows/BUILD.bazelpkg/executor/windows/builder.gopkg/executor/windows/partition_topn_window.gopkg/executor/windows/pipelined_window.gopkg/executor/windows/window.gopkg/executor/windows/window_test.go
hawkingrei
force-pushed
the
executor-windows-package-only
branch
2 times, most recently
from
4f1e69c to
67bc798
Compare
ti-chi-bot
Bot
added
release-note-none
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/executor/windows/window_executor_test.go`:
- Around line 46-71: The test TestWindowReturnColumnNullableAttribute runs
window queries but doesn't enable window functions explicitly; before running
queries use the test kit to enable the feature by calling tk.MustExec("set
@@tidb_enable_window_function=1") (or equivalent session-set) at the start of
TestWindowReturnColumnNullableAttribute so checkNullable and the select
executions use a deterministic session with window functions enabled; place the
tk.MustExec call near the top of the test (after tk := testkit.NewTestKit(...))
and keep the change minimal and local to this test.
🪄 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: d7808589-f3d7-4be1-a099-fbfcdcdcfbf3
📒 Files selected for processing (8)
pkg/executor/BUILD.bazelpkg/executor/builder.gopkg/executor/windows/BUILD.bazelpkg/executor/windows/builder.gopkg/executor/windows/pipelined_window.gopkg/executor/windows/window.gopkg/executor/windows/window_executor_test.gopkg/executor/windows/window_sql_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/executor/windows/window_sql_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/executor/windows/BUILD.bazel
- pkg/executor/windows/window.go
- pkg/executor/windows/pipelined_window.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67993 +/- ##
================================================
- Coverage 77.7924% 77.4387% -0.3538%
================================================
Files 1982 1970 -12
Lines 549742 550794 +1052
================================================
- Hits 427658 426528 -1130
- Misses 121164 124264 +3100
+ Partials 920 2 -918
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hawkingrei
force-pushed
the
executor-windows-package-only
branch
from
67bc798 to
67a3de4
Compare
ti-chi-bot
Bot
added
size/XL
hawkingrei
force-pushed
the
executor-windows-package-only
branch
from
67a3de4 to
0706b97
Compare
hawkingrei
force-pushed
the
executor-windows-package-only
branch
from
0706b97 to
9ad10fa
Compare
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
hawkingrei
changed the title
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
wshwsh12
approved these changes
Apr 23, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: windtalker, wshwsh12 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
[LGTM Timeline notifier]Timeline:
|
Member
Author
|
/retest |
2 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
13 tasks
premal
pushed a commit
to premal/tidb
that referenced
this pull request
Apr 30, 2026
This was referenced May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #67989
Problem Summary:
The previous executor split was still too large for review. This PR narrows the
scope to the generic window executor move only, so later planner or ordered-input
work can be reviewed separately on top of the new package boundary.
What changed and how does it work?
WindowExecandPipelinedWindowExecfrompkg/executortopkg/executor/windowspkg/executor/builder.goas the entry point and delegate window executorconstruction to
windows.Buildpkg/executor/window_test.gotopkg/executor/windows/window_sql_test.gopkg/executor/windows/window_executor_test.goCheck List
Tests
Side effects
Documentation
Test
Release note
Summary by CodeRabbit
Tests
Refactor
Chores