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

plan: support `?` in Order By / Group By / Limit Offset clauses #8206

Merged
merged 11 commits into from Dec 3, 2018

Conversation

Projects
None yet
8 participants
@dbjoa
Copy link
Contributor

dbjoa commented Nov 6, 2018

What problem does this PR solve?

Fix #8153

What is changed and how it works?

  • Modify parser.ByItem to support ast.ParamMarkerExpr
  • The prepared statements having parameters in OrderByClause or GroupByClause aren't cached

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
  • Has persistent data change (i.e., parser)

Side effects

  • No

Related changes

  • Need to update the parser repository (i.e., the parser PR #27)

This change is Reviewable

@sre-bot

This comment has been minimized.

Copy link

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

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Nov 6, 2018

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Nov 6, 2018

@dbjoa Would you file another PR to make this order by statement cacheable?

@dbjoa

This comment has been minimized.

Copy link
Contributor Author

dbjoa commented Nov 6, 2018

@zz-jason Yes, I would.

@dbjoa

This comment has been minimized.

Copy link
Contributor Author

dbjoa commented Nov 6, 2018

@zz-jason How can we run the CI test (i.e., run-all-tests) based on the parser PR #20 ?

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Nov 6, 2018

@iamxy Could you help us with this?

How can we run the CI test (i.e., run-all-tests) based on the parser PR pingcap/parser#20 ?

Show resolved Hide resolved planner/core/logical_plan_builder.go
Show resolved Hide resolved planner/core/expression_rewriter.go Outdated
Show resolved Hide resolved planner/core/expression_rewriter.go Outdated

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8153 branch from 4813104 to 364ef09 Nov 6, 2018

@dbjoa dbjoa changed the title executor,planner: fix the issue #8153 [WIP] executor,planner: fix the issue #8153 Nov 6, 2018

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

tiancaiamao commented Nov 7, 2018

Hi, @dbjoa
pingcap/parser#20 broke the CI
The root problem is that, update TiDB and parser should be together, we didn't do it well during the migration to Go module.

Here I add some instructions on how to update parser for TiDB:
pingcap/parser#23

You can follow that.
Any suggestions are welcome, thanks.

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8153 branch from 364ef09 to 379c02b Nov 7, 2018

@dbjoa

This comment has been minimized.

Copy link
Contributor Author

dbjoa commented Nov 7, 2018

/run-all-tests

@dbjoa

This comment has been minimized.

Copy link
Contributor Author

dbjoa commented Nov 7, 2018

@eurekaka
Would you let me know the schema and queries of the failed CI tests?

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8153 branch 2 times, most recently from 2905fa3 to 8bc935d Nov 7, 2018

@eurekaka

This comment has been minimized.

Copy link
Contributor

eurekaka commented Nov 8, 2018

@dbjoa the log indicates it is the newly added case that fails:

FAIL: prepared_test.go:571: testSuite.TestPreparedIssue8153

prepared_test.go:602:
    c.Assert(err.Error(), Equals, "[planner:1054]Unknown column '?' in 'order clause'")
... obtained string = "[planner:1054]Unknown column '3' in 'order clause'"
... expected string = "[planner:1054]Unknown column '?' in 'order clause'"

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8153 branch from 8bc935d to 4a49a79 Nov 8, 2018

@dbjoa

This comment has been minimized.

Copy link
Contributor Author

dbjoa commented Nov 8, 2018

/run-all-tests

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8153 branch from 4a49a79 to f1153e8 Nov 8, 2018

@dbjoa

This comment has been minimized.

Copy link
Contributor Author

dbjoa commented Nov 8, 2018

/run-all-tests

1 similar comment
@shenli

This comment has been minimized.

Copy link
Member

shenli commented Nov 8, 2018

/run-all-tests

@dbjoa dbjoa referenced this pull request Nov 8, 2018

Merged

ast: fix the TiDB issue #8153 #27

@dbjoa dbjoa changed the title [WIP] executor,planner: fix the issue #8153 executor,planner: fix the issue #8153 Nov 8, 2018

@XuHuaiyu XuHuaiyu changed the title executor,planner: fix the issue #8153 executor,planner: make `order by ?` performs correctly when prepare-cache is enabled Nov 12, 2018

@eurekaka
Copy link
Contributor

eurekaka left a comment

LGTM

@eurekaka eurekaka added the status/LGT1 label Dec 3, 2018

@eurekaka eurekaka changed the title executor,planner: make `order by ?` performs correctly when prepare-cache is enabled plan: support `Order By ?` | `Group By ?` | `Limit ? Offset ?` Dec 3, 2018

@eurekaka eurekaka changed the title plan: support `Order By ?` | `Group By ?` | `Limit ? Offset ?` plan: support `ParamMarkerExpr` in Order By / Group By / Limit Offset clauses Dec 3, 2018

@eurekaka eurekaka changed the title plan: support `ParamMarkerExpr` in Order By / Group By / Limit Offset clauses plan: support `?` in Order By / Group By / Limit Offset clauses Dec 3, 2018

Show resolved Hide resolved expression/util.go
}
value, err := GetParamExpression(ctx, v.P.(*driver.ParamMarkerExpr))
if err != nil {
return 0, true, errors.Trace(err)

This comment has been minimized.

Copy link
@winoros

winoros Dec 3, 2018

Member

Once it's traced, we don't need to trace it anymore.
So here we can just return err.

This comment has been minimized.

Copy link
@dbjoa

dbjoa Dec 3, 2018

Author Contributor

Thank you for your detailed comments. The updated PR will address the issue.

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8153 branch from 497fa92 to 96a31d8 Dec 3, 2018

@crazycs520

This comment has been minimized.

Copy link
Contributor

crazycs520 commented Dec 3, 2018

/run-all-tests

@dbjoa

This comment has been minimized.

Copy link
Contributor Author

dbjoa commented Dec 3, 2018

/run-integration-ddl-test

@@ -526,3 +527,55 @@ func GetParamExpression(ctx sessionctx.Context, v *driver.ParamMarkerExpr, useCa
}
return value, nil
}

