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

expression: rewrite builtin function: ADDDATE, SUBDATE #4504

Merged
merged 25 commits into from Sep 14, 2017

Conversation

Projects
None yet
3 participants
@zz-jason
Member

zz-jason commented Sep 12, 2017

to #4080

@zz-jason zz-jason added the status/WIP label Sep 12, 2017

@zz-jason zz-jason referenced this pull request Sep 12, 2017

Closed

rewrite builtin functions #4080

150 of 150 tasks complete

@zz-jason zz-jason removed the status/WIP label Sep 13, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Sep 13, 2017

@XuHuaiyu PTAL

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 13, 2017

Member

/run-all-test

Member

zz-jason commented Sep 13, 2017

/run-all-test

Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go

zz-jason added some commits Sep 14, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 14, 2017

Member

/run-all-test

Member

zz-jason commented Sep 14, 2017

/run-all-test

Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
return types.Time{}, true, errors.Trace(err)
}
interval, isNull, err := b.getIntervalFromInt(b.ctx, b.args, row, unit)

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

getIntervalFromString or getIntervalFromInt?

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

getIntervalFromString or getIntervalFromInt?

Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
AddResult string
SubResult string
}{
{"\"2011-11-11\"", "1", "DAY", "2011-11-12", "2011-11-10"},

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

why escape " ?

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

why escape " ?

This comment has been minimized.

@zz-jason

zz-jason Sep 14, 2017

Member

means it is a string literal

@zz-jason

zz-jason Sep 14, 2017

Member

means it is a string literal

@XuHuaiyu

Comment addressed will make this a LGTM

Show outdated Hide outdated expression/builtin_time.go
@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

PTAL @winkyao

Contributor

XuHuaiyu commented Sep 14, 2017

PTAL @winkyao

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

LGTM

Contributor

XuHuaiyu commented Sep 14, 2017

LGTM

Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
dateTp = mysql.TypeDatetime
}
date, err := types.ParseTime(dateStr, dateTp, types.MaxFsp)

This comment has been minimized.

@winkyao

winkyao Sep 14, 2017

Member

types.MaxFsp? why not use dateStr decimal length?

@winkyao

winkyao Sep 14, 2017

Member

types.MaxFsp? why not use dateStr decimal length?

This comment has been minimized.

@zz-jason

zz-jason Sep 14, 2017

Member

dateStr's Decimal will always be -1

@zz-jason

zz-jason Sep 14, 2017

Member

dateStr's Decimal will always be -1

)
// handleInvalidTimeError reports error or warning depend on the context.
func handleInvalidTimeError(ctx context.Context, err error) error {
if err == nil || !terror.ErrorEqual(err, types.ErrInvalidTimeFormat) {
if err == nil || !(terror.ErrorEqual(err, types.ErrInvalidTimeFormat) || types.ErrIncorrectDatetimeValue.Equal(err)) {

This comment has been minimized.

@winkyao

winkyao Sep 14, 2017

Member

types. ErrInvalidTimeFormat.Equal(err) ?

@winkyao

winkyao Sep 14, 2017

Member

types. ErrInvalidTimeFormat.Equal(err) ?

This comment has been minimized.

@zz-jason

zz-jason Sep 14, 2017

Member

ErrInvalidTimeFormat do not have a method named Equal

@zz-jason

zz-jason Sep 14, 2017

Member

ErrInvalidTimeFormat do not have a method named Equal

@pingcap pingcap deleted a comment from winkyao Sep 14, 2017

@winkyao

LGTM

@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao

winkyao Sep 14, 2017

Member

/run-all-test

Member

winkyao commented Sep 14, 2017

/run-all-test

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Sep 14, 2017

@zz-jason zz-jason merged commit 27f2594 into master Sep 14, 2017

10 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@zz-jason zz-jason deleted the zz-jason/expression/time/date_arith branch Sep 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment