-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: DATEDIFF #4212
Conversation
expression/builtin_time.go
Outdated
bf, err := newBaseBuiltinFuncWithTp(args, ctx, tpInt, tpTime, tpTime) | ||
if err != nil { | ||
return nil, errors.Trace(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set Flen
and Decimal
of bf.tp
func (b *builtinDateDiffSig) evalInt(row []types.Datum) (int64, bool, error) { | ||
ctx := b.ctx.GetSessionVars().StmtCtx | ||
t1, isNull, err := b.args[0].EvalTime(row, ctx) | ||
if isNull || err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in mysql:
MySQL > select datediff("2014-12-09", "aaaa");
Field 1: `datediff("2014-12-09", "aaaa")`
Catalog: `def`
Database: ``
Table: ``
Org_table: ``
Type: LONGLONG
Collation: binary (63)
Length: 7
Max_length: 0
Decimals: 0
Flags: BINARY NUM
+--------------------------------+
| datediff("2014-12-09", "aaaa") |
+--------------------------------+
| NULL |
+--------------------------------+
1 row in set, 1 warning (0.00 sec)
MySQL > show warnings;
Field 1: `Level`
Catalog: `def`
Database: ``
Table: ``
Org_table: ``
Type: VAR_STRING
Collation: utf8_general_ci (33)
Length: 21
Max_length: 7
Decimals: 31
Flags: NOT_NULL
Field 2: `Code`
Catalog: `def`
Database: ``
Table: ``
Org_table: ``
Type: LONG
Collation: binary (63)
Length: 4
Max_length: 4
Decimals: 0
Flags: NOT_NULL UNSIGNED BINARY NUM
Field 3: `Message`
Catalog: `def`
Database: ``
Table: ``
Org_table: ``
Type: VAR_STRING
Collation: utf8_general_ci (33)
Length: 1536
Max_length: 32
Decimals: 31
Flags: NOT_NULL
+---------+------+----------------------------------+
| Level | Code | Message |
+---------+------+----------------------------------+
| Warning | 1292 | Incorrect datetime value: 'aaaa' |
+---------+------+----------------------------------+
1 row in set (0.00 sec)
expression/integration_test.go
Outdated
@@ -963,6 +963,9 @@ func (s *testIntegrationSuite) TestTimeBuiltin(c *C) { | |||
result.Check(testkit.Rows("62966505600 63426672000 63426721412 63426721412")) | |||
result = tk.MustQuery("select to_days(950501), to_days('2007-10-07'), to_days('2007-10-07 00:00:59'), to_days('0000-01-01')") | |||
result.Check(testkit.Rows("728779 733321 733321 1")) | |||
result = tk.MustQuery("select datediff('2007-12-31 23:59:59','2007-12-30'), datediff('2010-11-30 23:59:59', " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add more tests like datediff("2014-12-09", "aaaa");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
plan/typeinfer_test.go
Outdated
@@ -994,5 +994,12 @@ func (s *testPlanSuite) createTestCase4TimeFuncs() []typeInferTestCase { | |||
{"microsecond(c_blob )", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag, 6, 0}, | |||
{"microsecond(c_set )", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag, 6, 0}, | |||
{"microsecond(c_enum )", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag, 6, 0}, | |||
|
|||
{"datediff(c_char, c_datetime)", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag, 20, 0}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flen
is not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it's not consistent with mysql. But I think it's reasonable to set it to mysql.MaxIntWidth. I'll take a look how this was set in mysql......
expression/integration_test.go
Outdated
@@ -963,6 +963,9 @@ func (s *testIntegrationSuite) TestTimeBuiltin(c *C) { | |||
result.Check(testkit.Rows("62966505600 63426672000 63426721412 63426721412")) | |||
result = tk.MustQuery("select to_days(950501), to_days('2007-10-07'), to_days('2007-10-07 00:00:59'), to_days('0000-01-01')") | |||
result.Check(testkit.Rows("728779 733321 733321 1")) | |||
result = tk.MustQuery("select datediff('2007-12-31 23:59:59','2007-12-30'), datediff('2010-11-30 23:59:59', " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add case like select datediff('2007-12-31 23:59:59', '23:59:59');
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@spongedu plz resolve conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
399dba5
to
71c2db8
Compare
@spongedu please resolve conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest LGTM
expression/integration_test.go
Outdated
@@ -1078,6 +1078,9 @@ func (s *testIntegrationSuite) TestTimeBuiltin(c *C) { | |||
result.Check(testkit.Rows("62966505600 63426672000 63426721412 63426721412")) | |||
result = tk.MustQuery("select to_days(950501), to_days('2007-10-07'), to_days('2007-10-07 00:00:59'), to_days('0000-01-01')") | |||
result.Check(testkit.Rows("728779 733321 733321 1")) | |||
result = tk.MustQuery("select datediff('2007-12-31 23:59:59','2007-12-30'), datediff('2010-11-30 23:59:59', " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test cases like
- "0000-01-01", "0001-01-01"
- "0001-00-01", "0001-00-01"
- "0001-01-00", "0001-01-00"
- "2017-01-01", "2017-01-01"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. @XuHuaiyu PTAL
eedfb4e
to
e3504f9
Compare
Please merge the master, I've split tpTime into tpDatetime and tpTimestamp. |
e3504f9
to
fe55689
Compare
@XuHuaiyu Done |
please fix ci |
@XuHuaiyu Done. PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
for #4080