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

planner: report error for invalid window specs which are not used (#21083) #21976

Merged
merged 9 commits into from
Jan 27, 2021

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #21083 to release-4.0


What problem does this PR solve?

Issue Number: close #12356

Problem Summary: Didn't check for invalid window specs if they are not used in window functions.

What is changed and how it works?

  • Add new clause codes windowOrderBy, partitionBy.
  • Modify groupWindowFuncs to store unused named window specs.
  • Check for unused named window specs in buildWindowFunctions.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • Report error for invalid window specifications which are not used in window functions.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@dyzsr you're already a collaborator in bot's repo.

Copy link
Contributor

@lzmhhh123 lzmhhh123 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-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 31, 2020
@jebter jebter modified the milestones: 4.0.0, v4.0.10 Jan 7, 2021
@jebter jebter modified the milestones: v4.0.10, v4.0.11 Jan 18, 2021
@dyzsr
Copy link
Contributor

dyzsr commented Jan 20, 2021

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

@dyzsr
Copy link
Contributor

dyzsr commented Jan 20, 2021

/run-common-test tidb-test=pr/1153
/run-integration-common-test tidb-test=pr/1153

@dyzsr
Copy link
Contributor

dyzsr commented Jan 20, 2021

/run-integration-common-test tidb-test=pr/1153

zz-jason
zz-jason previously approved these changes Jan 25, 2021
@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 25, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

Please resolve conflicts; @dyzsr @XuHuaiyu

@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 26, 2021
@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 21945
  • 22136
  • 22119
  • 21853
  • 22174
  • 21881
  • 22527
  • 22527
  • 22148
  • 22174
  • 22119

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-all-tests

@dyzsr
Copy link
Contributor

dyzsr commented Jan 27, 2021

/run-integration-common-test tidb-test=pr/1153

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

Should we merge https://github.com/pingcap/tidb-test/pull/1153 first? @dyzsr

@dyzsr
Copy link
Contributor

dyzsr commented Jan 27, 2021

Should we merge pingcap/tidb-test#1153 first? @dyzsr

Yes

@dyzsr
Copy link
Contributor

dyzsr commented Jan 27, 2021

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

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-all-tests

@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jan 27, 2021
@qw4990 qw4990 merged commit f0a757b into pingcap:release-4.0 Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants