expression: compatibility is changed #30653

Tangruilin · 2021-12-12T11:38:18Z

What problem does this PR solve?

Issue Number: close #19150

Problem Summary:

What is changed and how it works?

After ParseDuration in builtinAddStringAndStringSig.evalString , format like *-* will be checked，if the arg1Strmatch this format, it will return isNull

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2021-12-12T11:38:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

dveeden
zimulala

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

sre-bot · 2021-12-12T11:38:55Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

*: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

Tangruilin · 2021-12-12T12:07:10Z

/run-check_title

sre-bot · 2021-12-12T12:07:35Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

*: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

Tangruilin · 2021-12-12T12:12:14Z

/run-check_title

Tangruilin · 2021-12-12T12:13:27Z

/run-check_dev

Tangruilin · 2021-12-12T12:14:06Z

/run-unit-test

Tangruilin · 2021-12-12T12:27:42Z

when I pull the code, there are 4 test in expression cannot pass

Tangruilin · 2021-12-15T05:44:06Z

/cc @gregwebs Could you help look at it, I do not know why I cannot pass unit_test, and I pull the code without my PR then do make gotest, it will failed, too

Tangruilin · 2021-12-16T07:58:55Z

/cc @foobar Could someone look at it? emmmm....

zimulala

[2021-12-12T13:04:46.677Z] --- FAIL: TestTimeBuiltin (0.51s)
[2021-12-12T13:04:46.677Z]     result.go:50:
[2021-12-12T13:04:46.677Z]         	Error Trace:	result.go:50
 	            				integration_serial_test.go:2170
[2021-12-12T13:04:46.677Z]         	Error:      	Not equal:
[2021-12-12T13:04:46.677Z]         	            	expected: "[2017-01-01 01:01:06 2017-01-01 01:00:56 2017-01-01 01:01:01 2017-01-01 01:01:02.340000]\n"
[2021-12-12T13:04:46.677Z]         	            	actual  : "[2017-01-01 01:01:06 <nil> 2017-01-01 01:01:01 2017-01-01 01:01:02.340000]\n"

How about the case like the test in this TestTimeBuiltin.

mysql> select addtime('2017-01-01 01:01:01', -5);
+------------------------------------+
| addtime('2017-01-01 01:01:01', -5) |
+------------------------------------+
| 2017-01-01 01:00:56                |
+------------------------------------+
1 row in set (0.01 sec)

Tangruilin · 2021-12-16T13:56:22Z

/run-check_dev_2

sre-bot · 2021-12-21T06:22:12Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/4631a1c23ab65c45a9b02e291786597be410027b

dveeden

This seems to fix the issue described.

Note that there are still some other corner cases:

MySQL 8.0.27:

mysql> select ADDTIME('2020-05-13 14:01:24', '-10000000');
+---------------------------------------------+
| ADDTIME('2020-05-13 14:01:24', '-10000000') |
+---------------------------------------------+
| 2020-04-08 15:01:25                         |
+---------------------------------------------+
1 row in set, 1 warning (0.00 sec)

Warning (Code 1292): Truncated incorrect time value: '-10000000'

TiDB:

sql> select ADDTIME('2020-05-13 14:01:24', '-10000000');
+---------------------------------------------+
| ADDTIME('2020-05-13 14:01:24', '-10000000') |
+---------------------------------------------+
| NULL                                        |
+---------------------------------------------+
1 row in set, 1 warning (0.0007 sec)
Warning (code 1292): Truncated incorrect time value: '-10000000'

lance6716 · 2021-12-21T07:33:37Z

ping @zimulala @bb7133

dveeden · 2021-12-21T07:35:40Z

expression/builtin_time.go

@@ -5556,6 +5557,16 @@ func (b *builtinAddStringAndStringSig) evalString(row chunk.Row) (result string,
 		}
 		return "", true, err
 	}
+
+	check := arg1Str


I think it would be useful to add a comment here about why this is needed

Yeah, In fact I think add the code in func isDuration or create a new func maybeErrDuration may be butter, but isDuration is a util func, I'm afraid to get new wrong, so I add the code here. I may add some cmment or create a new func, but which file may i add the func?

I will check the other concor case and try to solve them

Tangruilin · 2022-01-03T14:12:08Z

ping @zimulala @bb7133

zimulala

LGTM

zimulala · 2022-01-05T04:37:39Z

/merge

ti-chi-bot · 2022-01-05T04:37:42Z

This pull request has been accepted and is ready to merge.

Commit hash: 585d649

ti-chi-bot · 2022-01-05T04:37:54Z

@Tangruilin: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

fix pingcap#19150

52c5443

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 12, 2021

ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 12, 2021

Tangruilin changed the title ~~pkg expression is changed~~ pkg expression: compatibility is changed Dec 12, 2021

Tangruilin changed the title ~~pkg expression: compatibility is changed~~ expression: compatibility is changed Dec 12, 2021

fix pingcap#19150

0de8e9a

zimulala reviewed Dec 16, 2021

View reviewed changes

fix pingcap#19150

6d429d9

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 16, 2021

Tangruilin requested a review from zimulala December 16, 2021 14:09

fix pingcap#19150

e3adf9a

Tangruilin force-pushed the master branch from e31e4d2 to e3adf9a Compare December 16, 2021 14:41

Merge branch 'master' into master

567923d

Tangruilin mentioned this pull request Dec 21, 2021

Incompatibility with MySQL for ADDTIME() #19150

Closed

dveeden approved these changes Dec 21, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 21, 2021

dveeden reviewed Dec 21, 2021

View reviewed changes

Merge branch 'master' into master

585d649

zimulala approved these changes Jan 5, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 5, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 5, 2022

Merge branch 'master' into master

4631a1c

ti-chi-bot merged commit 1a6564b into pingcap:master Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: compatibility is changed #30653

expression: compatibility is changed #30653

Tangruilin commented Dec 12, 2021 •

edited

Loading

ti-chi-bot commented Dec 12, 2021 •

edited

Loading

sre-bot commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

sre-bot commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 15, 2021

Tangruilin commented Dec 16, 2021

zimulala left a comment •

edited

Loading

Tangruilin commented Dec 16, 2021

sre-bot commented Dec 21, 2021 •

edited

Loading

dveeden left a comment

lance6716 commented Dec 21, 2021

dveeden Dec 21, 2021

Tangruilin Dec 21, 2021

Tangruilin Dec 21, 2021

Tangruilin commented Jan 3, 2022

zimulala left a comment

zimulala commented Jan 5, 2022

ti-chi-bot commented Jan 5, 2022

ti-chi-bot commented Jan 5, 2022

expression: compatibility is changed #30653

expression: compatibility is changed #30653

Conversation

Tangruilin commented Dec 12, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Dec 12, 2021 • edited Loading

sre-bot commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

sre-bot commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 12, 2021

Tangruilin commented Dec 15, 2021

Tangruilin commented Dec 16, 2021

zimulala left a comment • edited Loading

Choose a reason for hiding this comment

Tangruilin commented Dec 16, 2021

sre-bot commented Dec 21, 2021 • edited Loading

dveeden left a comment

Choose a reason for hiding this comment

lance6716 commented Dec 21, 2021

dveeden Dec 21, 2021

Choose a reason for hiding this comment

Tangruilin Dec 21, 2021

Choose a reason for hiding this comment

Tangruilin Dec 21, 2021

Choose a reason for hiding this comment

Tangruilin commented Jan 3, 2022

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Jan 5, 2022

ti-chi-bot commented Jan 5, 2022

ti-chi-bot commented Jan 5, 2022

Tangruilin commented Dec 12, 2021 •

edited

Loading

ti-chi-bot commented Dec 12, 2021 •

edited

Loading

zimulala left a comment •

edited

Loading

sre-bot commented Dec 21, 2021 •

edited

Loading