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

expression: reuse tipb.Expr 'val' to push down expression implicit parameters #10790

Merged
merged 5 commits into from
Jun 21, 2019
Merged

expression: reuse tipb.Expr 'val' to push down expression implicit parameters #10790

merged 5 commits into from
Jun 21, 2019

Conversation

lonng
Copy link
Contributor

@lonng lonng commented Jun 13, 2019

What problem does this PR solve?

Add an implicitParameters method to the builtinFunc interface, and provide a default implementation for baseBuiltinFunc.

The UnionAll executor will infer its children's results type and add CAST function if necessary. The CAST functions in this scenario maybe have some different behaviors. The flag InUnionStmt which is useful to cover some corner case in CAST functions implementation in the coprocessor, so we should pushdown related Flag to the coprocessor.

What is changed and how it works?

This PR overwrite the implicitParameters for baseBuiltinCastFunc to push the inUnion flag.

Check List

Tests

  • Unit test

@lonng lonng changed the title executor: reuse tipb.Expr 'val' field to pass expression flag to coprocessor expression: reuse tipb.Expr 'val' field to pass expression flag to coprocessor Jun 13, 2019
@codecov
Copy link

codecov bot commented Jun 13, 2019

Codecov Report

Merging #10790 into master will decrease coverage by 0.0411%.
The diff coverage is 82.3529%.

@@               Coverage Diff                @@
##             master     #10790        +/-   ##
================================================
- Coverage   80.9963%   80.9551%   -0.0412%     
================================================
  Files           419        419                
  Lines         88825      88759        -66     
================================================
- Hits          71945      71855        -90     
- Misses        11652      11672        +20     
- Partials       5228       5232         +4

@lonng lonng changed the title expression: reuse tipb.Expr 'val' field to pass expression flag to coprocessor expression: reuse tipb.Expr 'val' to push down expression implicit parameters Jun 19, 2019
@zz-jason zz-jason requested review from zz-jason and XuHuaiyu June 19, 2019 02:57
@lonng
Copy link
Contributor Author

lonng commented Jun 20, 2019

@breeswish PTAL

@lonng lonng requested a review from breezewish June 20, 2019 10:09
@breezewish
Copy link
Member

LGTM

@lonng
Copy link
Contributor Author

lonng commented Jun 20, 2019

/run-all-tests

qw4990
qw4990 previously approved these changes Jun 21, 2019
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990
Copy link
Contributor

qw4990 commented Jun 21, 2019

Please fix these CI problems.

Signed-off-by: Lonng <[email protected]>
@@ -244,6 +253,17 @@ type baseBuiltinCastFunc struct {
inUnion bool
}

// implicitArgs returns the implicit arguments of cast functions
func (b *baseBuiltinCastFunc) implicitArgs() []types.Datum {
Copy link
Member

Choose a reason for hiding this comment

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

Why use Datum? Can we change to another data structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Datum is the best structure which is used to exchange implicit arguments.

  1. Codec is easily on both sides.
  2. The encoded bytes are small.
  3. General used in TiDB/TiKV (some implicit arguments do not need any transform)

@lonng
Copy link
Contributor Author

lonng commented Jun 21, 2019

/run-all-tests

@lonng
Copy link
Contributor Author

lonng commented Jun 21, 2019

/run-mybatis-test

Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 21, 2019
@qw4990 qw4990 merged commit 1e40387 into pingcap:master Jun 21, 2019
@lonng lonng deleted the pushdown-cast-flag branch June 21, 2019 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants