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: check if period is valid in period_add #10380

Merged
merged 6 commits into from
May 9, 2019
Merged

expression: check if period is valid in period_add #10380

merged 6 commits into from
May 9, 2019

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented May 7, 2019

What problem does this PR solve?

fix #9736.

What is changed and how it works?

add validPeriod.

Check List

Tests

  • Unit test

@qw4990 qw4990 requested review from erjiaqing, alivxxx and winoros May 7, 2019 08:36
@sourcelliu
Copy link
Contributor

#10332 is the same question. @qw4990 @lonng

@qw4990
Copy link
Contributor Author

qw4990 commented May 7, 2019

#10332 is the same question. @qw4990 @lonng

Yes, it seems that #10332 can be fixed by this PR.

@sourcelliu sourcelliu mentioned this pull request May 7, 2019
Merged
alivxxx
alivxxx reviewed May 8, 2019
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label May 8, 2019
@qw4990 qw4990 requested a review from lonng May 8, 2019 05:45
XuHuaiyu
XuHuaiyu reviewed May 8, 2019
View reviewed changes
expression/builtin_time.go Show resolved Hide resolved
expression/builtin_time.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 8, 2019 •
edited
Loading

Codecov Report

Merging #10380 into master will increase coverage by 0.0008%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10380        +/-   ##
================================================
+ Coverage   77.3505%   77.3514%   +0.0008%     
================================================
  Files           412        412                
  Lines         85724      85692        -32     
================================================
- Hits          66308      66284        -24     
+ Misses        14387      14379         -8     
  Partials       5029       5029

@qw4990 qw4990 changed the title expression: fix issue that period_add is not compatible with MySQL expression: check if period is valid in period_add May 8, 2019
@qw4990
Copy link
Contributor Author

qw4990 commented May 9, 2019

PTAL

XuHuaiyu
XuHuaiyu reviewed May 9, 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 previously approved these changes May 9, 2019
View reviewed changes
@qw4990
Copy link
Contributor Author

qw4990 commented May 9, 2019

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented May 9, 2019

/run-all-tests

@qw4990 qw4990 added status/all tests passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 9, 2019
@qw4990 qw4990 added the status/LGT2 Indicates that a PR has LGTM 2. label May 9, 2019
@qw4990
Copy link
Contributor Author

qw4990 commented May 9, 2019

To solve the CI problem, I git rebase this PR to master without any change, so please help me to approve this PR again~ @lamxTyler @XuHuaiyu

@qw4990 qw4990 merged commit 37e8241 into pingcap:master May 9, 2019
@qw4990 qw4990 mentioned this pull request May 9, 2019
Closed
30 tasks
qw4990 added a commit to qw4990/tidb that referenced this pull request May 13, 2019
@qw4990
expression: check if period is valid in period_add (pingcap#10380)
7bcb77c
@qw4990 qw4990 mentioned this pull request May 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@alivxxx alivxxx alivxxx approved these changes

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@erjiaqing erjiaqing Awaiting requested review from erjiaqing

@winoros winoros Awaiting requested review from winoros

@lonng lonng Awaiting requested review from lonng

Assignees
No one assigned
Labels
component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PERIOD_ADD is not compatible with Mysql
5 participants
@qw4990 @sourcelliu @zz-jason @XuHuaiyu @alivxxx