// ConvertToByItemExpr rewrites ByItem.ExprNode to a proper ExprNode.
func ConvertToByItemExpr(n ast.Node) ast.Node {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Dec 3, 2018

Member

how about:

func ConstructPositionExpr(v *driver.ParamMarkerExpr) *ast.PositionExpr {
	return &ast.PositionExpr{P: v}
}

This comment has been minimized.

Copy link
@dbjoa

dbjoa Dec 3, 2018

Author Contributor

In order to do that, we must add the checking logic of driver.ParamMarkerExpr typed node before calling ConstructPositionExpr. Hence, IMHO, the current impl. might be simpler than your suggestions.

This comment has been minimized.

Copy link
@zz-jason

zz-jason Dec 3, 2018

Member
[jianzhang.zj:~/opt/gopath/src/github.com/pingcap/tidb] git:(cloud-pi/fix-the-issue-8153 ✔)
➜ ag -w ConvertToByItemExpr
planner/core/logical_plan_builder.go
827:            newNode := expression.ConvertToByItemExpr(inNode)
1207:           newNode := expression.ConvertToByItemExpr(inNode)

expression/util.go
531:// ConvertToByItemExpr rewrites ByItem.ExprNode to a proper ExprNode.
532:func ConvertToByItemExpr(n ast.Node) ast.Node {

These are all the appearance of ConvertToByItemExpr, and as you can see in the function caller, the input parameter is guaranteed to be driver.ParamMarkerExpr

This comment has been minimized.

Copy link
@dbjoa

dbjoa Dec 3, 2018

Author Contributor

Got it!

@dbjoa dbjoa force-pushed the cloud-pi:fix-the-issue-8153 branch from 96a31d8 to 7ac5db0 Dec 3, 2018

@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Dec 3, 2018

@zz-jason zz-jason merged commit c677187 into pingcap:master Dec 3, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.