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 wrong behavior for IF(not_int, *, *) #15016

Merged
merged 6 commits into from
Mar 13, 2020
Merged

expression: fix wrong behavior for IF(not_int, *, *) #15016

merged 6 commits into from
Mar 13, 2020

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented Feb 29, 2020 •
edited
Loading

What problem does this PR solve?

fix #11601

What is changed and how it works?

original issue was cause by using types.ETInt for arg0 rounding float to int. This PR using wrapWithIsTrue of isTrueOrFalseFunctionClass to handle different types correctly convert to int.

Check List

Tests

  • Unit test
    • add cases whose args close to 0.0

Code changes

Side effects

Related changes

Release note

@lance6716 lance6716 requested a review from a team as a code owner February 29, 2020 02:42
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Feb 29, 2020
@lance6716 lance6716 closed this Feb 29, 2020
@lance6716 lance6716 mentioned this pull request Feb 29, 2020
Closed
@lance6716 lance6716 reopened this Feb 29, 2020
@zz-jason zz-jason requested review from a team and removed request for a team March 2, 2020 01:52
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team March 2, 2020 01:52
@codecov
Copy link

codecov bot commented Mar 3, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #15016   +/-   ##
===========================================
  Coverage          ?   80.2193%           
===========================================
  Files             ?        503           
  Lines             ?     130921           
  Branches          ?          0           
===========================================
  Hits              ?     105024           
  Misses            ?      17592           
  Partials          ?       8305

@lance6716 lance6716 changed the title expression: fix wrong behavior for IF(real, *, *) expression: fix wrong behavior for IF(not_int, *, *) Mar 3, 2020
SunRunAway
SunRunAway reviewed Mar 11, 2020
View reviewed changes
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.

Well done.
Could you help fix CASE function in this PR either? See #11601 (comment)

@SunRunAway
Copy link
Contributor

A separated PR is OK, likewise.

@lance6716
Copy link
Contributor Author

caseWhenFunction is more complicated, I prefer another PR.

@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 11, 2020
SunRunAway
SunRunAway reviewed Mar 11, 2020
View reviewed changes
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.

LGTM

@SunRunAway SunRunAway requested a review from qw4990 March 11, 2020 12:26
@lance6716 lance6716 mentioned this pull request Mar 11, 2020
Closed
@lance6716
Copy link
Contributor Author

A very similar PR is #15309, please take a glance after reviewing this PR 😄

qw4990
qw4990 approved these changes Mar 13, 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! NICE!

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Mar 13, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 13, 2020

Your auto merge job has been accepted, waiting for 15309

@qw4990 qw4990 removed the status/LGT1 Indicates that a PR has LGTM 1. label Mar 13, 2020
@SunRunAway SunRunAway linked an issue Mar 13, 2020 that may be closed by this pull request
Function NOT/IF returns incorrect result with decimal argument #11188
Closed
@sre-bot
Copy link
Contributor

sre-bot commented Mar 13, 2020

/run-all-tests

@sre-bot sre-bot merged commit 4f21076 into pingcap:master Mar 13, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 13, 2020
@lance6716 @sre-bot
expression: fix wrong behavior for IF(not_int, *, *) (pingcap#15016)
181bcbb
@sre-bot sre-bot mentioned this pull request Mar 13, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Mar 13, 2020

cherry pick to release-2.1 in PR #15355

@SunRunAway SunRunAway mentioned this pull request Mar 13, 2020
Closed
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 13, 2020
@lance6716 @sre-bot
expression: fix wrong behavior for IF(not_int, *, *) (pingcap#15016)
1986749
@sre-bot sre-bot mentioned this pull request Mar 13, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Mar 13, 2020

cherry pick to release-3.0 in PR #15356

@SunRunAway SunRunAway assigned SunRunAway and unassigned SunRunAway Mar 13, 2020
sre-bot added a commit that referenced this pull request Apr 3, 2020
@sre-bot
expression: fix wrong behavior for IF(not_int, *, *) (#15016) (#15355)
cce7ff2
sre-bot added a commit that referenced this pull request Apr 4, 2020
@sre-bot
expression: fix wrong behavior for IF(not_int, *, *) (#15016) (#15356)
8528234
sre-bot added a commit to sre-bot/tidb that referenced this pull request Apr 15, 2020
@sre-bot
expression: fix wrong behavior for IF(not_int, *, *) (pingcap#15016) (p…
21a0586 
…ingcap#15356)
@sre-bot sre-bot mentioned this pull request Apr 15, 2020
Merged
zz-jason pushed a commit that referenced this pull request Apr 15, 2020
@sre-bot
expression: fix wrong behavior for IF(not_int, *, *) (#15016)… (#16424)
849101e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SunRunAway SunRunAway SunRunAway left review comments

@qw4990 qw4990 qw4990 approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

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.
Projects
None yet
Milestone
No milestone
5 participants
@lance6716 @SunRunAway @sre-bot @qw4990 @zz-jason