Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

executor: create PipelinedWindowExec #23022

Merged
merged 14 commits into from Jun 3, 2021
Merged

executor: create PipelinedWindowExec #23022

merged 14 commits into from Jun 3, 2021

Conversation

ichn-hu
Copy link
Contributor

@ichn-hu ichn-hu commented Mar 1, 2021 •
edited

What problem does this PR solve?

Problem Summary:

What is changed and how it works?

Proposal: see design doc

What's Changed:

How it Works:

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Side effects

Release note

  • No release note

@ichn-hu ichn-hu requested a review from a team as a code owner March 1, 2021 07:52
@ichn-hu ichn-hu requested review from qw4990 and removed request for a team March 1, 2021 07:52
@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 1, 2021
@github-actions github-actions bot added the sig/execution SIG execution label Mar 1, 2021
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 1, 2021
@Tjianke
Copy link
Contributor

Tjianke commented Mar 1, 2021

Just FYI, the link is not available to public.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Mar 1, 2021

Just FYI, the link is not available to public.

Yeah, thanks for pointing this out, I will create a RFC PR for this.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Mar 1, 2021

@Tjianke I realized I can just turn the doc to open to web, so here you are: doc. The link in the PR description is also updated.

@Tjianke
Copy link
Contributor

Tjianke commented Mar 1, 2021

@Tjianke I realized I can just turn the doc to open to web, so here you are: doc. The link in the PR description is also updated.

I'm afraid the link is still not generally available, prompting corresponding rights needed.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Mar 1, 2021

@Tjianke I realized I can just turn the doc to open to web, so here you are: doc. The link in the PR description is also updated.

I'm afraid the link is still not generally available, prompting corresponding rights needed.

Sooorry, please see the design doc PR: https://github.com/ichn-hu/tidb/blob/add-pw-dc/docs/design/2021-03-01-pipelined-window-functions.md

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Mar 1, 2021

/cc @lzmhhh123

@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 31, 2021
@ichn-hu ichn-hu mentioned this pull request Apr 1, 2021
Closed
7 tasks
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 14, 2021
@ichn-hu ichn-hu requested a review from a team as a code owner April 14, 2021 09:27
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 14, 2021
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Apr 14, 2021
@ichn-hu ichn-hu changed the title executor: create PipelinedWindowExec based on current implementation and modify the windowProcessor interface executor: create PipelinedWindowExec Apr 21, 2021
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 21, 2021
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Jun 3, 2021

/run-all-tests tidb-test=pr/1203

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lzmhhh123
  • wshwsh12

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 3, 2021
@lzmhhh123
Copy link
Member

/run-all-tests tidb-test=pr/1203

@lzmhhh123
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 62ee4bd

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 3, 2021
@hi-rustin
Copy link
Member

/hold

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2021
@hi-rustin
Copy link
Member

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2021
@ti-chi-bot ti-chi-bot merged commit dbb753f into pingcap:master Jun 3, 2021
@ichn-hu ichn-hu mentioned this pull request Jun 30, 2021
Open
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 17, 2021

@tisonkun this pr implements all todo

@tisonkun tisonkun mentioned this pull request Oct 1, 2021
Merged
LittleFall
LittleFall reviewed Feb 16, 2022
View reviewed changes
expression/aggregation/window_func.go
Comment on lines +73 to +88
var useDefaultFrameWindowFuncs = map[string]ast.FrameClause{
ast.WindowFuncRowNumber: {
Type: ast.Rows,
Extent: ast.FrameExtent{
Start: ast.FrameBound{Type: ast.CurrentRow},
End: ast.FrameBound{Type: ast.CurrentRow},
},
},
}

// UseDefaultFrame indicates if the window function has a provided frame that will override user's designation
func UseDefaultFrame(name string) (bool, ast.FrameClause) {
frame, ok := useDefaultFrameWindowFuncs[strings.ToLower(name)]
return ok, frame
}

Copy link
Contributor

Choose a reason for hiding this comment

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

why row_number need default frame and others needn't?
it may block func grouping just like:

mysql> explain select *, row_number() over w, sum(salary) over w from employee window w as (partition by deptid);
+--------------------------------+---------+-----------+----------------+---------------------------------------------------------------------------------------------------------+
| id                             | estRows | task      | access object  | operator info                                                                                           |
+--------------------------------+---------+-----------+----------------+---------------------------------------------------------------------------------------------------------+
| Projection_8                   | 17.00   | root      |                | test.employee.empid, test.employee.deptid, test.employee.salary, Column#8, Column#7                     |
| └─Window_10                    | 17.00   | root      |                | row_number()->Column#8 over(partition by test.employee.deptid rows between current row and current row) |
|   └─Window_11                  | 17.00   | root      |                | sum(test.employee.salary)->Column#7 over(partition by test.employee.deptid)                             |
|     └─Sort_15                  | 17.00   | root      |                | test.employee.deptid                                                                                    |
|       └─TableReader_14         | 17.00   | root      |                | data:TableFullScan_13                                                                                   |
|         └─TableFullScan_13     | 17.00   | cop[tikv] | table:employee | keep order:false, stats:pseudo                                                                          |
+--------------------------------+---------+-----------+----------------+---------------------------------------------------------------------------------------------------------+
6 rows in set (0.00 sec)

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

@zz-jason zz-jason zz-jason left review comments

@LittleFall LittleFall LittleFall left review comments

@wshwsh12 wshwsh12 wshwsh12 approved these changes

@lzmhhh123 lzmhhh123 lzmhhh123 approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ichn-hu @Tjianke @ti-chi-bot @lzmhhh123 @hi-rustin @zz-jason @wshwsh12 @LittleFall