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 ADDTIME, SUBTIME #4333

Merged
merged 46 commits into from Sep 15, 2017

Conversation

Projects
None yet
4 participants
@spongedu
Contributor

spongedu commented Aug 26, 2017

for #4080 . Rewrite builtin function ADDTIME and SUBTIME, and fixes some former bugs.

1. TiDB loss precision when add duration to duration.

For example:
In mysql:

mysql> select addtime('01:01:11', '0:0:1.013');
Field   1:  `addtime('01:01:11', '0:0:1.013')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       STRING
Collation:  utf8_general_ci (33)
Length:     87
Max_length: 15
Decimals:   31
Flags:


+----------------------------------+
| addtime('01:01:11', '0:0:1.013') |
+----------------------------------+
| 01:01:12.013000                  |   
+----------------------------------+
1 row in set (0.00 sec)

In TiDB:

tidb> select addtime('01:01:11', '0:0:1.013');
Field   1:  `addtime('01:01:11', '0:0:1.013')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       VAR_STRING
Collation:  utf8_general_ci (33)
Length:     0
Max_length: 8
Decimals:   31
Flags:


+----------------------------------+
| addtime('01:01:11', '0:0:1.013') |
+----------------------------------+
| 01:01:12                         |
+----------------------------------+
1 row in set (0.00 sec)

2. fsp of ADDTIME, SUBTIME's results is not consistent with mysql under certain circumstance.

For example:
In mysql:

mysql> select addtime('01:01:11.00', '0:0:1');
Field   1:  `addtime('01:01:11.00', '0:0:1')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       STRING
Collation:  utf8_general_ci (33)
Length:     87
Max_length: 8
Decimals:   31
Flags:

+---------------------------------+
| addtime('01:01:11.00', '0:0:1') |
+---------------------------------+
| 01:01:12                        |
+---------------------------------+
1 row in set (0.00 sec)

mysql> select addtime('2017-01-01 01:01:11.12', '0:0:1');
Field   1:  `addtime('2017-01-01 01:01:11.12', '0:0:1')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       STRING
Collation:  utf8_general_ci (33)
Length:     87
Max_length: 26
Decimals:   31
Flags:


+--------------------------------------------+
| addtime('2017-01-01 01:01:11.12', '0:0:1') |
+--------------------------------------------+
| 2017-01-01 01:01:12.120000                 |
+--------------------------------------------+
1 row in set (0.00 sec)

mysql> select subtime('2017-01-01 01:01:11.12', '0:0:1.120000');
Field   1:  `subtime('2017-01-01 01:01:11.12', '0:0:1.120000')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       STRING
Collation:  utf8_general_ci (33)
Length:     87
Max_length: 19
Decimals:   31
Flags:


+---------------------------------------------------+
| subtime('2017-01-01 01:01:11.12', '0:0:1.120000') |
+---------------------------------------------------+
| 2017-01-01 01:01:10                               |
+---------------------------------------------------+
1 row in set (0.00 sec)

In TiDB:

tidb> select addtime('01:01:11.00', '0:0:1');
Field   1:  `addtime('01:01:11.00', '0:0:1')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       VAR_STRING
Collation:  utf8_general_ci (33)
Length:     0
Max_length: 15
Decimals:   31
Flags:


+---------------------------------+
| addtime('01:01:11.00', '0:0:1') |
+---------------------------------+
| 01:01:12.000000                 |
+---------------------------------+
1 row in set (0.00 sec)


