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: fix different err msg from MySQL when group by window function (#16134) #16161

Closed

Conversation

sre-bot
Copy link
Contributor

@sre-bot sre-bot commented Apr 8, 2020

cherry-pick #16134 to release-3.0


What problem does this PR solve?

Issue Number: close #11518

Problem Summary: When GROUP BY is followed by a number in a select stmt and this number correspond to a window function in select fields, TiDB prints a different error message from MySQL.

What is changed and how it works?

What's Changed: When dealing with ast.PositionExpr in gbyResolver's Leave() method, we check if it's a ast.WindowFuncExpr. If so, ErrWrongGroupField will occur, which is expected.

Check List

Tests

  • Unit test

@sre-bot
Copy link
Contributor Author

sre-bot commented Apr 8, 2020

/run-all-tests

@eurekaka
Copy link
Contributor

eurekaka commented Apr 8, 2020

@time-and-fate Please help take a look at the CI failure.

@time-and-fate
Copy link
Member

@eurekaka An error is returned by testPlanSuite.ParseOneStmt() on the new added test case. Seems like indicating a syntax error. Then I happened to find that testPlanSuite.Parser.lexer.supportWindowFunc is false. I think this might be the cause.
How can I help to fix it?

@eurekaka
Copy link
Contributor

eurekaka commented Apr 8, 2020

@time-and-fate you can replace ParseOneStmt with ParseSQL in the test, or move the added case to a test which uses ParseSQL

@eurekaka
Copy link
Contributor

eurekaka commented Apr 8, 2020

How does it succeed in master branch BTW?

@time-and-fate
Copy link
Member

@eurekaka supportWindowFunc is switched on in SetUpSuite() in logical_plan_test.go in pr #13406
Should we also add these code here?

s.ctx.GetSessionVars().EnableWindowFunction = true
s.Parser.EnableWindowFunc(true)

@eurekaka
Copy link
Contributor

eurekaka commented Apr 8, 2020

@time-and-fate Yes please.

@winoros winoros removed their request for review May 12, 2020 18:36
@lzmhhh123 lzmhhh123 removed their request for review May 13, 2020 08:22
@SunRunAway
Copy link
Contributor

Cherry-picked in manual, #16210

@SunRunAway SunRunAway closed this May 26, 2020
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 type/compatibility type/3.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants