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: QUARTER/DATE_FORMAT compatibility with mysql for 0/0.0 values #12488

Merged
merged 63 commits into from
Nov 9, 2019

Conversation

ekalinin
Copy link
Contributor

@ekalinin ekalinin commented Sep 29, 2019

What problem does this PR solve?

Partial fix for the #11203 (+ #11223 as a global issue, + #9728)

What is changed and how it works?

Added corner cases into builtinCastIntAsTimeSig/builtinCastDecimalAsTimeSig.
If the original value is 0 or 0.0 then it converted as empty Time struct (not a NULL value).

builtinQuarterSig.evalInt checks if we have zero date and it's not not a NULL after casting then we return 0, not NULL.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Code changes

  • internal methods only

Side effects

  • Breaking backward compatibility?

Related changes

  • Need to update the documentation

Release note

  • Write release note for bug-fix.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Sep 29, 2019
@ekalinin
Copy link
Contributor Author

TiDB now works for 0 | 0.0 as mysql:
Schema (MySQL v5.7)


Query #1

select quarter(0), quarter(0.0), quarter("0000-00-00"), quarter("0000-00-00 00:00:00");
quarter(0) quarter(0.0) quarter("0000-00-00") quarter("0000-00-00 00:00:00")
0 0 null null

Query #2

show warnings;
Level Code Message
Warning 1292 Incorrect datetime value: '0000-00-00'
Warning 1292 Incorrect datetime value: '0000-00-00 00:00:00'

View on DB Fiddle

@qw4990
Copy link
Contributor

qw4990 commented Sep 30, 2019

/run-unit-test

@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

Merging #12488 into master will increase coverage by 0.5494%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #12488        +/-   ##
================================================
+ Coverage   80.1169%   80.6663%   +0.5494%     
================================================
  Files           469        469                
  Lines        112045     114500      +2455     
================================================
+ Hits          89767      92363      +2596     
+ Misses        15320      15207       -113     
+ Partials       6958       6930        -28

Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

Hi, thank you for the pr. I've left some comments, PTAL.

expression/builtin_cast.go Outdated Show resolved Hide resolved
expression/integration_test.go Outdated Show resolved Hide resolved
@ekalinin ekalinin changed the title expression: better 'quarter' compatibility with mysql expression: QUARTER/DATE_FORMAT compatibility with mysql for 0/0.0 values Oct 3, 2019
@ekalinin
Copy link
Contributor Author

ekalinin commented Oct 3, 2019

It looks like DATE_FORMAT is already fixed:

Schema (MySQL v5.7)

mysql> 
    select DATE_FORMAT(0, '%W'), 
           DATE_FORMAT(0.0, '%W'), 
           DATE_FORMAT('0', '%W'), 
           DATE_FORMAT('0.0', '%W'),
           DATE_FORMAT('0000-00-00', '%W');

| DATE_FORMAT(0, '%W') | DATE_FORMAT(0.0, '%W') | DATE_FORMAT('0', '%W') | DATE_FORMAT('0.0', '%W') | DATE_FORMAT('0000-00-00', '%W') |
| -------------------- | ---------------------- | ---------------------- | ------------------------ | ------------------------------- |
|                 NULL |                   NULL |                   NULL |                     NULL |                            NULL |

mysql>    show warnings;

| Level   | Code | Message                                |
| ------- | ---- | -------------------------------------- |
| Warning | 1292 | Incorrect datetime value: '0'          |
| Warning | 1292 | Incorrect datetime value: '0.0'        |
| Warning | 1292 | Incorrect datetime value: '0000-00-00' |

View on DB Fiddle

TiDB:

mysql> select DATE_FORMAT(0, '%W'), DATE_FORMAT(0.0, '%W'), DATE_FORMAT('0', '%W'), DATE_FORMAT('0.0', '%W'), DATE_FORMAT('0000-00-00', '%W %M');
+----------------------+------------------------+------------------------+--------------------------+------------------------------------+
| DATE_FORMAT(0, '%W') | DATE_FORMAT(0.0, '%W') | DATE_FORMAT('0', '%W') | DATE_FORMAT('0.0', '%W') | DATE_FORMAT('0000-00-00', '%W %M') |
+----------------------+------------------------+------------------------+--------------------------+------------------------------------+
| NULL                 | NULL                   | NULL                   | NULL                     | NULL                               |
+----------------------+------------------------+------------------------+--------------------------+------------------------------------+
1 row in set, 5 warnings (0.00 sec)

mysql> show warnings;
+---------+------+--------------------------------------------------------+
| Level   | Code | Message                                                |
+---------+------+--------------------------------------------------------+
| Warning | 1292 | Incorrect datetime value: '0000-00-00 00:00:00'        |
| Warning | 1292 | Incorrect datetime value: '0000-00-00 00:00:00'        |
| Warning | 1292 | invalid time format: '0'                               |
| Warning | 1292 | invalid time format: '0.0'                             |
| Warning | 1292 | Incorrect datetime value: '0000-00-00 00:00:00.000000' |
+---------+------+--------------------------------------------------------+
5 rows in set (0.00 sec)

So i just fixed warnings for 0/0.0 values and added more tests.

@ekalinin
Copy link
Contributor Author

ekalinin commented Oct 3, 2019

PTAL

Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the late reply, we've taken a long vacation and back to work now.
I've left some comments again, PTAL. Thanks.

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

0.0* fixed.

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

@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2019

Your auto merge job has been accepted, waiting for 13014, 12827, 13181, 13088

@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2019

@ekalinin merge failed.

@sykp241095
Copy link
Member

/run-unit-test

@ngaut ngaut merged commit b1aad07 into pingcap:master Nov 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 9, 2019

cherry pick to release-3.1 failed

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1 release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @SunRunAway PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants