Skip to content

executor: add stream window execution helpers#68113

Open
hawkingrei wants to merge 5 commits into
pingcap:masterfrom
hawkingrei:exec-stream-window-slice
Open

executor: add stream window execution helpers#68113
hawkingrei wants to merge 5 commits into
pingcap:masterfrom
hawkingrei:exec-stream-window-slice

Conversation

@hawkingrei
Copy link
Copy Markdown
Member

@hawkingrei hawkingrei commented Apr 29, 2026

What problem does this PR solve?

Issue Number: ref #67989

Problem Summary:

This PR splits the executor-side stream-window building blocks out of the demo PR #67696 so they can be reviewed independently from planner enumeration and plan golden changes.

What changed and how does it work?

  • Add StreamWindowExec as a thin wrapper over the existing pipelined window executor.
  • Add PipelinedWindowExec.OpenSelf() so builder paths that already opened children can initialize only the window executor state.
  • Add PartitionTopNWindowExec, an executor-side helper for row_number() <= K stream-window pruning that emits at most K rows per partition and materializes the row_number column.
  • Add direct executor coverage for cross-chunk partition handling.
  • Add agent notes documenting the executor-only boundary and the follow-up planner split.

This PR intentionally does not add planner admission rules, physical stream-window enumeration, or plan goldens. Those should be reviewed in a follow-up PR.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

  • New Features

    • Partitioned Top‑N window processing
    • Stream‑window executor for pipelined window execution
  • Fixes / Reliability

    • Improved group‑boundary handling and reset semantics across chunk boundaries and repeated executions
    • Explicit reset behavior to allow safe executor reuse
  • Documentation

    • Added executor notes on stream‑window behavior, pruning limits, and testing guidance
  • Tests

    • New unit tests validating partitioned Top‑N behavior across chunk boundaries

@ti-chi-bot ti-chi-bot Bot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 29, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 29, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign 3pointer for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 29, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f85abd19-cbf7-4c30-8eaa-565bf215bc21

📥 Commits

Reviewing files that changed from the base of the PR and between d19bfff and 49484b5.

📒 Files selected for processing (9)
  • docs/agents/architecture-index.md
  • docs/agents/executor/stream_window_notes.md
  • pkg/executor/internal/vecgroupchecker/vec_group_checker.go
  • pkg/executor/internal/vecgroupchecker/vec_group_checker_test.go
  • pkg/executor/windows/BUILD.bazel
  • pkg/executor/windows/builder.go
  • pkg/executor/windows/partition_topn_window.go
  • pkg/executor/windows/partition_topn_window_test.go
  • pkg/executor/windows/pipelined_window.go
✅ Files skipped from review due to trivial changes (2)
  • docs/agents/architecture-index.md
  • docs/agents/executor/stream_window_notes.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • pkg/executor/internal/vecgroupchecker/vec_group_checker_test.go
  • pkg/executor/internal/vecgroupchecker/vec_group_checker.go
  • pkg/executor/windows/BUILD.bazel
  • pkg/executor/windows/pipelined_window.go
  • pkg/executor/windows/builder.go

📝 Walkthrough

Walkthrough

Adds a StreamWindow wrapper and PartitionTopN window executor, splits PipelinedWindowExec initialization into Open/OpenSelf, introduces vecgroupchecker execution reset, builder helpers, BUILD/test updates, and executor runbook notes.

Changes

Partition Top‑N + Stream Window Executor

