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: for UNION statements, ORDER BY cannot use column references including a table name #8389

Merged
merged 5 commits into from Nov 29, 2018

Conversation

Projects
None yet
7 participants
@dbjoa
Contributor

dbjoa commented Nov 22, 2018

What problem does this PR solve?

fix #8196

What is changed and how it works?

  • Remove the source col.TblName when building the columns of Union
  • For UNION or UNION ALL, ORDER BY cannot use column references including a table name (that is, names in tbl_name.col_name format). See: https://dev.mysql.com/doc/refman/5.7/en/union.html
  • These UNION statements should return an error if ORDER BY uses column references with a table name

Check List

Tests

  • Unit test

Code changes

  • Has changed the impl.

Side effects

  • None

Related changes

  • None

This change is Reviewable

@sre-bot

This comment has been minimized.

sre-bot commented Nov 22, 2018

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.

@dbjoa

This comment has been minimized.

Contributor

dbjoa commented Nov 22, 2018

/run-all-tests

@shenli

This comment has been minimized.

Member

shenli commented Nov 22, 2018

@dbjoa Thanks!

@XuHuaiyu

This comment has been minimized.

Contributor

XuHuaiyu commented Nov 22, 2018

Hi, @dbjoa
We'd better using a more detailed title and a commit message to describe the work of a PR and a commit. Fix issue#xx can be put in the PR description. It'll make it easier when checking the commit history and writing the release-note.

@dbjoa dbjoa changed the title from planner: fix the issue #8196 to planner: for UNION statements, ORDER BY cannot use column references including a table name Nov 22, 2018

@dbjoa

This comment has been minimized.

Contributor

dbjoa commented Nov 22, 2018

@XuHuaiyu Thank you for your suggestion. I've updated the title.

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8196 branch from b03f3c2 to 8be96a7 Nov 22, 2018

tk.MustExec("analyze table t1")
tk.MustExec("analyze table t2")
_, err = tk.Exec("(select a,b from t1 limit 2) union all (select a,b from t2 order by a limit 1) order by t1.b")
c.Assert(err, NotNil)

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 22, 2018

Contributor

Please check the error code here.

tk.MustExec("analyze table t1")
tk.MustExec("analyze table t2")
_, err = tk.Exec("(select a,b from t1 limit 2) union all (select a,b from t2 order by a limit 1) order by t1.b")
c.Assert(err.Error(), Equals, "[planner:1054]Unknown column 't1.b' in 'order clause'")

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 22, 2018

Contributor

Is it possible to make this error message be consistent with MySQL?

This comment has been minimized.

@dbjoa

dbjoa Nov 22, 2018

Contributor

In order to do that, we should know the statement type (e.g., SELECT). If we can confirm that UNION is SELECT statement, we can generate the same error like MySQL for this case only, which might be ad hoc. The generalized error message handling might beyond the PR.

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 22, 2018

Contributor

Why do we need to know whether the statement type?
Can OrderByClause.ForUnion help?

This comment has been minimized.

@eurekaka

eurekaka Nov 22, 2018

Contributor

Seems ForUnion is never set now, we may need parser change.

This comment has been minimized.

@dbjoa

dbjoa Nov 22, 2018

Contributor

The updated PR should produce the same error message like MySQL.

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 26, 2018

Contributor

Hi, I do not really understand why don't we use ForUnion? @dbjoa

This comment has been minimized.

@dbjoa

dbjoa Nov 27, 2018

Contributor

@XuHuaiyu We can use ForUnion to indicate OrderByClause used in UNION statement. However, I have concern that to use the indicator the existing methods should be changed on the path of the call stack significantly comparing to the PR. That is, PlanBuilder.buildSort() needs to use ast.OrderByClause as its argument instead of ast.ByItem, and accordingly, expression.expressionRewriter() needs to extend for rewriting ast.OrderByClause.

This comment has been minimized.

@eurekaka

eurekaka Nov 28, 2018

Contributor

Can we use ForUnion and check if table name is specified in Preprocess? We can report error there, so we don't need to modify buildSort and expressionRewriter?

This comment has been minimized.

@dbjoa

dbjoa Nov 28, 2018

Contributor

@eurekaka Yes, we can. However, IMHO, it might not be a simple work as done in the PR because we need to write codes to check all the constraints as done in the existing implementation to produce the same errors.

@dbjoa

This comment has been minimized.

Contributor

dbjoa commented Nov 22, 2018

/run-all-tests

@zz-jason

This comment has been minimized.

Member

zz-jason commented Nov 26, 2018

@XuHuaiyu PTAL

Show resolved Hide resolved planner/core/expression_rewriter.go
tk.MustExec("analyze table t1")
tk.MustExec("analyze table t2")
_, err = tk.Exec("(select a,b from t1 limit 2) union all (select a,b from t2 order by a limit 1) order by t1.b")
c.Assert(err.Error(), Equals, "[planner:1054]Unknown column 't1.b' in 'order clause'")

This comment has been minimized.

@eurekaka

eurekaka Nov 28, 2018

Contributor

Can we use ForUnion and check if table name is specified in Preprocess? We can report error there, so we don't need to modify buildSort and expressionRewriter?

@zz-jason

@dbjoa please merge master and resolve conflicts.

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8196 branch from f1012ed to f483a33 Nov 28, 2018

@dbjoa

This comment has been minimized.

Contributor

dbjoa commented Nov 28, 2018

/run-all-tests

@zz-jason

This comment has been minimized.

Member

zz-jason commented Nov 29, 2018

@eurekaka PTAL

@eurekaka

This comment has been minimized.

Contributor

eurekaka commented Nov 29, 2018

I withhold my opinion about ForUnion approach. This PR has already got 2 LGTMs, you can carry it forward. @zz-jason

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Nov 29, 2018

@zz-jason zz-jason merged commit 3b68ee5 into pingcap:master Nov 29, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@zz-jason

This comment has been minimized.

Member

zz-jason commented Nov 29, 2018

@dbjoa Could you help us cherry-pick this commit to release-2.1 and release-2.0 branch?

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Nov 29, 2018

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Nov 29, 2018

planner: for UNION statements, ORDER BY cannot use column references …
…including a table name (pingcap#8389)

# Conflicts:
#	cmd/explaintest/r/explain_easy.result
#	plan/errors.go
#	plan/expression_rewriter.go
#	plan/logical_plan_builder.go
#	plan/physical_plan_test.go

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Nov 29, 2018

shenli added a commit that referenced this pull request Nov 30, 2018

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