-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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: make in more powerful. #4969
Conversation
for i := stkLen - lLen; i < stkLen; i++ { | ||
if er.ctxStack[i].GetType().Tp != mysql.TypeNull && er.ctxStack[i].GetType().EvalType() != leftEt { | ||
for _, arg := range args[1:] { | ||
if arg.GetType().Tp != mysql.TypeNull && expression.GetAccurateCmpType(args[0], arg) != leftEt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we collect all the argument types and merge them to one type, then just cast all the expressions to this type and always use in
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can group them to the form x in(type_x_expr) or cast(x to type_a) in(type_a_expr)or...
.But cannot to convert to one type. e.g. x in ('a', 1.1)
. x's type is string type. We should compare x
and a
in string type, x
and 1.1
in double type. But we can't compare x
and a
in double type since this will make result absolutely wrong. And we can't compare x
and 1.1
in string type since this don't obey the compare system in MySQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about build DNF with different types?
(intValLeft in (intA, intB, intC)) or (floatValLeft in (floatA, floatB))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But currently this is not very necessary to do this. This case can't push down to tikv, and can't be used to calculate range.
LGTM |
util/ranger/range_test.go
Outdated
@@ -161,10 +161,12 @@ func (s *testRangerSuite) TestTableRange(c *C) { | |||
resultStr: "[(-inf,-1] [1,+inf)]", | |||
}, | |||
// TODO: cast will change the extraction behavior of accessCondition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this comment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
/run-unit-test /run-integration-ddl-test |
mysql-test needs to do some change. Don't merge for a while. |
/run-common-test tidb-test=pr/396 |
Let
in
can handle more case, so we can make more expression able to calculate range.PTAL @XuHuaiyu @zz-jason @hanfei1991