expression: Timestamp literal with time zone offset #57845

mjonss · 2024-11-29T18:45:11Z

What problem does this PR solve?

Issue Number: close #51742

Problem Summary:
timestamp literal, like TIMESTAMP '2024-11-29 19:20:21' did not allow time zone offset, like TIMESTAMP '2024-11-29 19:20:21+08:00'

What changed and how does it work?

Adjusted the internal regex limiting allowed patterns for TIMESTAMP literals.

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

Release note

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

Allow time zone offset for TIMESTAMP literal.

tiprow · 2024-11-29T18:45:27Z

Hi @mjonss. 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.

codecov · 2024-11-29T19:24:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.4654%. Comparing base (b11d034) to head (dc7b358).
Report is 72 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57845        +/-   ##
================================================
+ Coverage   72.8126%   73.4654%   +0.6528%     
================================================
  Files          1677       1689        +12     
  Lines        463954     477205     +13251     
================================================
+ Hits         337817     350581     +12764     
- Misses       105277     105554       +277     
- Partials      20860      21070       +210

Flag	Coverage Δ
integration	`43.2817% <100.0000%> (?)`
unit	`72.3259% <0.0000%> (+0.1110%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (-0.0764%)`	⬇️
parser	`∅ <ø> (∅)`
br	`43.6526% <ø> (-1.8496%)`	⬇️

mjonss · 2024-11-29T19:53:59Z

pkg/expression/builtin_time.go

+		// (regardless if min/sec exists or not...)
+		`(\.\d*)?` +
+		// Optionally time zone offset, must be +/-HH:MM format
+		`([+-]\d{2}[:]\d{2})?` +


MySQL has this part very strict. It is more strict than what types.ParseTime parses the same string, so I think we should be more strict here as well, to avoid the situation where customer starts to use a more relaxed syntax, and we need to make it more strict.

Yes, I agree with that

mjonss · 2024-11-29T19:57:14Z

pkg/expression/builtin_time.go

+		// Hour is mandatory
+		// Any number of leading zeroes
+		// 1-2 Hour digits
+		`0*\d{1,2}` +


This is a fix to allow SELECT TIMESTAMP '2024-01-01 18'

mjonss · 2024-11-29T19:58:59Z

pkg/expression/builtin_time.go

+		// Any non-digit separator before Minute and Second parts
+		// Any number of leading zeroes in Min/Sec!
+		// 1-2 digit minutes/seconds
+		`([^\d]0*\d{1,2}){0,2}` +


Another fix for allowing SELECT TIMESTAMP '2024-01-01 18[:00]'. SELECT TIMESTAMP '2024-01-01 18:00:00' did already work.

…rrently.

dveeden

Should it also accept this with the ODBC syntax?

mysql-8.0.11-TiDB-v8.5.0-alpha-219-gecd70f4222> SELECT { ts '2024-01-01 14:00:00-14:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00-14:00' } |
+------------------------------------+
| 2024-01-02 05:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

Note that the MySQL behavior here is interesting:

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00+00:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00+00:00' } |
+------------------------------------+
| 2024-01-01 14:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00-00:00' };
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
1 row in set (0.00 sec)

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00+01:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00+01:00' } |
+------------------------------------+
| 2024-01-01 13:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00-01:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00-01:00' } |
+------------------------------------+
| 2024-01-01 15:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

dveeden · 2024-12-02T16:16:45Z

pkg/expression/builtin_time.go

@@ -67,7 +67,34 @@ var (
 	durationPattern = regexp.MustCompile(`^\s*[-]?(((\d{1,2}\s+)?0*\d{0,3}(:0*\d{1,2}){0,2})|(\d{1,7}))?(\.\d*)?\s*$`)

 	// timestampPattern checks whether a string matches the format of timestamp.
-	timestampPattern = regexp.MustCompile(`^\s*0*\d{1,4}([^\d]0*\d{1,2}){2}\s+(0*\d{0,2}([^\d]0*\d{1,2}){2})?(\.\d*)?\s*$`)
+	timestampPattern = regexp.MustCompile(`^` +


This commented multi line regexp is a big improvement!

dveeden · 2024-12-02T16:17:54Z

pkg/expression/builtin_time.go

+		// (regardless if min/sec exists or not...)
+		`(\.\d*)?` +
+		// Optionally time zone offset, must be +/-HH:MM format
+		`([+-]\d{2}[:]\d{2})?` +


Yes, I agree with that

ti-chi-bot · 2024-12-02T16:35:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dveeden
Once this PR has been reviewed and has the lgtm label, please assign windtalker for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

~~OWNERS~~ [dveeden]
pkg/expression/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-12-02T16:35:28Z

[LGTM Timeline notifier]

Timeline:

2024-12-02 16:35:28.072764413 +0000 UTC m=+1086315.692418928: ☑️ agreed by dveeden.

dveeden · 2024-12-02T16:37:01Z

Should it also accept this with the ODBC syntax?

mysql-8.0.11-TiDB-v8.5.0-alpha-219-gecd70f4222> SELECT { ts '2024-01-01 14:00:00-14:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00-14:00' } |
+------------------------------------+
| 2024-01-02 05:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

Note that the MySQL behavior here is interesting:

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00+00:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00+00:00' } |
+------------------------------------+
| 2024-01-01 14:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00-00:00' };
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
1 row in set (0.00 sec)

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00+01:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00+01:00' } |
+------------------------------------+
| 2024-01-01 13:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00-01:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00-01:00' } |
+------------------------------------+
| 2024-01-01 15:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mysql-8.0.11-TiDB-v8.5.0-alpha-219-gecd70f4222> SELECT { ts '2024-01-01 14:00:00-00:00' };
ERROR 1292 (22007): Incorrect datetime value: '2024-01-01 14:00:00-00:00'
mysql-8.0.11-TiDB-v8.5.0-alpha-219-gecd70f4222> SELECT { ts '2024-01-01 14:00:00+00:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00+00:00' } |
+------------------------------------+
| 2024-01-01 15:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mysql-8.0.11-TiDB-v8.5.0-alpha-219-gecd70f4222> SELECT { ts '2024-01-01 14:00:00+01:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00+01:00' } |
+------------------------------------+
| 2024-01-01 14:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mysql-8.0.11-TiDB-v8.5.0-alpha-219-gecd70f4222> SELECT { ts '2024-01-01 14:00:00-01:00' };
+------------------------------------+
| { ts '2024-01-01 14:00:00-01:00' } |
+------------------------------------+
| 2024-01-01 16:00:00                |
+------------------------------------+
1 row in set (0.00 sec)

mjonss · 2024-12-04T20:09:17Z

Should it also accept this with the ODBC syntax?
...
Note that the MySQL behavior here is interesting:

... 
mysql-9.1.0> SELECT { ts '2024-01-01 14:00:00-00:00' };
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
1 row in set (0.00 sec)
...

mysql-8.0.11-TiDB-v8.5.0-alpha-219-gecd70f4222> SELECT { ts '2024-01-01 14:00:00-00:00' };
ERROR 1292 (22007): Incorrect datetime value: '2024-01-01 14:00:00-00:00'

@dveeden I thinks this is due to MySQL falls back to string literal for ODBC syntax, in case it is not a proper timestamp literal:

mysql> SELECT { ts '2024-01-01 14:00:00-00:00' };
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
| 2024-01-01 14:00:00-00:00 |
+---------------------------+
1 row in set (0.00 sec)

mysql> select timestamp'2024-01-01 14:00:00-00:00';
ERROR 1525 (HY000): Incorrect DATETIME value: '2024-01-01 14:00:00-00:00'
mysql> select version();
+-----------+
| version() |
+-----------+
| 9.0.1     |
+-----------+
1 row in set (0.00 sec)

mjonss added 2 commits November 29, 2024 17:29

Updated restrictions on TIMSTAMP literal and changed error code

790172f

Added one more test

b22c3d8

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 29, 2024

mjonss requested a review from dveeden November 29, 2024 18:49

mjonss commented Nov 29, 2024

View reviewed changes

mjonss added 2 commits November 29, 2024 21:28

Added test for documenting we do not support MySQL internal format cu…

f2d943f

…rrently.

Updated tests affected by error change.

ecd70f4

mjonss mentioned this pull request Nov 29, 2024

Document DATE/TIME/TIMESTAMP literal prefixes. pingcap/docs#19543

Open

dveeden approved these changes Dec 2, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 2, 2024

added more tests, including for ODBC literal syntax

dc7b358

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: Timestamp literal with time zone offset #57845

expression: Timestamp literal with time zone offset #57845

mjonss commented Nov 29, 2024

tiprow bot commented Nov 29, 2024

codecov bot commented Nov 29, 2024 •

edited

Loading

mjonss Nov 29, 2024

dveeden Dec 2, 2024

mjonss Nov 29, 2024

mjonss Nov 29, 2024

dveeden left a comment

dveeden Dec 2, 2024

dveeden Dec 2, 2024

ti-chi-bot bot commented Dec 2, 2024

ti-chi-bot bot commented Dec 2, 2024

dveeden commented Dec 2, 2024

mjonss commented Dec 4, 2024

expression: Timestamp literal with time zone offset #57845

Are you sure you want to change the base?

expression: Timestamp literal with time zone offset #57845

Conversation

mjonss commented Nov 29, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 29, 2024

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

mjonss Nov 29, 2024

Choose a reason for hiding this comment

dveeden Dec 2, 2024

Choose a reason for hiding this comment

mjonss Nov 29, 2024

Choose a reason for hiding this comment

mjonss Nov 29, 2024

Choose a reason for hiding this comment

dveeden left a comment

Choose a reason for hiding this comment

dveeden Dec 2, 2024

Choose a reason for hiding this comment

dveeden Dec 2, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 2, 2024

ti-chi-bot bot commented Dec 2, 2024

[LGTM Timeline notifier]

dveeden commented Dec 2, 2024

mjonss commented Dec 4, 2024

codecov bot commented Nov 29, 2024 •

edited

Loading