Skip to content

executor: add ordered window builder#68480

Open
hawkingrei wants to merge 2 commits into
pingcap:masterfrom
hawkingrei:exec-ordered-window-only
Open

executor: add ordered window builder#68480
hawkingrei wants to merge 2 commits into
pingcap:masterfrom
hawkingrei:exec-ordered-window-only

Conversation

@hawkingrei
Copy link
Copy Markdown
Member

@hawkingrei hawkingrei commented May 19, 2026

What problem does this PR solve?

Issue Number: ref #67989

Problem Summary:

This PR keeps only the executor-side building block needed by follow-up ordered-window planning work. It intentionally does not include planner enumeration, index-join fusion, or partition TopN pruning.

What changed and how does it work?

  • Add OrderedWindowExec as a thin wrapper over PipelinedWindowExec.
  • Add PipelinedWindowExec.OpenSelf() so builder paths that already opened children can initialize only window-local state.
  • Add BuildOrdered(...), which forces the pipelined window executor and leaves the ordered-input guarantee to the caller.
  • Add direct executor coverage for building and running the ordered-window wrapper while tidb_enable_pipelined_window_function is disabled.

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.

Commands:

  • make bazel_prepare
  • git diff --check

Additional validation:

  • The focused executor test passed before the naming-only follow-up.

Local limitation:

  • The renamed focused test, the full package test, and the Bazel target were attempted but blocked by local disk exhaustion (No space left on device) while writing temporary build outputs.

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

    • Optimized window function execution with improved handling for scenarios where input data is already properly partitioned and sorted according to window requirements.
  • Tests

    • Comprehensive test coverage added for window execution functionality to validate correctness across various data scenarios.

Review Change Stack

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

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: d7b45789-dbb1-4737-9dfb-226e61fb0b9a

📥 Commits

Reviewing files that changed from the base of the PR and between a62a6d1 and 4f7242d.

📒 Files selected for processing (4)
  • pkg/executor/windows/BUILD.bazel
  • pkg/executor/windows/builder.go
  • pkg/executor/windows/pipelined_window.go
  • pkg/executor/windows/window_executor_test.go

📝 Walkthrough

Walkthrough

This PR introduces OrderedWindowExec, a new executor for window functions when child data already satisfies partition and order requirements. It refactors PipelinedWindowExec initialization by extracting setup logic into OpenSelf(), adds the BuildOrdered() builder function, and includes comprehensive test coverage with updated Bazel configuration.

Changes

Ordered Window Executor

Layer / File(s) Summary
Executor types and initialization refactor
pkg/executor/windows/pipelined_window.go
OrderedWindowExec struct embeds *PipelinedWindowExec. PipelinedWindowExec.Open() refactored to call BaseExecutor.Open() first, then delegate frame/buffer initialization to new OpenSelf() method.
BuildOrdered API entry point
pkg/executor/windows/builder.go
New BuildOrdered() function builds window executors for pre-ordered child data by calling Build(forcePipelined=true) and type-asserting to *PipelinedWindowExec, wrapped in OrderedWindowExec. Adds github.com/pingcap/errors import for error handling.
Test validation and build configuration
pkg/executor/windows/window_executor_test.go, pkg/executor/windows/BUILD.bazel
TestBuildOrderedWindowExec validates BuildOrdered() end-to-end: builds mock child, creates row_number window plan, asserts executor type, opens/drains chunks, verifies output. Helper physicalWindowForTest constructs test window plans. BUILD.bazel windows_test target updated with shard_count=8 and new dependencies.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • pingcap/tidb#67993: Prior PR that moved window executors into the pkg/executor/windows subpackage and introduced windows.Build; this PR extends that foundation with BuildOrdered and executor lifecycle refactoring.

Suggested labels

size/L, approved, lgtm

Suggested reviewers

  • windtalker
  • wshwsh12
  • joechenrh

Poem

🐰 A window frame, so neat and tidy,
Ordered data marching by,
BuildOrdered shows the way,
Pipelined streams in grand array,
Tests confirm it works just right! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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 ordered window builder' accurately reflects the main change—adding a new BuildOrdered function and OrderedWindowExec wrapper for window execution.
Description check ✅ Passed The PR description is comprehensive and mostly complete, covering the problem statement (ref #67989), detailed explanation of changes, test coverage, and proper checklist selections; however, it lacks structured clarity in some sections.
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.

Copy link
Copy Markdown
Contributor

@windtalker windtalker left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 19, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 18.75000% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.3559%. Comparing base (a62a6d1) to head (4f7242d).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68480        +/-   ##
================================================
- Coverage   77.2798%   75.3559%   -1.9239%     
================================================
  Files          2010       2018         +8     
  Lines        555326     566697     +11371     
================================================
- Hits         429155     427040      -2115     
- Misses       125251     139359     +14108     
+ Partials        920        298       -622     
Flag Coverage Δ
integration 41.4609% <18.7500%> (+1.6679%) ⬆️

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

Components Coverage Δ
dumpling 60.4679% <ø> (ø)
parser ∅ <ø> (∅)
br 49.9534% <ø> (-13.0557%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@hawkingrei hawkingrei requested review from AilinKid and guo-shaoge May 19, 2026 05:55
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 19, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tangenta, windtalker

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [tangenta,windtalker]

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 lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 19, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 19, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-05-19 04:07:13.587816096 +0000 UTC m=+237163.091946762: ☑️ agreed by windtalker.
  • 2026-05-19 06:30:59.8149476 +0000 UTC m=+245789.319078276: ☑️ agreed by tangenta.

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

3 similar comments
@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@hawkingrei
Copy link
Copy Markdown
Member Author

/retest

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 19, 2026

@hawkingrei: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-integration-realcluster-test-next-gen 4f7242d link unknown /test pull-integration-realcluster-test-next-gen

Full PR test history. Your PR dashboard.

Details

Instructions 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. I understand the commands that are listed here.

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

Labels

approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants