Skip to content

planner/core: check the window func arg first before checking window specs#11613

Merged
SunRunAway merged 14 commits into
pingcap:masterfrom
gaoxingliang:fixwindow
Aug 10, 2019
Merged

planner/core: check the window func arg first before checking window specs#11613
SunRunAway merged 14 commits into
pingcap:masterfrom
gaoxingliang:fixwindow

Conversation

@gaoxingliang
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

sre-bot commented Aug 5, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Aug 5, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 5, 2019

Codecov Report

Merging #11613 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11613   +/-   ##
===========================================
  Coverage   81.6469%   81.6469%           
===========================================
  Files           429        429           
  Lines         93614      93614           
===========================================
  Hits          76433      76433           
  Misses        11780      11780           
  Partials       5401       5401

Comment thread planner/core/logical_plan_builder.go Outdated
@alivxxx alivxxx requested a review from SunRunAway August 7, 2019 03:21
@zz-jason zz-jason added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Aug 7, 2019
@zz-jason zz-jason changed the title planner/window: check the window func arg first before checking window specs planner/core: check the window func arg first before checking window specs Aug 7, 2019
Copy link
Copy Markdown
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 added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 8, 2019
Comment thread planner/core/logical_plan_builder.go Outdated
return proj, propertyItems[:lenPartition], propertyItems[lenPartition:], newArgList, nil
}

func (b *PlanBuilder) buildArgs4WindowFunc(ctx context.Context, p LogicalPlan, args []ast.ExprNode, aggMap map[*ast.AggregateFuncExpr]int) ([]expression.Expression, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could buildArgs4WindowFunc also be used in buildProjectionForWindow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no. if we use that, we need to change func buildArgs4WindowFunc to add some projection information into the plan. and then you may find the two methods are same. if so, I think it's much better just call same method instead of two methods

Copy link
Copy Markdown
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. needs-cherry-pick-3.0 and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 9, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 9, 2019

/run-all-tests

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 9, 2019

@gaoxingliang merge failed.

@gaoxingliang
Copy link
Copy Markdown
Contributor Author

@gaoxingliang merge failed.

seems other cases failed

@SunRunAway
Copy link
Copy Markdown
Contributor

/run-integration-common-test

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 10, 2019

cherry pick to release-3.0 in PR #11705

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.

window function: should check fuction's arguments firstly.

5 participants