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: rewrite builtin function: YEAR, MONTH #4210

Merged
merged 11 commits into from
Aug 22, 2017
Merged

Conversation

zz-jason
Copy link
Member

to #4080

d, err = convertToTime(b.ctx.GetSessionVars().StmtCtx, args[0], mysql.TypeDate)
if err != nil || d.IsNull() {
return d, errors.Trace(err)
if isNull || err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like isNull has a higher priority for these functions.

if isNull {
return
}
if err {
appendwarning(err)
return
}

may be more clear?

Copy link
Member Author

Choose a reason for hiding this comment

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

in mysql:

MySQL > create table t(a bigint);
Query OK, 0 rows affected (0.02 sec)

MySQL > insert into t select year("aa");
ERROR 1292 (22007): Incorrect datetime value: 'aa'
MySQL > select year("aa");
+------------+
| year("aa") |
+------------+
|       NULL |
+------------+
1 row in set, 1 warning (0.00 sec)

seems mysql will throw this error during insert/update/delete, it will be solved in #4208

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

Copy link
Member

@jackysp jackysp Aug 18, 2017

Choose a reason for hiding this comment

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

mysql> set sql_mode = '';
Query OK, 0 rows affected, 1 warning (0.00 sec)
mysql> create table t (a bigint);
Query OK, 0 rows affected (0.01 sec)
mysql> insert into t select year("aa");
Query OK, 1 row affected, 1 warning (0.01 sec)
Records: 1  Duplicates: 0  Warnings: 1
mysql> show warnings;
+---------+------+--------------------------------+
| Level   | Code | Message                        |
+---------+------+--------------------------------+
| Warning | 1292 | Incorrect datetime value: 'aa' |
+---------+------+--------------------------------+
1 row in set (0.00 sec)

It seems it relies on sql_mode in mysql.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@zz-jason
Copy link
Member Author

@jackysp @XuHuaiyu PTAL

@@ -167,28 +170,31 @@ func (s *testEvaluatorSuite) TestDate(c *C) {
YearWeek interface{}
}{
{nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil},
{"0000-00-00", int64(0), int64(0), nil, int64(0), nil, nil, nil, nil, nil, nil, nil},
{"0000-00-00 00:00:00", nil, nil, nil, int64(0), nil, nil, nil, nil, nil, nil, nil},
Copy link
Member

Choose a reason for hiding this comment

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

Why change this instead of add a test?

Copy link
Member Author

Choose a reason for hiding this comment

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

added back.

@@ -943,6 +943,46 @@ func (s *testIntegrationSuite) TestTimeBuiltin(c *C) {
result := tk.MustQuery("select makedate(a,a), makedate(b,b), makedate(c,c), makedate(d,d), makedate(e,e), makedate(f,f), makedate(null,null), makedate(a,b) from t")
result.Check(testkit.Rows("2001-01-01 2001-01-01 <nil> <nil> <nil> 2021-01-21 <nil> 2001-01-01"))

// for year
result = tk.MustQuery(`select year("2013-01-09"), year("2013-00-09"), year("000-01-09"), year("1-01-09"), year("20131-01-09"), year(null);`)
Copy link
Contributor

Choose a reason for hiding this comment

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

add test for "2013-00-00", "2013-00-00 00:00:00", "0000-00-00 12:12:12", "2017-00-00 12:12:12"
so as month

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu XuHuaiyu added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 22, 2017
@zz-jason
Copy link
Member Author

@jackysp PTAL

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp merged commit 9372c6a into master Aug 22, 2017
@jackysp jackysp deleted the timefunc_year branch August 22, 2017 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants