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

executor: fix a bug for 'int column <cmp> non-int constant' #11050

Merged
merged 12 commits into from
Jul 11, 2019

Conversation

wshwsh12
Copy link
Contributor

@wshwsh12 wshwsh12 commented Jul 3, 2019

What problem does this PR solve?

#10675
Wrong result for int column <cmp> non-int constant when the constant is inf or -inf

CREATE TABLE `t` (
  `a` int(11) DEFAULT NULL
);
insert into t values(1);
select * from t where a < -184467440737095516167.1;

Before this pr:

select * from t where a < -184467440737095516167.1;
+------+
| a    |
+------+
|    1 |
+------+

This pr:

select * from t where a < -184467440737095516167.1;
Empty set (0.00 sec)

What is changed and how it works?

When the compare expression is int column <cmp> non-int constant or non-int constant <cmp> int column, TiDB will refine the constant to the int column type.
E.g., a < 1.1 will be rewritten to a < 2.

Before this pr, the logic of refine function is wrong. It is only judged whether the constant is Overflow.
a < -184467440737095516167.1 will be rewritten to a < inf
a > 184467440737095516167.1 will be rewritten to a > -inf

This pr, I use the leading sing and Overflow flag to decide the constant is 'inf' or '-inf'
a < -184467440737095516167.1 will be rewritten to a < -inf
a > 184467440737095516167.1 will be rewritten to a > inf

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

Side effects

Related changes

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Jul 3, 2019

/run-all-tests

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@47f449a). Click here to learn what that means.
The diff coverage is 82%.

@@            Coverage Diff             @@
##             master    #11050   +/-   ##
==========================================
  Coverage          ?   81.259%           
==========================================
  Files             ?       423           
  Lines             ?     90006           
  Branches          ?         0           
==========================================
  Hits              ?     73138           
  Misses            ?     11569           
  Partials          ?      5299

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Jul 3, 2019

/run-all-tests

dt, err := con.Eval(chunk.Row{})
if err != nil {
return con, false
return con, nil, false
}
sc := ctx.GetSessionVars().StmtCtx
intFieldType := types.NewFieldType(mysql.TypeLonglong)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can define this intFieldType as an input parameter. It should be the FiledType of the int column.

tidb> create table t (a int);
Query OK, 0 rows affected (0.01 sec)

tidb> desc select * from t where a > 128; -- It should be TableDual for the sql as well.
+---------------------+----------+------+------------------------------------------------------------+
| id                  | count    | task | operator info                                              |
+---------------------+----------+------+------------------------------------------------------------+
| TableReader_7       | 3333.33  | root | data:Selection_6                                           |
| └─Selection_6       | 3333.33  | cop  | gt(test.t.a, 128)                                          |
|   └─TableScan_5     | 10000.00 | cop  | table:t, range:[-inf,+inf], keep order:false, stats:pseudo |
+---------------------+----------+------+------------------------------------------------------------+
3 rows in set (0.00 sec)

tidb> desc select * from t where a > 11111111111111111111111111111;
+-------------+-------+------+---------------+
| id          | count | task | operator info |
+-------------+-------+------+---------------+
| TableDual_6 | 0.00  | root | rows:0        |
+-------------+-------+------+---------------+
1 row in set (0.00 sec)

@qw4990
Copy link
Contributor

qw4990 commented Jul 5, 2019

Could you please describe more details about the root reason causing this bug.
Then it's more convenient for reviewers to review your changes. @wshwsh12

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Jul 8, 2019

/run-all-tests

1 similar comment
@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Jul 8, 2019

/run-all-tests

expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
@wshwsh12 wshwsh12 force-pushed the fix_plan branch 3 times, most recently from 846eb97 to 385f5c8 Compare July 9, 2019 07:04
@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Jul 9, 2019

/run-all-tests

@wshwsh12 wshwsh12 requested a review from XuHuaiyu July 9, 2019 07:45
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 Jul 11, 2019

/run-all-tests

@XuHuaiyu
Copy link
Contributor

/run-unit-test

@wshwsh12
Copy link
Contributor Author

/rebuild

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants