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 TIMEDIFF #4496

Merged
merged 19 commits into from Sep 14, 2017

Conversation

Projects
None yet
3 participants
@winkyao
Member

winkyao commented Sep 11, 2017

to #4080

@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao
Member

winkyao commented Sep 11, 2017

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

Closed

rewrite builtin functions #4080

150 of 150 tasks complete
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
// TruncateOverflowMySQLTime truncates d when it overflows, and return truncated duration.
func TruncateOverflowMySQLTime(d gotime.Duration) (gotime.Duration, bool) {
// TruncateOverflowMySQLTime truncates d when it overflows, and return ErrTruncatedWrongVal.
func TruncateOverflowMySQLTime(d gotime.Duration) (gotime.Duration, error) {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 12, 2017

Contributor

add test for this function.

@XuHuaiyu

XuHuaiyu Sep 12, 2017

Contributor

add test for this function.

This comment has been minimized.

@winkyao

winkyao Sep 12, 2017

Member

done!

@winkyao

winkyao Sep 12, 2017

Member

done!

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
@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao
Member

winkyao commented Sep 12, 2017

@XuHuaiyu PTAL

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
@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao
Member

winkyao commented Sep 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@winkyao
Member

winkyao commented Sep 14, 2017

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
Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
@@ -47,7 +47,7 @@ func findColumnIndexByGroup(groups []LogicalPlan, col *expression.Column) int {
return i
}
}
log.Errorf("Unknown columns %s, from id %d, position %d", col, col.FromID, col.Position)
log.Errorf("Unknown columns %s, from id %v, position %d", col, col.FromID, col.Position)

This comment has been minimized.

@zz-jason

zz-jason Sep 14, 2017

Member

why change "%d" to "%v" ?

@zz-jason

zz-jason Sep 14, 2017

Member

why change "%d" to "%v" ?

This comment has been minimized.

@winkyao

winkyao Sep 14, 2017

Member

my local go vet will report "plan/join_reorder.go:50: arg col.FromID for printf verb %d of wrong type: string"

@winkyao

winkyao Sep 14, 2017

Member

my local go vet will report "plan/join_reorder.go:50: arg col.FromID for printf verb %d of wrong type: string"

winkyao added some commits Sep 14, 2017

@XuHuaiyu

A tiny change will make this a LGTM.

// See https://dev.mysql.com/doc/refman/5.5/en/date-and-time-literals.html.
func StrToDuration(sc *variable.StatementContext, str string, fsp int) (t Time, err error) {
func StrToDuration(sc *variable.StatementContext, str string, fsp int) (d Duration, t Time, isDuration bool, err error) {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

Str2TimeOrDuration better?

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

Str2TimeOrDuration better?

This comment has been minimized.

@winkyao

winkyao Sep 14, 2017

Member

MySQL use strToTime, so I think no need to change it...the better way is to eliminate the duration type totally.

@winkyao

winkyao Sep 14, 2017

Member

MySQL use strToTime, so I think no need to change it...the better way is to eliminate the duration type totally.

Show outdated Hide outdated expression/builtin_time.go
@winkyao

This comment has been minimized.

Show comment
Hide comment
@winkyao
Member

winkyao commented Sep 14, 2017

@XuHuaiyu PTAL

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

LGTM

Contributor

XuHuaiyu commented Sep 14, 2017

LGTM

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

/run-all-test

Contributor

XuHuaiyu commented Sep 14, 2017

/run-all-test

@XuHuaiyu XuHuaiyu added status/LGT2 and removed status/LGT1 labels Sep 14, 2017

@XuHuaiyu XuHuaiyu merged commit b812f32 into master Sep 14, 2017

4 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
license/cla Contributor License Agreement is signed.
Details

@XuHuaiyu XuHuaiyu deleted the winkyao/rewrite_timediff branch Sep 14, 2017

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