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.

Sign up for GitHub

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

window function: function name of error message should be lowercase #11047

Merged
merged 6 commits into from
Jul 5, 2019
Merged

window function: function name of error message should be lowercase #11047

merged 6 commits into from
Jul 5, 2019

Conversation

tcmichael
Copy link

@tcmichael tcmichael commented Jul 3, 2019 •
edited by SunRunAway
Loading

What problem does this PR solve?

function name of error message should be lowercase
Closes #11009

What is changed and how it works?

wrap windowFunc.F with strings.ToLower

Check List

Tests

  • Unit test

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@tcmichael
Copy link
Author

@SunRunAway PTAL

@codecov
Copy link

codecov bot commented Jul 3, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11047   +/-   ##
===========================================
  Coverage   81.1351%   81.1351%           
===========================================
  Files           420        420           
  Lines         89802      89802           
===========================================
  Hits          72861      72861           
  Misses        11695      11695           
  Partials       5246       5246

alivxxx
alivxxx reviewed Jul 3, 2019
View reviewed changes
planner/core/logical_plan_builder.go
@@ -3148,7 +3148,7 @@ func (b *PlanBuilder) buildWindowFunctions(p LogicalPlan, groupedFuncs map[*ast.
return nil, nil, err
}
if desc == nil {
return nil, nil, ErrWrongArguments.GenWithStackByArgs(windowFunc.F)
return nil, nil, ErrWrongArguments.GenWithStackByArgs(strings.ToLower(windowFunc.F))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does ErrWindowInvalidWindowFuncUse need to lower the function name?

Copy link
Author

Choose a reason for hiding this comment

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

ErrWindowInvalidWindowFuncUse is not mentioned in #11009.
I think it's another issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

#11508

SunRunAway
SunRunAway reviewed Jul 3, 2019
View reviewed changes
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 sig/planner SIG: Planner type/bugfix This PR fixes a bug. status/LGT1 Indicates that a PR has LGTM 1. labels Jul 3, 2019
@SunRunAway
Copy link
Contributor

/run-all-tests

@jackysp jackysp added the contribution This PR is from a community contributor. label Jul 3, 2019
winoros
winoros approved these changes Jul 4, 2019
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@winoros
Copy link
Member

winoros commented Jul 4, 2019

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

@SunRunAway
Copy link
Contributor

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

1 similar comment
@SunRunAway
Copy link
Contributor

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

@SunRunAway
Copy link
Contributor

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

@SunRunAway
Copy link
Contributor

/rebuild

@SunRunAway
Copy link
Contributor

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

@SunRunAway SunRunAway merged commit 7177291 into pingcap:master Jul 5, 2019
@tcmichael tcmichael deleted the bugfix branch July 5, 2019 03:29
@SunRunAway SunRunAway mentioned this pull request Jul 8, 2019
Merged
zz-jason added a commit to SunRunAway/tidb that referenced this pull request Jul 8, 2019
@zz-jason
Merge branch 'release-3.0' into automated-cherry-pick-of-pingcap#11047-…
4d591ad 
…release-3.0
zz-jason added a commit to SunRunAway/tidb that referenced this pull request Jul 8, 2019
@zz-jason
Merge branch 'release-3.0' into automated-cherry-pick-of-pingcap#11047-…
7d9d668 
…release-3.0
@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @lamxTyler PTAL.

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Apr 8, 2020

/run-cherry-picker

@sre-bot
Copy link
Contributor

sre-bot commented Apr 8, 2020

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Apr 8, 2020

cherry pick to release-3.1 failed

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

@alivxxx alivxxx alivxxx left review comments

@SunRunAway SunRunAway SunRunAway left review comments

@winoros winoros winoros approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

window function: function name of error message should be lowercase
8 participants
@tcmichael @CLAassistant @SunRunAway @winoros @sre-bot @XuHuaiyu @alivxxx @jackysp