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
planner: refine range builder when meet unsigned_int_col <cmp> -int_cnst
#10471
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10471 +/- ##
================================================
- Coverage 78.1703% 78.1639% -0.0065%
================================================
Files 413 413
Lines 87514 87566 +52
================================================
+ Hits 68410 68445 +35
- Misses 13961 13975 +14
- Partials 5143 5146 +3 |
util/ranger/ranger_test.go
Outdated
@@ -660,6 +660,13 @@ func (s *testRangerSuite) TestIndexRangeForUnsignedInt(c *C) { | |||
filterConds: "[]", | |||
resultStr: `[(NULL,1) (2,9223372036854775810) (9223372036854775810,+inf]]`, | |||
}, | |||
{ | |||
indexPos: 0, | |||
exprStr: `a >= -2`, |
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.
If a == -2
, we'll get [eq(test.t.a), 0]
?
Does it seem to be wrong?
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.
no
If a == -2, we'll get [eq(test.t.a), -2]
convertPoint function only affect ranges
@b41sh Could you add some comments and test cases? |
util/ranger/points.go
Outdated
value.Kind() == types.KindMysqlDecimal { | ||
needFix = true | ||
zeroValue.SetMysqlDecimal(new(types.MyDecimal)) | ||
} |
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.
should we also consider float or double values?
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 don't need consider float and decimal, only unsigned integer will convert a negative number to positive number in function func (d *Datum) convertToUint(sc *stmtctx.StatementContext, target *FieldType) (Datum, error)
It'll be better to add the integration tests mentioned in #10450. |
util/ranger/points.go
Outdated
@@ -250,6 +250,28 @@ func (r *builder) buildFormBinOp(expr *expression.ScalarFunction) []point { | |||
return nil | |||
} | |||
|
|||
// if the colunm is unsigned integer and the value | |||
// is less than zero, we only need compare with zero. | |||
if mysql.HasUnsignedFlag(ft.Flag) { |
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 extract the part as a function like handleUnsignedIntCol
?
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.
ok
PTAL @winoros |
…into fix-unsigned_int_col
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.
rest part is ok for me
util/ranger/points.go
Outdated
@@ -250,6 +250,11 @@ func (r *builder) buildFormBinOp(expr *expression.ScalarFunction) []point { | |||
return nil | |||
} | |||
|
|||
value, op, isValidRange := handleUnsignedIntCol(ft, value, op) | |||
if isValidRange == false { |
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.
if !isValidRange
/run-all-tests |
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
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
Thanks for you contribution!
What problem does this PR solve?
fix unexpected result of
unsigned_int_col >= -int_cnst
#10450What is changed and how it works?
convert negative value to zero for unsigned column
Check List
Tests