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.

Sign up for GitHub

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

Jump to bottom

expression : fix function 'binSearch' in builtinIntervalRealSig not return error #12957

Merged
merged 9 commits into from
Nov 27, 2019
Merged

expression : fix function 'binSearch' in builtinIntervalRealSig not return error #12957

merged 9 commits into from
Nov 27, 2019

Conversation

gtygo
Copy link
Contributor

@gtygo gtygo commented Oct 28, 2019

What problem does this PR solve?

When I implemented the vectorization interface, I found a tiny error in the builtin_compare.go file.
So I fixed it

What is changed and how it works?

In the binsreach function, when we call EvalReal, the return value is err1, but when we judge the error, we check err, this err is always empty, which causes us to not capture EvalReal The error returned.

Check List

Tests

  • Unit test

@gtygo gtygo requested a review from a team as a code owner October 28, 2019 04:10
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Oct 28, 2019
@ghost ghost requested review from XuHuaiyu and removed request for a team October 28, 2019 04:10
@francis0407
Copy link
Member

Good catch!

@francis0407
Copy link
Member

/run-unit-test

@codecov
Copy link

codecov bot commented Oct 28, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #12957   +/-   ##
===========================================
  Coverage   80.0644%   80.0644%           
===========================================
  Files           474        474           
  Lines        116746     116746           
===========================================
  Hits          93472      93472           
  Misses        15887      15887           
  Partials       7387       7387

francis0407
francis0407 reviewed Oct 28, 2019
View reviewed changes
Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

LGTM

@francis0407 francis0407 added component/expression status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug. labels Oct 28, 2019
@francis0407
Copy link
Member

Shall we cherry-pick this to the release branch? @XuHuaiyu

@XuHuaiyu
Copy link
Contributor

@francis0407 We need to cherry-pick this commit to 2.1, 3.0, 3.1

@gtygo Is it possible to build a test case for this modification?

@XuHuaiyu XuHuaiyu removed their request for review October 28, 2019 06:17
@gtygo gtygo requested a review from francis0407 October 30, 2019 06:48
@XuHuaiyu
Copy link
Contributor

@gtygo args[1] will be promised to be of type Column ?
Will it be a ScalarFunction?

@francis0407
Copy link
Member

It can be other expressions, such as builtinArithmeticMultiplyRealSig which may generate a overflow error. You can make a case that trigger the overflow.

@gtygo
Copy link
Contributor Author

gtygo commented Nov 11, 2019

@XuHuaiyu Let you wait a long time , i added a group of unit tests, please give comments.

@zz-jason zz-jason requested a review from XuHuaiyu November 13, 2019 02:22
XuHuaiyu
XuHuaiyu reviewed Nov 14, 2019
View reviewed changes
expression/builtin_compare_test.go Show resolved Hide resolved
expression/builtin_compare_test.go
{types.MakeDatums(uint64(9223372036854775806), 9223372036854775807), 0, false},
{types.MakeDatums(uint64(9223372036854775806), -9223372036854775807), 1, false},
{types.MakeDatums("9007199254740991", "9007199254740992"), 0, false},
{types.MakeDatums(1, uint32(1), uint32(1)), 0, true},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why will this return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This testcase will cover (SF * scalarfunction) evalreal , and it will return an error: cannot convert 1 (type uint32) to string

@gtygo gtygo requested a review from XuHuaiyu November 15, 2019 04:22
@gtygo
Copy link
Contributor Author

gtygo commented Nov 25, 2019

@XuHuaiyu please review

@francis0407 francis0407 removed their request for review November 25, 2019 09:54
XuHuaiyu
XuHuaiyu approved these changes Nov 27, 2019
View reviewed changes
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 status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 27, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

@gtygo merge failed.

@gtygo
Copy link
Contributor Author

gtygo commented Nov 27, 2019

/run-all-tests

@gtygo
Copy link
Contributor Author

gtygo commented Nov 27, 2019

/run-unit-test

@gtygo
Copy link
Contributor Author

gtygo commented Nov 27, 2019

/run-auto-merge

@francis0407
Copy link
Member

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

/run-all-tests

@sre-bot sre-bot merged commit f996816 into pingcap:master Nov 27, 2019
@sre-bot sre-bot mentioned this pull request Nov 27, 2019
Closed
@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

cherry pick to release-3.1 in PR #13766

@sre-bot sre-bot mentioned this pull request Nov 27, 2019
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

cherry pick to release-3.0 in PR #13767

@sre-bot sre-bot mentioned this pull request Nov 27, 2019
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Nov 27, 2019

cherry pick to release-2.1 in PR #13768

@gtygo gtygo deleted the myfeature branch November 27, 2019 06:06
qw4990 pushed a commit to qw4990/tidb that referenced this pull request Nov 27, 2019
@gtygo @qw4990
expression : fix function 'binSearch' in builtinIntervalRealSig not r…
d4b5424 
…eturn error (pingcap#12957)
ngaut pushed a commit that referenced this pull request Nov 27, 2019
@sre-bot @ngaut
expression : fix function 'binSearch' in builtinIntervalRealSig not r…
1db38d5 
…eturn error (#12957) (#13767)
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@gtygo @XiaTianliang
expression : fix function 'binSearch' in builtinIntervalRealSig not r…
d30103d 
…eturn error (pingcap#12957)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francis0407 francis0407 francis0407 left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

Assignees
No one assigned
Labels
component/expression contribution This PR is from a community contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gtygo @francis0407 @XuHuaiyu @sre-bot @coocood