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: handle builtin time getInterval from Decimal\Real #11479

Merged
merged 6 commits into from
Jul 30, 2019
Merged

expression: handle builtin time getInterval from Decimal\Real #11479

merged 6 commits into from
Jul 30, 2019

Conversation

AndrewDi
Copy link
Contributor

@AndrewDi AndrewDi commented Jul 28, 2019 •
edited by zz-jason
Loading

What problem does this PR solve?

Part of issue #11223, fixes #11309

What is changed and how it works?

  • Use evalDecimal to retrive decimal value and convert to string.
  • Keep real's precision.

Check List

Tests

  • Integration test

@AndrewDi
Copy link
Contributor Author

AndrewDi commented Jul 28, 2019 •
edited
Loading

@wshwsh12 @SunRunAway PTAL

@codecov
Copy link

codecov bot commented Jul 28, 2019

Codecov Report

Merging #11479 into master will decrease coverage by 0.1952%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #11479        +/-   ##
================================================
- Coverage   81.5575%   81.3623%   -0.1953%     
================================================
  Files           424        424                
  Lines         91745      90918       -827     
================================================
- Hits          74825      73973       -852     
- Misses        11605      11624        +19     
- Partials       5315       5321         +6

@codecov
Copy link

codecov bot commented Jul 28, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11479   +/-   ##
===========================================
  Coverage   81.3449%   81.3449%           
===========================================
  Files           424        424           
  Lines         91053      91053           
===========================================
  Hits          74067      74067           
  Misses        11657      11657           
  Partials       5329       5329

@shenli shenli added the contribution This PR is from a community contributor. label Jul 28, 2019
@SunRunAway SunRunAway mentioned this pull request Jul 29, 2019
Open
37 tasks
@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Jul 29, 2019
qw4990
qw4990 reviewed Jul 29, 2019
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

@sre-bot
Copy link
Contributor

sre-bot commented Jul 29, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

XuHuaiyu
XuHuaiyu reviewed Jul 29, 2019
View reviewed changes
expression/builtin_time.go
@@ -2652,10 +2652,11 @@ func (du *baseDateArithmitical) getIntervalFromString(ctx sessionctx.Context, ar
}

func (du *baseDateArithmitical) getIntervalFromDecimal(ctx sessionctx.Context, args []Expression, row chunk.Row, unit string) (string, bool, error) {
interval, isNull, err := args[1].EvalString(ctx, row)
decimal, isNull, err := args[1].EvalDecimal(ctx, row)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unexpectedly, we do not have a case to cover this line before. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XuHuaiyu I add unit test for getIntervalFromDecimal PTAL

expression/integration_test.go
tk.MustExec("use test")
tk.MustExec(`drop table if exists t;`)
tk.MustExec(`CREATE TABLE t (a decimal(6,3),b double(6,3),c float(6,3));`)
tk.MustExec(`INSERT INTO t VALUES (1.100,1.100,1.100);`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more complicated cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any Idea...I think type covertion is enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

decimal(11,7): 123.9999999, -123.9999999

Copy link
Contributor

Choose a reason for hiding this comment

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

double(11, 7)
float(11, 7)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wshwsh12 wshwsh12 requested a review from XuHuaiyu July 30, 2019 05:25
XuHuaiyu
XuHuaiyu reviewed Jul 30, 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 the status/can-merge Indicates a PR has been approved by a committer. label Jul 30, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 30, 2019

/run-all-tests

@sre-bot sre-bot merged commit 768f61f into pingcap:master Jul 30, 2019
@sre-bot sre-bot mentioned this pull request Jul 30, 2019
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jul 30, 2019

cherry pick to release-3.0 in PR #11527

@sre-bot
Copy link
Contributor

sre-bot commented Jul 30, 2019

cherry pick to release-2.1 failed

sre-bot added a commit that referenced this pull request Aug 1, 2019
@sre-bot
expression: handle builtin time getInterval from Decimal\Real (#11479) (
d523d65 
#11527)
This was referenced Aug 5, 2019
Merged
Merged
wshwsh12 pushed a commit to wshwsh12/tidb that referenced this pull request Aug 5, 2019
@AndrewDi @wshwsh12
expression: handle builtin time getInterval from Decimal\Real (pingca…
6c1a9b3 
…p#11479)

All tests passed, auto merged by Bot
@wshwsh12 wshwsh12 mentioned this pull request Aug 5, 2019
Merged
sre-bot pushed a commit that referenced this pull request Aug 6, 2019
@wshwsh12 @sre-bot
expression: handle builtin time getInterval from Decimal\Real (#11479) (
97ef360 
#11628)
@AndrewDi AndrewDi deleted the fix_issue_11309 branch February 1, 2021 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qw4990 qw4990 qw4990 left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

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. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong result of Dateadd() when it working on a Decimal(or float,double) column
6 participants
@AndrewDi @sre-bot @qw4990 @XuHuaiyu @shenli @winkyao