Layer / File(s) Summary
Data / Grouping reset
pkg/executor/internal/vecgroupchecker/vec_group_checker.go
Adds ResetForNewExecution() and clarifies Reset() comment to control execution-level carryover and boundary detection. pkg/executor/internal/vecgroupchecker/vec_group_checker_test.go asserts behavior difference after Reset() vs ResetForNewExecution().
Core executor: Pipelined → Stream init split
pkg/executor/windows/pipelined_window.go
Adds StreamWindowExec, refactors PipelinedWindowExec.Open to call OpenSelf() for state-only init, and implements OpenSelf to reset window-internal state without opening children.
Core executor: PartitionTopN
pkg/executor/windows/partition_topn_window.go
Introduces PartitionTopNWindowExec with Open/OpenSelf/Close/Next, cross-chunk group splitting via VecGroupChecker, per-partition rank tracking, emission up to limitCount, and handling of partitions spanning child chunks.
Builder wiring
pkg/executor/windows/builder.go
Adds BuildStream(...) to wrap a pipelined exec as StreamWindowExec and BuildPartitionTopN(...) to construct PartitionTopNWindowExec with partition expressions, resultColIdx, and limitCount.
Tests: PartitionTopN
pkg/executor/windows/partition_topn_window_test.go
Adds TestPartitionTopNWindowExecAcrossChunks, buildPartitionTopNWindowExecForTest, and drainPartitionTopNRows to validate per-partition top‑K behavior across chunk boundaries and reopen determinism.
Build config / deps
pkg/executor/windows/BUILD.bazel
Adds partition_topn_window.go and test to targets, increases test sharding, and adds test dependencies (//pkg/types, mock utilities, stretchr/testify/require, etc.).
Documentation / runbook
docs/agents/architecture-index.md, docs/agents/executor/stream_window_notes.md
Adds runbook index link and notes documenting StreamWindowExec semantics, PipelinedWindowExec.OpenSelf() behavior, PartitionTopNWindowExec pruning semantics, and cross-chunk testing guidance.

Sequence Diagram(s)

sequenceDiagram
    participant Exec as PartitionTopNWindowExec
    participant Child as Child Executor
    participant Grouper as VecGroupChecker
    participant Output as Output Chunk

    Exec->>Exec: Open() / OpenSelf() (reset execution state)
    loop produce output
        Exec->>Child: Next(req) to fetch child chunk
        Child-->>Exec: return chunk
        Exec->>Grouper: SplitIntoGroups(chunk, partitionBy)
        Grouper-->>Exec: group boundaries
        loop per-row in current group
            Exec->>Exec: increment groupRank
            alt groupRank <= limitCount
                Exec->>Output: Append row + write row_number into resultColIdx
            else
                Exec->>Exec: skip row (limit reached)
            end
            alt output chunk full
                Exec->>Output: return chunk to caller
            end
        end
        alt last partition spans chunk
            Exec->>Exec: resume groupRank on next child chunk
        else
            Exec->>Exec: reset groupRank for new partition
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • pingcap/tidb#67993: Modifies the same pkg/executor/windows package and earlier window-executor refactors; likely directly related.

Suggested labels

approved, lgtm

Suggested reviewers

  • windtalker
  • wshwsh12
  • YangKeao

Poem

🐰
I hop through windows, rows in line,
Per-partition peaks I count and mind.
Streams reset softly when children wake,
Chunks and ranks—careful hops I take.
Carrots, code, and tests — a tidy bake!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'executor: add stream window execution helpers' clearly and concisely summarizes the main change: adding new window executor helpers for stream window processing.
Description check ✅ Passed The PR description provides comprehensive details about the problem statement, changes made, testing approach, and documentation updates, covering all major sections of the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

❌ Patch coverage is 2.80374% with 104 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.0336%. Comparing base (84d8269) to head (49484b5).

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68113        +/-   ##
================================================
- Coverage   77.7288%   77.0336%   -0.6953%     
================================================
  Files          1990       1973        -17     
  Lines        551970     552769       +799     
================================================
- Hits         429040     425818      -3222     
- Misses       122010     126948      +4938     
+ Partials        920          3       -917     
Flag Coverage Δ
integration 41.3509% <2.8037%> (+1.5491%) ⬆️

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

Components Coverage Δ
dumpling 60.4888% <ø> (ø)
parser ∅ <ø> (∅)
br 50.0597% <ø> (-13.0338%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

1 similar comment
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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/partition_topn_window.go`:
- Around line 58-67: OpenSelf currently resets executor fields but leaves
e.groupChecker state intact, which can carry stale
VecGroupChecker.lastGroupKeyOfPrevChk into the next execution and break
partition detection in SplitIntoGroups causing wrong groupRank values; call
e.groupChecker.Reset() (or explicitly clear lastGroupKeyOfPrevChk on
e.groupChecker) inside PartitionTopNWindowExec.OpenSelf so the group checker is
cleared when the executor is re-opened.
🪄 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: 096ba8ba-8475-41f0-84b0-a935b79f8a6c

📥 Commits

Reviewing files that changed from the base of the PR and between e5f28b5 and a6969ad.

📒 Files selected for processing (1)
  • pkg/executor/windows/partition_topn_window.go

Comment thread pkg/executor/windows/partition_topn_window.go
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

1 similar comment
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@hawkingrei hawkingrei force-pushed the exec-stream-window-slice branch from d19bfff to 49484b5 Compare May 7, 2026 06:05
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant