expression: fix errors set utc_timestamp precision | tidb-test=pr/2406

[chagelo]

What problem does this PR solve?

Issue Number: close #56451

Problem Summary:

What changed and how does it work?

Correct the error when set invalid precision for utc_timestamp

Check List

Tests

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

  • I checked and no code files have been changed.

Side effects

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

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix errors when set invalid precision of date or time related functions, such as too big precision.

[sre-bot]

All committers have signed the CLA.

[ti-chi-bot]

Welcome @chagelo!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to pingcap/tidb. 😃

[ti-chi-bot]

Hi @chagelo. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[tiprow]

Hi @chagelo. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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-sigs/prow repository.

[chagelo]

/ok-to-test

[ti-chi-bot]

@chagelo: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

[tiprow]

@chagelo: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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-sigs/prow repository.

[mjonss]

/ok-to-test

[mjonss]

Looks good, just missing the statement in the .test file.

[codecov]

Codecov Report

Attention: Patch coverage is 38.46154% with 24 lines in your changes missing coverage. Please review.

Project coverage is 56.3106%. Comparing base (e017e1b) to head (cb64e84).
Report is 17 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56453         +/-   ##
=================================================
- Coverage   73.3414%   56.3106%   -17.0309%     
=================================================
  Files          1629       1753        +124     
  Lines        449976     633500     +183524     
=================================================
+ Hits         330019     356728      +26709     
- Misses        99734     252786     +153052     
- Partials      20223      23986       +3763     

Flag	Coverage Δ
integration	36.8174% <30.7692%> (?)
unit	72.4452% <38.4615%> (-0.0277%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	52.9478% <ø> (ø)
parser	∅ <ø> (∅)
br	52.3614% <ø> (+6.7983%)	⬆️

[chagelo]

Thank you very much for the suggestions!

And I'm gonna fix the rest errors tomorrow.😀

[mjonss]

Looks good to me!

I have re-recorded the test results for the failing CI test idc-jenkins-ci-tidb/mysql-test, which is a private repository for various integration tests and mysql compatibility tests. so hopefully the tests will pass.

[mjonss]

/retest

[mjonss]

/retest

[mjonss]

/run-check-issue-triage-complete

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2024-10-05 23:47:21.004009259 +0000 UTC m=+744196.424222285: ☑️ agreed by mjonss.
• 2024-10-07 05:32:37.699472883 +0000 UTC m=+851313.119685895: ☑️ agreed by dveeden.

[dveeden]

There are a few places that check for a fsp that's less than MinFsp. But looks like there is no way to reach that error as the parser is not allowing this.

        } else if fsp > int64(types.MaxFsp) {
                return types.ZeroTime, true, types.ErrTooBigPrecision.GenWithStackByArgs(fsp, "now", types.MaxFsp)
        } else if fsp < int64(types.MinFsp) {
                return types.ZeroTime, true, errors.Errorf("Invalid negative %d specified, must in [0, 6]", fsp)
        }

Maybe:

• Remove the check for fsp < MinFsp
• Add a comment to indicate that this should never be reached and/or change to message to indicate this
• Add tests for things like NOW(-1) to ensure it gets rejected by the parser

Side note: Looks like there is a MySQL bug in the error message that we don't follow:

MySQL 9.0.1:

sql> SELECT NOW(99999);
ERROR: 1426 (42000): Too-big precision 159 specified for 'now'. Maximum is 6.

TiDB:

sql> SELECT NOW(99999);
ERROR: 1426 (42000): Too-big precision 99999 specified for 'now'. Maximum is 6.

[dveeden]

For the bug in MySQL:
https://bugs.mysql.com/bug.php?id=116307

[dveeden]

Inspired by the MySQL bug it looks like we have a similar bug (but way less severe). This also answers my question about negative fsp's somewhat:

sql> SELECT SYSDATE(999999999999999999);
ERROR: 1426 (42000): Too-big precision 999999999999999999 specified for 'sysdate'. Maximum is 6.

sql> SELECT SYSDATE(9999999999999999999);
ERROR: 1105 (HY000): Invalid negative -8446744073709551617 specified, must in [0, 6]

[chagelo]

1. For MySQL, it uses the last byte in binary representation as the precision, so SELECT NOW(257) equals to SELECT NOW(1).
2. And if precision has more than 32 bits like 2147483647 + 1, it return a SQL syntax error.
3. I'll add a comment.

[dveeden]

MariaDB seems to be more strict than MySQL here. Looks like MariaDB accepts larger numbers in the parser but checks them later on.

mysql-11.4.2-MariaDB-ubu2404> SELECT NOW(9999999999999999999);
ERROR 1426 (42000): Too big precision specified for 'current_timestamp'. Maximum is 6
mysql-11.4.2-MariaDB-ubu2404> SELECT NOW(99999999999999999999);
ERROR 1064 (42000): Only integers allowed as number here near '99999999999999999999)' at line 1

[chagelo]

/retest

[dveeden]

@chagelo are the tests failing for something related to this PR?

[chagelo]

@dveeden yes, it is.

1. Because MySQL has the bug, I don't know what should I do to make it be compatible with MySQL.
2. I read the parser code. I want parser to limit precision's type like what MySQL does, but I don't understand the yyParseTab, it seems complicated.
3. It seems not good to throw a syntax error in pkg/expression/builtin_time.go(like negative or too long number precision), it should done by parser, it's right? I'm not sure about this.

[dveeden]

@dveeden yes, it is.

1. Because MySQL has the bug, I don't know what should I do to make it be compatible with MySQL.

I don't think we should follow the behavior of https://bugs.mysql.com/bug.php?id=116307 as I don't think people are relying on this behavior or should rely on this behavior.

2. I read the parser code. I want parser to limit precision's type like what MySQL does, but I don't understand the yyParseTab, it seems complicated.

yyParseTab is in pkg/parser/parser.go which is generated by goyacc based on pkg/parser/parser.y, so you need to ecit parser.y and run make parser.

3. It seems not good to throw a syntax error in pkg/expression/builtin_time.go(like negative or too long number precision), it should done by parser, it's right? I'm not sure about this.

I'm not sure. One of the issues with having the parser deal with things is that it might not have the right context to fill in the fields of the error message (e.g. the name of the table or the name of the function).

[dveeden]

From the issue description:

• error code is different
• sqlstate is different
• function name is not correct.

It looks like these are all fixed.

The following might be seen as a separate bug if it is not a regression:

mysql-8.0.11-TiDB-v8.4.0-alpha-326-ga2b0d7b115> SELECT UTC_TIMESTAMP(9990);
+----------------------------+
| UTC_TIMESTAMP(9990)        |
+----------------------------+
| 2024-10-14 07:12:01.054447 |
+----------------------------+
1 row in set (0.00 sec)

Note that this is somewhat similar to #46372 (comment)

[chagelo]

/retest

[chagelo]

SELECT CURRENT_TIME(-1);
Error 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 21 near "-1);" 
SELECT CURRENT_TIME(2147483648);
Error 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use

It seems TiDB parser handles the negative precision, but not resolve the overlong number precision, parser should have dealt with this, because this error contains some context.

[chagelo]

@dveeden Required review. Thanks.