tidb> select addtime('2017-01-01 01:01:11.12', '0:0:1');
Field   1:  `addtime('2017-01-01 01:01:11.12', '0:0:1')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       VAR_STRING
Collation:  utf8_general_ci (33)
Length:     0
Max_length: 22
Decimals:   31
Flags:


+--------------------------------------------+
| addtime('2017-01-01 01:01:11.12', '0:0:1') |
+--------------------------------------------+
| 2017-01-01 01:01:12.12                     |
+--------------------------------------------+
1 row in set (0.00 sec)

tidb> select subtime('2017-01-01 01:01:11.12', '0:0:1.120000');
Field   1:  `subtime('2017-01-01 01:01:11.12', '0:0:1.120000')`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       VAR_STRING
Collation:  utf8_general_ci (33)
Length:     0
Max_length: 26
Decimals:   31
Flags:      


+---------------------------------------------------+
| subtime('2017-01-01 01:01:11.12', '0:0:1.120000') |
+---------------------------------------------------+
| 2017-01-01 01:01:10.000000                        |
+---------------------------------------------------+
1 row in set (0.00 sec)

spongedu added some commits Aug 25, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 26, 2017

Member

you have to rewrite addtime and subtime ...

Member

zz-jason commented Aug 26, 2017

you have to rewrite addtime and subtime ...

@spongedu

This comment has been minimized.

Show comment
Hide comment
@spongedu

spongedu Aug 26, 2017

Contributor

@zz-jason WIP......

Contributor

spongedu commented Aug 26, 2017

@zz-jason WIP......

@zz-jason zz-jason added the status/WIP label Aug 26, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 26, 2017

to #4080

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

Closed

rewrite builtin functions #4080

150 of 150 tasks complete
@spongedu

This comment has been minimized.

Show comment
Hide comment
@spongedu

spongedu Aug 27, 2017

Contributor

Rewrite ADDTIME. @zz-jason PTAL

Contributor

spongedu commented Aug 27, 2017

Rewrite ADDTIME. @zz-jason PTAL

@spongedu spongedu changed the title from expression: result of builtin function 'ADDTIME' 'SUBTIME' is not consistent with mysql to expression: rewrite builtin function ADDTIME, SUBTIME Aug 27, 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

@zz-jason zz-jason removed the status/WIP label Aug 28, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 29, 2017

Member

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

@spongedu 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
Member

zz-jason commented Aug 29, 2017

@XuHuaiyu PTAL

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 31, 2017

@breeswish PTAL

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 3, 2017

Member

@spongedu plz resolve conflicts

Member

zz-jason commented Sep 3, 2017

@spongedu plz resolve conflicts

@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Sep 3, 2017

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.

sre-bot commented Sep 3, 2017

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.

Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
default:
argTp1, retTp = tpString, tpString
}
switch tp2.Tp {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 4, 2017

Contributor

according to mysql manual, tp2 must be a duration expr.
can we always set argTp2 = tpDuration?

@XuHuaiyu

XuHuaiyu Sep 4, 2017

Contributor

according to mysql manual, tp2 must be a duration expr.
can we always set argTp2 = tpDuration?

This comment has been minimized.

@spongedu

spongedu Sep 9, 2017

Contributor

That's what the former implementation do in TiDB, which get something wrong. If we set argTp2 as tpDuration, a cast-to-duration will be inserted if the argument given is not of duration type ( string, e.g). The default cast behavior do not satisfy the requirements of ADD_TIME, SUB_TIME in many conditions, such as fsp requirements, string pattern restriction, etc. So we have to deal with the type-casting manually.

@spongedu

spongedu Sep 9, 2017

Contributor

That's what the former implementation do in TiDB, which get something wrong. If we set argTp2 as tpDuration, a cast-to-duration will be inserted if the argument given is not of duration type ( string, e.g). The default cast behavior do not satisfy the requirements of ADD_TIME, SUB_TIME in many conditions, such as fsp requirements, string pattern restriction, etc. So we have to deal with the type-casting manually.

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
_ builtinFunc = &builtinAddTimeDateTimeNullSig{}
_ builtinFunc = &builtinAddStringAndDurationSig{}
_ builtinFunc = &builtinAddStringAndStringSig{}
_ builtinFunc = &builtinAddTimeStringNullSig{}

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 4, 2017

Contributor

Can we merge line 146, line 149 and line 152 into one sig, if the types of the return values are the same?

@XuHuaiyu

XuHuaiyu Sep 4, 2017

Contributor

Can we merge line 146, line 149 and line 152 into one sig, if the types of the return values are the same?

This comment has been minimized.

@spongedu

spongedu Sep 9, 2017

Contributor

They have different return types. I think it's clearer to seperate them part.

@spongedu

spongedu Sep 9, 2017

Contributor

They have different return types. I think it's clearer to seperate them part.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 11, 2017

Member

/ok-to-test

Member

zz-jason commented Sep 11, 2017

/ok-to-test

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 11, 2017

Member

/run-all-test

Member

zz-jason commented Sep 11, 2017

/run-all-test

spongedu and others added some commits 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

Show outdated Hide outdated expression/integration_test.go
Show outdated Hide outdated expression/integration_test.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
return types.ZeroDuration, isNull, errors.Trace(err)
}
result, err := arg0.Sub(arg1)
if err != nil {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 13, 2017

Contributor

ditto

@XuHuaiyu

XuHuaiyu Sep 13, 2017

Contributor

ditto

This comment has been minimized.

@spongedu

spongedu Sep 13, 2017

Contributor

done

@spongedu

spongedu Sep 13, 2017

Contributor

done

result, err := arg0.Sub(arg1)
if err != nil {
return types.ZeroDuration, true, errors.Trace(err)
}

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 13, 2017

Contributor

ditto

@XuHuaiyu

XuHuaiyu Sep 13, 2017

Contributor

ditto

This comment has been minimized.

@spongedu

spongedu Sep 13, 2017

Contributor

done

@spongedu

spongedu Sep 13, 2017

Contributor

done

Show outdated Hide outdated expression/builtin_time.go
Show outdated Hide outdated expression/builtin_time.go
}
type addTimeFunctionClass struct {
baseFunctionClass
}
func (c *addTimeFunctionClass) getFunction(ctx context.Context, args []Expression) (builtinFunc, error) {
if err := c.verifyArgs(args); err != nil {
func (c *addTimeFunctionClass) getFunction(ctx context.Context, args []Expression) (sig builtinFunc, err error) {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

extract a common to implement the common getFunction behavior?

@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

extract a common to implement the common getFunction behavior?

This comment has been minimized.

@spongedu

spongedu Sep 14, 2017

Contributor

OK

@spongedu

spongedu Sep 14, 2017

Contributor

OK

spongedu added some commits Sep 14, 2017

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 14, 2017

Contributor

PTAL @zz-jason

Contributor

XuHuaiyu commented Sep 14, 2017

PTAL @zz-jason

// See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_addtime
func (b *builtinAddDurationAndDurationSig) evalDuration(row []types.Datum) (types.Duration, bool, error) {
sc := b.getCtx().GetSessionVars().StmtCtx
arg0, isNull, err := b.args[0].EvalDuration(row, sc)

This comment has been minimized.

@zz-jason

zz-jason Sep 14, 2017

Member

can we change arg0 to lhs, and arg1 to rhs ?

@zz-jason

zz-jason Sep 14, 2017

Member

can we change arg0 to lhs, and arg1 to rhs ?

This comment has been minimized.

@spongedu

spongedu Sep 14, 2017

Contributor

@zz-jason arg0 and arg1 are of the same side of expression, I think lhs and rhs is not quite proper here.

@spongedu

spongedu Sep 14, 2017

Contributor

@zz-jason arg0 and arg1 are of the same side of expression, I think lhs and rhs is not quite proper here.

@zz-jason zz-jason merged commit 1006dab into pingcap:master Sep 15, 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

@spongedu spongedu deleted the spongedu:fix_addtime branch Sep 15, 2017

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