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: fix the issue that incorrect result for query that uses an AND operator on floats #15927

Merged
merged 11 commits into from
Apr 21, 2020

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Mar 31, 2020

What problem does this PR solve?

Issue Number: close #15743, close #16426

What is changed and how it works?

Don't round the value when converting it to bool.

Check List

Tests

  • Unit test

@qw4990 qw4990 requested a review from a team as a code owner March 31, 2020 13:22
@ghost ghost requested review from SunRunAway and removed request for a team March 31, 2020 13:22
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

zz-jason commented Mar 31, 2020

Please fix UT:

datum_test.go:66: testDatumSuite.TestToBool
[2020-03-31T13:24:24.756Z] FAIL: datum_test.go:66: testDatumSuite.TestToBool

[2020-03-31T13:24:24.756Z] 

[2020-03-31T13:24:24.756Z] datum_test.go:70:

[2020-03-31T13:24:24.756Z]     testDatumToBool(c, float32(0.1), 0)

[2020-03-31T13:24:24.756Z] datum_test.go:63:

[2020-03-31T13:24:24.756Z]     c.Assert(b, Equals, res64)

[2020-03-31T13:24:24.756Z] ... obtained int64 = 1

[2020-03-31T13:24:24.756Z] ... expected int64 = 0

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #15927 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #15927   +/-   ##
===========================================
  Coverage   80.4895%   80.4895%           
===========================================
  Files           506        506           
  Lines        137429     137429           
===========================================
  Hits         110616     110616           
  Misses        18221      18221           
  Partials       8592       8592           

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 1, 2020
@qw4990
Copy link
Contributor Author

qw4990 commented Apr 1, 2020

/run-all-tests

XuHuaiyu
XuHuaiyu previously approved these changes Apr 1, 2020
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu XuHuaiyu added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 1, 2020
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Apr 1, 2020

/merge

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 1, 2020

/run-all-tests

@ngaut ngaut removed the status/LGT1 Indicates that a PR has LGTM 1. label Apr 1, 2020
@qw4990
Copy link
Contributor Author

qw4990 commented Apr 2, 2020

[2020-04-01T11:53:55.438Z] Statement: #839 -  SELECT ATAN( `col_varbinary_32`, ( ROUND( `col_year_key` ) ) ) AS field1, ABS( 8188107072512983040 ) AS field2, - 0 AS field3 FROM `table10_int_autoinc` WHERE LOG( `col_varchar_64_key`, '2033-08-25 07:33:37.038938' ) /* QNO 841 CON_ID 184 */ 
[2020-04-01T11:53:55.438Z] NoPushDown Output: 
[2020-04-01T11:53:55.438Z] field1	field2	field3
[2020-04-01T11:53:55.438Z] 0	8188107072512983040	0
[2020-04-01T11:53:55.438Z] 0	8188107072512983040	0
[2020-04-01T11:53:55.438Z] 0	8188107072512983040	0
[2020-04-01T11:53:55.438Z] 
[2020-04-01T11:53:55.438Z] 
[2020-04-01T11:53:55.438Z] WithPushDown Output: 
[2020-04-01T11:53:55.438Z] field1	field2	fie
[2020-04-01T11:53:55.438Z] 0	8188107072512983040	0

It seems that the coprocessor also has this problem, could you help me to fix it? @zhongzc

@zhongzc
Copy link
Contributor

zhongzc commented Apr 2, 2020

@qw4990 ok

@zhongzc
Copy link
Contributor

zhongzc commented Apr 2, 2020

@qw4990 Seems there're still issues in TiDB.
In MySQL 5.7:

mysql root@127.0.0.1:test> SELECT ATAN( `col_varbinary_32`, ( ROUND( `col_year_key` ) ) ) AS field1, ABS( 8188107072512983040 ) AS field2, - 0 AS field3 FROM `table
                           10_int_autoinc` WHERE LOG( `col_varchar_64_key`, '2033-08-25 07:33:37.038938' ) /* QNO 841 CON_ID 184 */
+--------+---------------------+--------+
| field1 | field2              | field3 |
+--------+---------------------+--------+
| 0.0    | 8188107072512983040 | 0      |
+--------+---------------------+--------+

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 20, 2020

Wait for tikv/tikv#7342.

Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

Should mocktikv be updated?

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 20, 2020

/run-integration-copr-test tikv=pr/7342

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 20, 2020

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 20, 2020

/run-integration-copr-test tikv=pr/7342

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 21, 2020

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 21, 2020

/run-integration-copr-test tikv=pr/7342

@SunRunAway SunRunAway merged commit 94011e6 into pingcap:master Apr 21, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 21, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

cherry pick to release-2.1 in PR #16663

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 21, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

cherry pick to release-3.0 in PR #16664

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 21, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

cherry pick to release-3.1 in PR #16665

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 21, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

cherry pick to release-4.0 in PR #16666

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug.
Projects
None yet
7 participants