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: rewrite `in` behavior. #4813

Merged
merged 17 commits into from Oct 28, 2017

Conversation

@winoros
Member

winoros commented Oct 18, 2017

Decide whether one CNF clause can be used to calculate range isn't easy as decide an in function.
So we will make an in expr to an in function if we can.

NOTE: Since in function haven't been pushed down to tikv. Some unit test failed. I'll add it in coprocessor first.

in function now have been pushed down to tikv.

PTAL @XuHuaiyu @zz-jason @coocood

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Oct 18, 2017

Member

Will this PR affect the tidb_opt_insubquery_unfold system variable?

// tidb_opt_insubquery_unfold is used to enable/disable the optimizer rule of in subquery unfold.
TiDBOptInSubqUnFolding = "tidb_opt_insubquery_unfold"

Member

shenli commented Oct 18, 2017

Will this PR affect the tidb_opt_insubquery_unfold system variable?

// tidb_opt_insubquery_unfold is used to enable/disable the optimizer rule of in subquery unfold.
TiDBOptInSubqUnFolding = "tidb_opt_insubquery_unfold"

@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Oct 18, 2017

Member

@shenli No, that case is handled when building subquery

Member

winoros commented Oct 18, 2017

@shenli No, that case is handled when building subquery

@shenli shenli added the status/DNM label Oct 19, 2017

@hanfei1991 hanfei1991 added this to expression in Planner Oct 19, 2017

@hanfei1991 hanfei1991 added this to the 1.1 milestone Oct 19, 2017

@hanfei1991 hanfei1991 self-assigned this Oct 19, 2017

@hanfei1991 hanfei1991 self-requested a review Oct 19, 2017

@hanfei1991 hanfei1991 removed their assignment Oct 19, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 23, 2017

Member

@winoros plz fix ci

Member

zz-jason commented Oct 23, 2017

@winoros plz fix ci

@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Oct 23, 2017

Member

@zz-jason Test will be fixed after in is pushed down to tikv.

Member

winoros commented Oct 23, 2017

@zz-jason Test will be fixed after in is pushed down to tikv.

@@ -231,8 +230,8 @@ func containsAlphabet(s string) bool {
}
func (e *Evaluator) evalIn(expr *tipb.Expr) (types.Datum, error) {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Oct 25, 2017

Contributor

Can the code in distsql/ xeval be removed now?

@XuHuaiyu

XuHuaiyu Oct 25, 2017

Contributor

Can the code in distsql/ xeval be removed now?

This comment has been minimized.

@winoros

winoros Oct 27, 2017

Member

No, there're tests that rely on this.

@winoros

winoros Oct 27, 2017

Member

No, there're tests that rely on this.

Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go

@winoros winoros removed the status/DNM label Oct 26, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 26, 2017

Member

@winoros any update ?

Member

zz-jason commented Oct 26, 2017

@winoros any update ?

winoros added some commits Oct 27, 2017

@ngaut

This comment has been minimized.

Show comment
Hide comment
@ngaut

ngaut Oct 27, 2017

Member

PTAL

Member

ngaut commented Oct 27, 2017

PTAL

winoros added some commits Oct 27, 2017

Revert "undo some change."
This reverts commit bea75d8.
@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Oct 27, 2017

Member

/run-all-test

Member

winoros commented Oct 27, 2017

/run-all-test

@winoros

This comment has been minimized.

Show comment
Hide comment
@winoros

winoros Oct 27, 2017

Member

/run-sqllogic-test

Member

winoros commented Oct 27, 2017

/run-sqllogic-test

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Oct 27, 2017

Contributor

/run-common-test

Contributor

XuHuaiyu commented Oct 27, 2017

/run-common-test

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli
Member

shenli commented Oct 27, 2017

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli
Member

shenli commented Oct 27, 2017

@coocood PTAL

Show outdated Hide outdated distsql/xeval/eval_compare_ops.go
Show outdated Hide outdated expression/builtin.go
baseBuiltinFunc
}
func (b *builtinInIntSig) evalInt(row types.Row) (int64, bool, error) {

This comment has been minimized.

@zz-jason
@zz-jason

This comment has been minimized.

@winoros

winoros Oct 28, 2017

Member

added at the head of each sig.

@winoros

winoros Oct 28, 2017

Member

added at the head of each sig.

Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
for _, tc := range testCases {
fn, err := fc.getFunction(s.ctx, s.datumsToConstants(types.MakeDatums(tc.args...)))
c.Assert(err, IsNil)
d, err := evalBuiltinFunc(fn, types.DatumRow(types.MakeDatums(tc.args...)))

This comment has been minimized.

@zz-jason

zz-jason Oct 27, 2017

Member

We'd better not to call evalBuiltinFunc to write unit tests, you can take a look at TestArithmeticPlus() in file expression/builtin_arithmetic_test.go as an example.

@zz-jason

zz-jason Oct 27, 2017

Member

We'd better not to call evalBuiltinFunc to write unit tests, you can take a look at TestArithmeticPlus() in file expression/builtin_arithmetic_test.go as an example.

This comment has been minimized.

@winoros

winoros Oct 28, 2017

Member

in builtin_arithmtic_test.go, TestArithmeticMod/Divide/Multiply and so on have all used evalBuiltinFunc.

@winoros

winoros Oct 28, 2017

Member

in builtin_arithmtic_test.go, TestArithmeticMod/Divide/Multiply and so on have all used evalBuiltinFunc.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 28, 2017

Member

ut failed:

FAIL: eval_test.go:417: testEvalSuite.TestWhereIn
eval_test.go:458:
    c.Check(err, IsNil)
... value *terror.Error = &terror.Error{class:19, code:3, message:"IN needs more than 1 operand, got %d", args:[]interface {}{1}, file:"github.com/pingcap/tidb/distsql/xeval/_test/_obj_test/eval_compare_ops.go", line:327} ("[19:3]IN needs more than 1 operand, got 1")
eval_test.go:462:
    c.Check(res.Kind(), Equals, types.KindInt64)
... obtained uint8 = 0x0
... expected uint8 = 0x1
Member

zz-jason commented Oct 28, 2017

ut failed:

FAIL: eval_test.go:417: testEvalSuite.TestWhereIn
eval_test.go:458:
    c.Check(err, IsNil)
... value *terror.Error = &terror.Error{class:19, code:3, message:"IN needs more than 1 operand, got %d", args:[]interface {}{1}, file:"github.com/pingcap/tidb/distsql/xeval/_test/_obj_test/eval_compare_ops.go", line:327} ("[19:3]IN needs more than 1 operand, got 1")
eval_test.go:462:
    c.Check(res.Kind(), Equals, types.KindInt64)
... obtained uint8 = 0x0
... expected uint8 = 0x1
@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 28, 2017

Member

LGTM

Member

zz-jason commented Oct 28, 2017

LGTM

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Oct 28, 2017

Member

/run-all-test

Member

coocood commented Oct 28, 2017

/run-all-test

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Oct 28, 2017

Member

/run-all-tests

Member

shenli commented Oct 28, 2017

/run-all-tests

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Oct 28, 2017

Member

LGTM

Member

coocood commented Oct 28, 2017

LGTM

@coocood coocood merged commit c4f2930 into pingcap:master Oct 28, 2017

12 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 72.375%
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

winoros added a commit to winoros/tidb that referenced this pull request Oct 28, 2017

@winoros winoros deleted the winoros:yiding/in branch Oct 30, 2017

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