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.

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

coprocessor/dag/expr: add to_days #3978

Merged
merged 4 commits into from Dec 27, 2018

Conversation

@GinYM
Copy link
Contributor

commented Dec 25, 2018

What have you changed? (mandatory)

implement builtin function: ToDays in coprocessor.
#3275

What are the type of the changes? (mandatory)

Improvement

How has this PR been tested? (mandatory)

Unit Test

Does this PR affect documentation (docs) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 25, 2018

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.

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

Thanks @GinYM

Please sign off your all commits. See https://github.com/tikv/tikv/blob/master/CONTRIBUTING.md#signing-off-the-commit

If you have already pushed some commits before, you can use git rebase -i master --signoff to resign them off.

Signed-off-by: GinYM <jinyiming1996@gmail.com>
@GinYM GinYM force-pushed the GinYM:to_days branch from 6fffc3f to 1c8051e Dec 26, 2018
Copy link
Contributor

left a comment

Neat!


#[test]
fn test_to_days() {
let cases = vec![

This comment has been minimized.

Copy link
@rleungx

rleungx Dec 26, 2018

Member

Can you add more test cases? e.g."0000-00-00", "1992-13-00", "2007-10-07 23:59:61", 123456789

This comment has been minimized.

Copy link
@GinYM

GinYM Dec 26, 2018

Author Contributor

It seems that Time::parse_utc_datetime cannot check invalid date such as "1992-13-00". Do we need to implement the checking in pub fn to_days() or in parse_datetime_internal() which is in the file mod.rs

This comment has been minimized.

Copy link
@huachaohuang

huachaohuang Dec 27, 2018

Contributor

Is this just the problem of the test?

This comment has been minimized.

Copy link
@GinYM

GinYM Dec 27, 2018

Author Contributor

Yes, just the problem of test.

This comment has been minimized.

Copy link
@rleungx

rleungx Dec 27, 2018

Member

Do we need to test these invalid cases? @huachaohuang

This comment has been minimized.

Copy link
@huachaohuang

huachaohuang Dec 27, 2018

Contributor

I think it's fine. @GinYM if you are interested, please help to improve these DateTime tests in another PR :)

This comment has been minimized.

Copy link
@GinYM

GinYM Dec 27, 2018

Author Contributor

@huachaohuang Sure, I will try it.

Copy link
Member

left a comment

Thanks a lot! Looks nice!

@ice1000 ice1000 dismissed their stale review Dec 26, 2018

Due to there's an unresolved review, I'm gonna dismiss my approving review to prevent bypassers from merging this.

@Hijiao Hijiao added the S: LGT1 label Dec 26, 2018
Copy link
Contributor

left a comment

LGTM


#[test]
fn test_to_days() {
let cases = vec![

This comment has been minimized.

Copy link
@huachaohuang

huachaohuang Dec 27, 2018

Contributor

I think it's fine. @GinYM if you are interested, please help to improve these DateTime tests in another PR :)

@rleungx rleungx added S: LGT2 and removed S: LGT1 labels Dec 27, 2018
@huachaohuang huachaohuang merged commit 72bf02f into tikv:master Dec 27, 2018
4 checks passed
4 checks passed
DCO All commits are signed off!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
idc-jenkins-ci/build Jenkins job succeeded.
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
@AndreMouche AndreMouche referenced this pull request Jan 17, 2019
333 of 469 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.