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/core: check the window func arg first before checking window specs (#11613) #11705

Merged
merged 11 commits into from
Aug 13, 2019

Conversation

sre-bot
Copy link
Contributor

@sre-bot sre-bot commented Aug 10, 2019

cherry-pick #11613 to release-3.0


What problem does this PR solve?

Fix #11008.

What is changed and how it works?

it seems not a good idea but we need to be compatible with mysql.
so here, when extract windows funcs, it the continue to check the window arg. when checking window func arg, it need to be able to do the expression check.

Check List

Tests

  • Unit test -- added
  • Integration test -- the explain test tool is changed because the plan id increased. and the error EOF is ignored in test.

Code changes

  • Has exported function/method change -- add a function to check the window args.

Side effects

  • Increased code complexity and the process of parsing and check arg is done twice.

@sre-bot
Copy link
Contributor Author

sre-bot commented Aug 10, 2019

/run-all-tests

@alivxxx
Copy link
Contributor

alivxxx commented Aug 12, 2019

Could you fix the CI? @gaoxingliang

@gaoxingliang
Copy link
Contributor

gaoxingliang commented Aug 12, 2019

Hi, do you mean I need to fork from https://github.com/sre-bot/tidb/tree/release-3.0-4995fe741ea5 and create a PR into this branch?

I run the code and found it passed. is the code in CI not latest version? in previous commits, it will cause this problem.

building explain-test binary: ./explain_test
start tidb-server, log file: ./explain-test.out
tidb-server(PID: 27538) started
run all explain test cases

Great, All tests passed
explaintest end
➜  tidb git:(release-3.0-4995fe741ea5) 

after I merged from upstream/release-3.0 into release-3.0-4995fe741ea5, it's still passed.

alivxxx
alivxxx previously approved these changes Aug 13, 2019
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx dismissed their stale review August 13, 2019 07:51

Only 1 LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 13, 2019
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 13, 2019
@sre-bot
Copy link
Contributor Author

sre-bot commented Aug 13, 2019

/run-all-tests

@sre-bot sre-bot merged commit 09e10fa into pingcap:release-3.0 Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants