Skip to content
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, executor: castStringAsTime returns null when sql_mode is not strict #8516

Merged
merged 9 commits into from
Dec 6, 2018

Conversation

XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Nov 29, 2018

What problem does this PR solve?

castStringAsTime returns null when sql_mode is not strict.
Before this commit, the following query will raise an error:

tidb> set @@sql_mode = '';
Query OK, 0 rows affected (0.00 sec)

tidb> create table t(a datetime);
Query OK, 0 rows affected (0.01 sec)

tidb> insert into t value("1999-12-12");
Query OK, 1 row affected (0.00 sec)

tidb> update t set a = '';
ERROR 1292 (22007): invalid time format: ''

What is changed and how it works?

  1. Return warning when sql_mode is not strict in builtinCastStringAsTime.
  2. Return null when any error or warning happens in builtinCastStringAsTime.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

It's really hard to make all the behavior to be compatiable with MySQL when cast a string as datetime. The results of MySQL are different between implicit cast and explict cast.
e.g.

mysql> set @@sql_mode='';                                                           Query OK, 0 rows affected (0.00 sec)

mysql> select year("20123-12-12");
+---------------------+
| year("20123-12-12") |
+---------------------+
|                NULL | -- The implicit cast result of "20123-12-12" is NULL.
+---------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+-----------------------------------------+
| Level   | Code | Message                                 |
+---------+------+-----------------------------------------+
| Warning | 1292 | Incorrect datetime value: '20123-12-12' | 
+---------+------+-----------------------------------------+
1 row in set (0.00 sec)

mysql> select year(cast("20123-12-12" as datetime));
+---------------------------------------+
| year(cast("20123-12-12" as datetime)) |
+---------------------------------------+
|                                  NULL | -- The explicit cast result is NULL.
+---------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql> update t set a = "20123-12-12";
Query OK, 0 rows affected, 1 warning (0.00 sec)
Rows matched: 1  Changed: 0  Warnings: 1

mysql> select * from t;
+---------------------+
| a                   |
+---------------------+
| 0000-00-00 00:00:00 | -- The implicit cast result of "20123-12-12" is ZeroDatetime.
+---------------------+
1 row in set (0.00 sec)

mysql> update t set a=cast("20123-12-12" as datetime);
Query OK, 1 row affected, 1 warning (0.01 sec)
Rows matched: 1  Changed: 1  Warnings: 1

mysql> select * from t;
+------+
| a    |
+------+
| NULL | -- The explicit cast result is NULL.
+------+
1 row in set (0.00 sec)

We return NULL here despite of implict cast or explicit cast. Thus it may break some backward compatibility with MySQL.
e.g.

tidb> select unix_timestamp('20102-12-12');
+-------------------------------+
| unix_timestamp('20102-12-12') |
+-------------------------------+
|                          NULL | -- MySQL returns 0 here
+-------------------------------+
1 row in set (0.00 sec)

Maybe we need to check the warning as this comment mentioned.

  1. Some compatibility problems are still not resolved, like the different error message between implicit cast and explicit cast.
    e.g.
MySQL(root@127.0.0.1:test) > set @@sql_mode='';
Query OK, 0 rows affected (0.00 sec)

MySQL(root@127.0.0.1:test) > insert into t values(cast('' as datetime));                                                                                                                                                                                                                      Query OK, 1 row affected, 1 warning (0.01 sec)

MySQL(root@127.0.0.1:test) > show warnings;
+---------+------+------------------------------+
| Level   | Code | Message                      |
+---------+------+------------------------------+
| Warning | 1292 | Incorrect datetime value: '' | 
+---------+------+------------------------------+
1 row in set (0.00 sec)

MySQL(root@127.0.0.1:test) > insert into t values('');
Query OK, 1 row affected, 1 warning (0.02 sec)

MySQL(root@127.0.0.1:test) > show warnings;
+---------+------+----------------------------------------+
| Level   | Code | Message                                |
+---------+------+----------------------------------------+
| Warning | 1265 | Data truncated for column 'a' at row 1 |
+---------+------+----------------------------------------+
1 row in set (0.00 sec)

Related changes


This change is Reviewable

@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu
Copy link
Contributor Author

PTAL @tiancaiamao @zz-jason

expression/builtin_cast.go Outdated Show resolved Hide resolved
@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Dec 4, 2018

PTAL @tiancaiamao @winoros

expression/builtin_cast.go Outdated Show resolved Hide resolved
@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Dec 4, 2018

PTAL @zz-jason @winoros @tiancaiamao

expression/builtin_cast.go Outdated Show resolved Hide resolved
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -1175,13 +1175,13 @@ func (b *builtinCastStringAsTimeSig) evalTime(row chunk.Row) (res types.Time, is
sc := b.ctx.GetSessionVars().StmtCtx
res, err = types.ParseTime(sc, val, b.tp.Tp, b.tp.Decimal)
if err != nil {
return res, false, errors.Trace(err)
return types.Time{}, true, handleInvalidTimeError(b.ctx, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happen if b.tp.Tp is mysql.TypeTimestamp? 00-00-00 00:00:00 seems not a legal timestamp value ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the return value is either a NULL or an error. The returned types.Time{} is never used.

@zz-jason
Copy link
Member

zz-jason commented Dec 6, 2018

@tiancaiamao @winoros @eurekaka PTAL

@tiancaiamao
Copy link
Contributor

LGTM

@zz-jason zz-jason merged commit fde9c72 into pingcap:master Dec 6, 2018
@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 6, 2018
@zz-jason
Copy link
Member

zz-jason commented Dec 6, 2018

should we cherry-pick this commit to release-2.1? @XuHuaiyu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants