expression: handle zero date in `date_add()` #20015

zhangysh1995 · 2020-09-15T12:38:23Z

What problem does this PR solve?

Issue Number: close #15234

Problem Summary:

Invalid zero date could be processed

mysql> select date_add('0000-00-00 00:00:00', interval 2020 year);
+-----------------------------------------------------+
| date_add('0000-00-00 00:00:00', interval 2020 year) |
+-----------------------------------------------------+
| 2019-11-30 00:00:00                                 |
+-----------------------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+-----------------------------------------+
| Level   | Code | Message                                 |
+---------+------+-----------------------------------------+
| Warning | 1292 | Incorrect time value: '{0 0 0 0 0 0 0}' |
+---------+------+-----------------------------------------+
1 row in set (0.00 sec)

What is changed and how it works?

What's Changed:

Return NULL when the first date is invalid

How it Works:

Add a invalid check to how related functions

        ...
	if invalidDate := date.InvalidZero(); invalidDate {
		err = handleInvalidTimeError(b.ctx, types.ErrWrongValue.GenWithStackByArgs(types.TimeStr, date.String()))
		return types.ZeroTime, true, err
	}
        ...

Now it becomes:

mysql> select date_add('0000-00-00 00:00:00', interval 2020 year);
+-----------------------------------------------------+
| date_add('0000-00-00 00:00:00', interval 2020 year) |
+-----------------------------------------------------+
| NULL                                                |
+-----------------------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+----------------------------------------------------+
| Level   | Code | Message                                            |
+---------+------+----------------------------------------------------+
| Warning | 1292 | Incorrect time value: '0000-00-00 00:00:00.000000' |
+---------+------+----------------------------------------------------+
1 row in set (0.00 sec

Think a refactor of this patch is necessary, because I use paste now.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:

Check List

Tests

Unit test

Release note

DATE_ADD is compatible with MySQL with handling zero date

sre-bot · 2020-09-15T12:38:34Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

zhangysh1995 · 2020-09-15T12:46:57Z

/rebuild

sre-bot · 2020-09-15T12:47:04Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

zhouqiang-cl · 2020-09-15T13:10:39Z

/rebuild

zhangysh1995 · 2020-09-22T08:02:04Z

@ichn-hu Could you please have a look at the code?

rebelice

Could you please add some tests for this PR?

ichn-hu · 2020-09-22T09:09:33Z

@ichn-hu Could you please have a look at the code?

Sure, sorry for the delay, I don't see any thing wrong, but would you please as @rebelice said add some test which reflects both timestampdiff and date_add returned NULL on the given test case?

ti-challenge-bot · 2020-09-23T10:20:01Z

There is no reward for this challenge pull request, so you can request a reward from @XuHuaiyu.

More

Tip : About reward you can refs to reward-command.

Warning: None

ti-challenge-bot · 2020-09-23T10:51:09Z

There is no reward for this challenge pull request, so you can request a reward from @XuHuaiyu.

More

Tip : About reward you can refs to reward-command.

Warning: None

zhangysh1995 · 2020-09-23T11:38:18Z

I found that when the second argument is invalid, TiDB miss an warning:

-- mysql 5.7
mysql> select adddate("2018-08-16 20:21:01", "-00:00:00.000000");
+----------------------------------------------------+
| adddate("2018-08-16 20:21:01", "-00:00:00.000000") |
+----------------------------------------------------+
| 2018-08-16 20:21:01                                |
+----------------------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+-------------------------------------------------------+
| Level   | Code | Message                                               |
+---------+------+-------------------------------------------------------+
| Warning | 1292 | Truncated incorrect INTEGER value: '-00:00:00.000000' |
+---------+------+-------------------------------------------------------+
1 row in set (0.00 sec)

-- TiDB
mysql> select adddate("2018-08-16 20:21:01", "-00:00:00.000000");
+----------------------------------------------------+
| adddate("2018-08-16 20:21:01", "-00:00:00.000000") |
+----------------------------------------------------+
| 2018-08-16 20:21:01                                |
+----------------------------------------------------+
1 row in set (0.00 sec)

Because this is not related to the current issue, I will not fix it here.

ti-challenge-bot · 2020-09-23T11:38:21Z

There is no reward for this challenge pull request, so you can request a reward from @XuHuaiyu.

More

Tip : About reward you can refs to reward-command.

Warning: None

ti-challenge-bot · 2020-09-23T11:38:24Z

There is no reward for this challenge pull request, so you can request a reward from @XuHuaiyu.

More

Tip : About reward you can refs to reward-command.

Warning: None

zhangysh1995 · 2020-09-23T11:39:28Z

@XuHuaiyu What would be a possible reward for this issue?

ti-challenge-bot · 2020-09-23T12:27:57Z

There is no reward for this challenge pull request, so you can request a reward from @XuHuaiyu.

More

Tip : About reward you can refs to reward-command.

Warning: None

ti-challenge-bot · 2020-09-23T12:45:44Z

There is no reward for this challenge pull request, so you can request a reward from @XuHuaiyu.

More

Tip : About reward you can refs to reward-command.

Warning: None

zhangysh1995 · 2020-09-23T12:51:29Z

@rebelice @ichn-hu Added some tests~ Basically, I didn't see a test for date_add so I created a new one.

ti-challenge-bot · 2020-09-23T12:51:55Z

There is no reward for this challenge pull request, so you can request a reward from @XuHuaiyu.

More

Tip : About reward you can refs to reward-command.

Warning: None

XuHuaiyu · 2020-09-25T10:25:28Z

/reward 300

ti-challenge-bot · 2020-09-25T10:25:33Z

Reward success.

XuHuaiyu · 2020-09-27T03:23:45Z

expression/builtin_time.go

@@ -3416,6 +3416,11 @@ func (b *builtinAddDateStringStringSig) evalTime(row chunk.Row) (types.Time, boo
 		return types.ZeroTime, true, err


VecEvalTime should also be modified, you can check the following example in MySQL and TiDB

set @@sql_mode=''; create table t(a datetime); insert into t values('0000-00-00 00:00:00'); select date_add(a, interval 2020 year) from t;

But it'll be ok to fix this in another PR.

@XuHuaiyu @SunRunAway I think this vectorized function should also be fixed, but it may be too many changes for a single PR. To have a cleaner handling of this issue, a refactor of the code may be necessary. For example, I put the valid check in all the add_date functions, but I saw that there is little differences among all the implemented functions. If you think it is better to fix it in the same PR, I will do it. But I will prefer to open a new one for it.

SunRunAway · 2020-09-25T11:21:19Z

expression/builtin_time.go

@@ -3449,6 +3454,11 @@ func (b *builtinAddDateStringIntSig) evalTime(row chunk.Row) (types.Time, bool,
 		return types.ZeroTime, true, err
 	}

+	if invalidDate := date.InvalidZero(); invalidDate {


Is it better to be incorporated into b.getDateFromString?

I think it will help to remove the redundant check in the caller function, but it depends on the context to determine whether zeroDate is valid. For example, adddate('0000-00-00', interval 1 day) is invalid becuase the first zero, while adddate('2000-10-11', 0) is valid with the second argument as a zero. It should be better leave to the caller function, but my current fix is not elegant.

SunRunAway · 2020-09-25T11:21:54Z

expression/builtin_time.go

@@ -3449,6 +3454,11 @@ func (b *builtinAddDateStringIntSig) evalTime(row chunk.Row) (types.Time, bool,
 		return types.ZeroTime, true, err
 	}

+	if invalidDate := date.InvalidZero(); invalidDate {


Should this change be ported into vectorized implementation like builtinAddDateStringIntSig.vecEvalTime? And add more test cases for vectorized situation?

XuHuaiyu · 2020-09-28T03:24:11Z

I think that handle the vec-cases in another PR is acceptable.

zhangysh1995 · 2020-11-13T08:42:31Z

/pick-up

ti-challenge-bot · 2020-11-13T08:42:35Z

It is not a pickable issue!

Details

Tip : If you want this issue to be picked, you need to add a challenge-program label to it.

Warning: None

ichn-hu

LGTM

wshwsh12 · 2020-12-23T02:55:29Z

In master branch 49791bc,

[tidb]> select date_add('0000-00-00 00:00:00', interval 2020 year);
+-----------------------------------------------------+
| date_add('0000-00-00 00:00:00', interval 2020 year) |
+-----------------------------------------------------+
| NULL                                                |
+-----------------------------------------------------+
1 row in set, 1 warning (0.000 sec)

But the following case hasn't fixed.

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

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

[tidb]> insert into t values('0000-00-00 00:00:00');
Query OK, 1 row affected (0.003 sec)

[tidb]> select date_add(a, interval 2020 year) from t;
+---------------------------------+
| date_add(a, interval 2020 year) |
+---------------------------------+
| 2019-11-30 00:00:00             |
+---------------------------------+
1 row in set, 1 warning (0.001 sec)

wshwsh12

Why not add error handle in getDateFromXXXXXX ang vecGetDateFromXXXXXX? For your comment, #20015 (comment), I can't get your point.
The second argument will convert to interval, not datetime. So zero value will not cause error..

zhangysh1995 · 2020-12-23T07:23:57Z

It has been a while since I submitted this PR, I will take a look recently.

zz-jason · 2021-03-27T14:06:07Z

@zhangysh1995 any update?

zhangysh1995 · 2021-03-27T15:24:19Z

@zhangysh1995 any update?

I think a clarification on the scope of this PR is needed. First I want to fix for the non-vectorization case, then the vectorized case is required. Later because the master is fixed, and the latest review requires a refactor of my fix. I think I need a clear goal now for this PR. If a refactor and the additional feature should be fixed, I would consider close this one and open another. @wshwsh12 @XuHuaiyu What are your comments?

tisonkun · 2021-07-29T08:23:30Z

@zhangysh1995 from @wshwsh12 's comment I think this PR doesn't fix the issue. There are still two unanswered questions:

You can update the PR to fix the specific issue reported in #15234, regardless any further requirement.

ti-chi-bot · 2021-10-30T10:48:44Z

@zhangysh1995: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

zhangysh1995 · 2022-01-24T08:18:05Z

需求变动比较大，现在没精力改这个 PR 了，希望重新 assign 。

zhangysh1995 requested a review from a team as a code owner September 15, 2020 12:38

zhangysh1995 requested review from XuHuaiyu and removed request for a team September 15, 2020 12:38

ti-srebot added the contribution This PR is from a community contributor. label Sep 15, 2020

github-actions bot added the component/expression label Sep 15, 2020

XuHuaiyu requested a review from ichn-hu September 17, 2020 03:36

rebelice reviewed Sep 22, 2020

View reviewed changes

zhangysh1995 closed this Sep 23, 2020

zhangysh1995 added 2 commits September 23, 2020 18:53

types: handle zero date in data_add

f6fcf1e

test case: handle zero date in data_add

5ee7cd6

zhangysh1995 reopened this Sep 23, 2020

test case: fix params to adddate

c711228

test case: fix unit and null

c3063b9

zhangysh1995 changed the title ~~types: handle zero date in data_add~~ types: handle zero date in date_add Sep 23, 2020

ti-challenge-bot bot added the rewarded label Sep 25, 2020

XuHuaiyu added the type/bugfix This PR fixes a bug. label Sep 25, 2020

XuHuaiyu reviewed Sep 27, 2020

View reviewed changes

SunRunAway reviewed Sep 27, 2020

View reviewed changes

zz-jason changed the title ~~types: handle zero date in date_add~~ expression: handle zero date in date_add() Oct 5, 2020

ichn-hu mentioned this pull request Nov 3, 2020

Welcome to contribute #20804

Closed

ichn-hu approved these changes Nov 24, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 24, 2020

XuHuaiyu requested a review from wshwsh12 December 21, 2020 07:56

wshwsh12 reviewed Dec 23, 2020

View reviewed changes

tisonkun removed the rewarded label Sep 17, 2021

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2021

zhangysh1995 closed this Jan 24, 2022

		@@ -3416,6 +3416,11 @@ func (b *builtinAddDateStringStringSig) evalTime(row chunk.Row) (types.Time, boo
		return types.ZeroTime, true, err

expression: handle zero date in date_add() #20015

expression: handle zero date in date_add() #20015

Conversation

zhangysh1995 commented Sep 15, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

sre-bot commented Sep 15, 2020

zhangysh1995 commented Sep 15, 2020

sre-bot commented Sep 15, 2020

zhouqiang-cl commented Sep 15, 2020

zhangysh1995 commented Sep 22, 2020

rebelice left a comment

Choose a reason for hiding this comment

ichn-hu commented Sep 22, 2020

ti-challenge-bot bot commented Sep 23, 2020

ti-challenge-bot bot commented Sep 23, 2020

zhangysh1995 commented Sep 23, 2020

ti-challenge-bot bot commented Sep 23, 2020

ti-challenge-bot bot commented Sep 23, 2020

zhangysh1995 commented Sep 23, 2020

ti-challenge-bot bot commented Sep 23, 2020

ti-challenge-bot bot commented Sep 23, 2020

zhangysh1995 commented Sep 23, 2020 • edited Loading

ti-challenge-bot bot commented Sep 23, 2020

XuHuaiyu commented Sep 25, 2020

ti-challenge-bot bot commented Sep 25, 2020

XuHuaiyu Sep 27, 2020

Choose a reason for hiding this comment

zhangysh1995 Sep 27, 2020

Choose a reason for hiding this comment

SunRunAway Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

zhangysh1995 Sep 27, 2020

Choose a reason for hiding this comment

SunRunAway Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

XuHuaiyu commented Sep 28, 2020

zhangysh1995 commented Nov 13, 2020

ti-challenge-bot bot commented Nov 13, 2020

ichn-hu left a comment

Choose a reason for hiding this comment

wshwsh12 commented Dec 23, 2020 • edited Loading

wshwsh12 left a comment • edited Loading

Choose a reason for hiding this comment

zhangysh1995 commented Dec 23, 2020

zz-jason commented Mar 27, 2021

zhangysh1995 commented Mar 27, 2021

tisonkun commented Jul 29, 2021

ti-chi-bot commented Oct 30, 2021

zhangysh1995 commented Jan 24, 2022

expression: handle zero date in `date_add()` #20015

expression: handle zero date in `date_add()` #20015

zhangysh1995 commented Sep 15, 2020 •

edited

Loading

zhangysh1995 commented Sep 23, 2020 •

edited

Loading

SunRunAway Sep 25, 2020 •

edited

Loading

SunRunAway Sep 25, 2020 •

edited

Loading

wshwsh12 commented Dec 23, 2020 •

edited

Loading

wshwsh12 left a comment •

edited

Loading