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 the wrong behaviour bug of vec LeastTime #17848

Merged
merged 8 commits into from
Jun 12, 2020
Merged

expression: fix the wrong behaviour bug of vec LeastTime #17848

merged 8 commits into from
Jun 12, 2020

Conversation

JmPotato
Copy link
Member

@JmPotato JmPotato commented Jun 8, 2020

Signed-off-by: JmPotato [email protected]

What problem does this PR solve?

Issue Number: close #17826

Problem Summary:

LEAST() function's behaviour is inconsistent with MySQL.

What is changed and how it works?

What's Changed:

  • vectorized LeastTime function

How it Works:

  • Make a deep copy when find an invalid time.
  • Return NULL if any argument is NULL.

Check List

Tests

  • Unit test
  • Integration test

Release note

No release note

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Jun 8, 2020
@JmPotato
Copy link
Member Author

JmPotato commented Jun 8, 2020

@sre-bot /run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

/run-all-tests

@codecov
Copy link

codecov bot commented Jun 8, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #17848   +/-   ##
===========================================
  Coverage   79.5598%   79.5598%           
===========================================
  Files           524        524           
  Lines        142538     142538           
===========================================
  Hits         113403     113403           
  Misses        19987      19987           
  Partials       9148       9148

@JmPotato
Copy link
Member Author

JmPotato commented Jun 8, 2020

@sre-bot /run-integration-copr-test copr-test=pr/133 tikv=pr/7888

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

/run-integration-copr-test copr-test=pr/133 tikv=pr/7888

@JmPotato JmPotato mentioned this pull request Jun 8, 2020
Closed
@qw4990 qw4990 self-requested a review June 8, 2020 12:22
qw4990
qw4990 reviewed Jun 9, 2020
View reviewed changes
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.

Thanks for your contribution!!
Should the row-based implementation builtinLeastTimeSig.evalString() also be changed?
And please add some tests for this in expression/builtin_compare.go.

@JmPotato
Copy link
Member Author

JmPotato commented Jun 9, 2020

Thanks for your contribution!!
Should the row-based implementation builtinLeastTimeSig.evalString() also be changed?
And please add some tests for this in expression/builtin_compare.go.

The row-based implementation is not affected by this bug. And I added some unit tests to cover the invalid time and NULL cases. PTAL, thx!

@JmPotato JmPotato requested a review from qw4990 June 9, 2020 06:52
qw4990
qw4990 reviewed Jun 9, 2020
View reviewed changes
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 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 9, 2020
Reminiscent
Reminiscent approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@Reminiscent Reminiscent left a comment

Choose a reason for hiding this comment

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

LGTM

@JmPotato
Copy link
Member Author

JmPotato commented Jun 9, 2020

@sre-bot /run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

/run-all-tests

@JmPotato
Copy link
Member Author

JmPotato commented Jun 9, 2020

What's wrong with the CI test?

@JmPotato
Copy link
Member Author

JmPotato commented Jun 9, 2020

@sre-bot /run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

/run-all-tests

@JmPotato JmPotato requested a review from qw4990 June 9, 2020 08:02
@qw4990
Copy link
Contributor

qw4990 commented Jun 9, 2020

/run-all-tests

@JmPotato
Copy link
Member Author

JmPotato commented Jun 9, 2020

Some of the CI tests keep failing randomly. Is this normal?

@JmPotato
Copy link
Member Author

JmPotato commented Jun 9, 2020

@sre-bot /run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jun 9, 2020

/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Jun 9, 2020

/run-integration-br-test

@qw4990
Copy link
Contributor

qw4990 commented Jun 9, 2020

/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Jun 12, 2020

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jun 12, 2020

/run-integration-copr-test copr-test=pr/133 tikv=pr/7888

@qw4990 qw4990 merged commit 765612f into pingcap:master Jun 12, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 12, 2020
@JmPotato @sre-bot
cherry pick pingcap#17848 to release-4.0
a31fdf7 
Signed-off-by: sre-bot <[email protected]>
@sre-bot sre-bot mentioned this pull request Jun 12, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jun 12, 2020

cherry pick to release-4.0 in PR #17972

@JmPotato JmPotato deleted the fix_least branch June 12, 2020 03:39
ti-srebot pushed a commit that referenced this pull request Jul 28, 2020
@sre-bot
expression: fix the wrong behaviour bug of vec LeastTime (#17848) (#1…
9c66a42 
…7972)

Signed-off-by: sre-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Reminiscent Reminiscent Reminiscent approved these changes

@qw4990 qw4990 qw4990 approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
Labels
component/expression contribution This PR is from a community contributor. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong behaviour of MySQL function LEAST()
4 participants
@JmPotato @sre-bot @qw4990 @Reminiscent