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: the order by with default field not parsed (#11210) #11668

Closed
wants to merge 1 commit into from
Closed

planner/core: the order by with default field not parsed (#11210) #11668

wants to merge 1 commit into from

Conversation

gaoxingliang
Copy link
Contributor

What problem does this PR solve?

Fix #11210
when order by with default(colName) is not resolved and got unknown column

What is changed and how it works?

parse the column name expr in the order by resolver.

Check List

Tests

  • Unit test -- added and passed

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

sre-bot commented Aug 7, 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.

@gaoxingliang
Copy link
Contributor Author

Need more time to fix UTs

@alivxxx alivxxx removed their request for review August 8, 2019 08:37
@zz-jason
Copy link
Member

@gaoxingliang friendly ping, any update?

@gaoxingliang
Copy link
Contributor Author

@gaoxingliang friendly ping, any update?

the progress is not good. I think I missed something when I try to parse the ColumnName so many cases need to be considered (similar with the ColumnNameExpr, but I don't understand the difference). I think I need to close this PR temp now and do more checks.

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

Can we check if the column has default value definition in expression rewriting of buildSort and report error (if no definition) or replace the ByItem to a constant?

@zz-jason
Copy link
Member

@gaoxingliang friendly ping, any update?

@gaoxingliang
Copy link
Contributor Author

gaoxingliang commented Sep 23, 2019

Can we check if the column has default value definition in expression rewriting of buildSort and report error (if no definition) or replace the ByItem to a constant?

Hi @eurekaka , it will not working. I test and found the schema information is not correct when we calling the plan.schema in buildSort and try to remapped in the itemTransformer

@eurekaka
Copy link
Contributor

@gaoxingliang Sorry I didn't get your point. IMHO, theoretically, we can make the transformation when resolving the ORDER BY clause.

@gaoxingliang
Copy link
Contributor Author

Hi @eurekaka below is more details about this issue.
the problem occur when the default(x) is not added into the plan p.Schema() at logical_plan_builder.go#buildSort
So I try to find where the schema select fields are added with comparison with another func like length(x)
I found it's added in the havingWindowAndOrderbyExprResolver with case ColumnNameExpr at here for length function.
but for default function, it's a ColumnName instead of a ColumnNameExpr which I think this is what parser done. I don't get the difference.

for your suggestion, the [transformer] (https://github.com/pingcap/tidb/blob/release-3.0/planner/core/logical_plan_builder.go#L2132) happened after the error occur. so I don't think that will fix this problem.

@zz-jason zz-jason requested review from eurekaka and removed request for eurekaka December 28, 2019 13:39
@winoros winoros requested review from eurekaka and winoros March 5, 2020 12:04
@zz-jason
Copy link
Member

Hi @gaoxingliang, friendly ping, any update?

@sre-bot
Copy link
Contributor

sre-bot commented Mar 15, 2020

@gaoxingliang, please update your pull request.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Mar 22, 2020

@gaoxingliang, please update your pull request.

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

@gaoxingliang PR closed due to no update for a long time. Feel free to reopen it anytime.

@sre-bot sre-bot closed this Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. sig/planner SIG: Planner type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ErrBadField occurs when using Function DEFAULT with ORDER BY clause
4 participants