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
Merged

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

merged 11 commits into from
Dec 3, 2018

Conversation

dbjoa
Copy link
Contributor

@dbjoa 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
Copy link
Contributor

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
Copy link
Member

zz-jason commented Nov 6, 2018

@winoros @eurekaka PTAL

@zz-jason zz-jason added contribution This PR is from a community contributor. type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Nov 6, 2018
@zz-jason
Copy link
Member

zz-jason commented Nov 6, 2018

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

@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 6, 2018

@zz-jason Yes, I would.

@dbjoa
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
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 ?

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 Show resolved Hide resolved
@dbjoa dbjoa changed the title executor,planner: fix the issue #8153 [WIP] executor,planner: fix the issue #8153 Nov 6, 2018
@tiancaiamao
Copy link
Contributor

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
Copy link
Contributor Author

dbjoa commented Nov 7, 2018

/run-all-tests

@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 7, 2018

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

@eurekaka
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
Copy link
Contributor Author

dbjoa commented Nov 8, 2018

/run-all-tests

2 similar comments
@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 8, 2018

/run-all-tests

@shenli
Copy link
Member

shenli commented Nov 8, 2018

/run-all-tests

@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
@zz-jason
Copy link
Member

@dbjoa Could you update go.mod to use the latest tidb parser?

@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 15, 2018

@zz-jason I've updated the PR to use the latest parser.

@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 15, 2018

/run-all-tests

1 similar comment
@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 15, 2018

/run-all-tests

planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 15, 2018

/run-all-tests

@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 30, 2018

I've updated PR to remove InPrepare flag by setting the default parameter values as NULL.
@eurekaka @zz-jason PTAL

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.

LGTM

@eurekaka eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. 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
}
value, err := GetParamExpression(ctx, v.P.(*driver.ParamMarkerExpr))
if err != nil {
return 0, true, errors.Trace(err)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@crazycs520
Copy link
Contributor

/run-all-tests

@dbjoa
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 {
Copy link
Member

Choose a reason for hiding this comment

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

how about:

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

Copy link
Contributor Author

@dbjoa dbjoa Dec 3, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

[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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 3, 2018
@zz-jason zz-jason merged commit c677187 into pingcap:master Dec 3, 2018
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Dec 13, 2018
zz-jason pushed a commit to zz-jason/tidb that referenced this pull request Oct 3, 2019
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 status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants