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: TIME_TO_SEC, SEC_TO_TIME #4342

Merged
merged 11 commits into from Sep 5, 2017

Conversation

Projects
None yet
3 participants
@breeswish
Member

breeswish commented Aug 28, 2017

to #4080

Fix #4004

@zz-jason zz-jason referenced this pull request Aug 28, 2017

Closed

rewrite builtin functions #4080

150 of 150 tasks complete
argType := args[0].GetType()
argEvalTp := fieldTp2EvalTp(argType)
if argEvalTp == tpString {
retFsp = types.UnspecifiedLength

This comment has been minimized.

@zz-jason

zz-jason Aug 28, 2017

Member

how about retFsp = types.MaxFsp directly ?

@zz-jason

zz-jason Aug 28, 2017

Member

how about retFsp = types.MaxFsp directly ?

This comment has been minimized.

@breeswish

breeswish Aug 28, 2017

Member

I wrote it as this to emphasis that ret_decimal = -1 when arg_type = string is not because that it is a special case, but because that when string is used in arg, its own decimal is -1 (See #4236)

@breeswish

breeswish Aug 28, 2017

Member

I wrote it as this to emphasis that ret_decimal = -1 when arg_type = string is not because that it is a special case, but because that when string is used in arg, its own decimal is -1 (See #4236)

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 28, 2017

Member

can this pr fix #4162 ?

Member

zz-jason commented Aug 28, 2017

can this pr fix #4162 ?

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

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Aug 28, 2017

Member

@zz-jason No it can't. I think the bug is EvalDuration's scope. 00:00:61 is an invalid duration.

Member

breeswish commented Aug 28, 2017

@zz-jason No it can't. I think the bug is EvalDuration's scope. 00:00:61 is an invalid duration.

breeswish and others added some commits Aug 28, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 29, 2017

Member

@breeswish since pr #4347 merged, function newBaseBuiltinFuncWithTp does not return error any more, plz update code and fix ci

Member

zz-jason commented Aug 29, 2017

@breeswish since pr #4347 merged, function newBaseBuiltinFuncWithTp does not return error any more, plz update code and fix ci

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 31, 2017

Member

@breeswish plz resolve conflicts.

can this pr fix issue #4004 ?

Member

zz-jason commented Aug 31, 2017

@breeswish plz resolve conflicts.

can this pr fix issue #4004 ?

Show outdated Hide outdated expression/builtin_time_test.go
Show outdated Hide outdated plan/typeinfer_test.go
Show outdated Hide outdated expression/integration_test.go
Show outdated Hide outdated expression/integration_test.go
@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 2, 2017

Member

@breeswish plz address comment && resolve conflicts

Member

zz-jason commented Sep 2, 2017

@breeswish plz address comment && resolve conflicts

@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish
Member

breeswish commented Sep 5, 2017

@XuHuaiyu

LGTM

@breeswish breeswish merged commit 0513ab3 into master Sep 5, 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

@breeswish breeswish deleted the wenxuan/exp-rewrite-time-to-sec branch Sep 5, 2017

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