expression: Truncate FSP instead of rounding for utc_timestamp() | tidb-test=pr/2405 #56431

mjonss · 2024-10-01T09:25:10Z

What problem does this PR solve?

Issue Number: close #56430 , close #31938

Problem Summary:
utc_timestamp() was rounding instead of truncating.

What changed and how does it work?

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.

Changing CURTIME(), SYSDATE() and UTC_TIMESTAMP() to truncate the FSP part instead of rounding it, so it is compatible with MySQL and in-line with NOW().

tiprow · 2024-10-01T09:25:29Z

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-10-01T09:43:11Z

Codecov Report

Attention: Patch coverage is 85.41667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 75.3508%. Comparing base (74034d4) to head (2400cf9).

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56431        +/-   ##
================================================
+ Coverage   73.3645%   75.3508%   +1.9862%     
================================================
  Files          1624       1624                
  Lines        448069     448161        +92     
================================================
+ Hits         328724     337693      +8969     
+ Misses        99207      89904      -9303     
- Partials      20138      20564       +426

Flag	Coverage Δ
integration	`48.8092% <43.7500%> (?)`
unit	`72.4688% <85.4166%> (+0.0051%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.3022% <ø> (+6.7892%)`	⬆️

mjonss · 2024-10-01T16:38:32Z

Also sysdate() is affected:

tidb> select now(2), now(6), utc_timestamp(2), utc_timestamp(6), sysdate(2), sysdate(6);
+------------------------+----------------------------+------------------------+----------------------------+------------------------+----------------------------+
| now(2)                 | now(6)                     | utc_timestamp(2)       | utc_timestamp(6)           | sysdate(2)             | sysdate(6)                 |
+------------------------+----------------------------+------------------------+----------------------------+------------------------+----------------------------+
| 2024-10-01 18:34:39.11 | 2024-10-01 18:34:39.117223 | 2024-10-01 16:34:39.12 | 2024-10-01 16:34:39.117224 | 2024-10-01 18:34:39.12 | 2024-10-01 18:34:39.117387 |
+------------------------+----------------------------+------------------------+----------------------------+------------------------+----------------------------+

vs MySQL 8.0.32:

mysql> select now(2), now(6), utc_timestamp(2), utc_timestamp(6), sysdate(2), sysdate(6);
+------------------------+----------------------------+------------------------+----------------------------+------------------------+----------------------------+
| now(2)                 | now(6)                     | utc_timestamp(2)       | utc_timestamp(6)           | sysdate(2)             | sysdate(6)                 |
+------------------------+----------------------------+------------------------+----------------------------+------------------------+----------------------------+
| 2024-10-01 18:35:08.18 | 2024-10-01 18:35:08.186503 | 2024-10-01 16:35:08.18 | 2024-10-01 16:35:08.186503 | 2024-10-01 18:35:08.18 | 2024-10-01 18:35:08.186681 |
+------------------------+----------------------------+------------------------+----------------------------+------------------------+----------------------------+

dveeden · 2024-10-02T06:46:06Z

/test build

ti-chi-bot · 2024-10-02T06:46:25Z

[LGTM Timeline notifier]

Timeline:

2024-10-02 06:46:25.744486953 +0000 UTC m=+423741.164699963: ☑️ agreed by dveeden.

tiprow · 2024-10-02T06:46:29Z

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

In response to this:

/test build

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.

dveeden · 2024-10-02T06:47:24Z

/check-issue-triage-complete

dveeden · 2024-10-02T06:47:53Z

/ok-to-test

dveeden · 2024-10-02T06:48:56Z

I think this does need a release note

mjonss · 2024-10-02T14:08:56Z

/retest

mjonss · 2024-10-03T15:27:34Z

pkg/types/time.go

-		tz = gotime.Local
-	}
-
+func (d Duration) RoundFrac(fsp int) (Duration, error) {


Duration should not use gotime.Time for rounding or truncating. There is simply no need or benefit to convert back and forth.

mjonss · 2024-10-03T21:37:41Z

/retest

dveeden

So this is not yet fixing SYSDATE() right?

sql> select utc_timestamp(6), utc_timestamp(), now(), now(6), utc_timestamp(6), utc_timestamp(), now(1), now(3), utc_timestamp(1), utc_timestamp(3), now(1), now(3), utc_timestamp(6)+0, now(6)+0, sysdate(6), current_timestamp(6), unix_timestamp(now(6)), unix_timestamp(sysdate(6)), from_unixtime(unix_timestamp(now(6))), microsecond(now(6))\G
*************************** 1. row ***************************
                     utc_timestamp(6): 2024-10-04 08:31:43.397413
                      utc_timestamp(): 2024-10-04 08:31:43
                                now(): 2024-10-04 10:31:43
                               now(6): 2024-10-04 10:31:43.397413
                     utc_timestamp(6): 2024-10-04 08:31:43.397413
                      utc_timestamp(): 2024-10-04 08:31:43
                               now(1): 2024-10-04 10:31:43.3
                               now(3): 2024-10-04 10:31:43.397
                     utc_timestamp(1): 2024-10-04 08:31:43.3
                     utc_timestamp(3): 2024-10-04 08:31:43.397
                               now(1): 2024-10-04 10:31:43.3
                               now(3): 2024-10-04 10:31:43.397
                   utc_timestamp(6)+0: 20241004083143.397413
                             now(6)+0: 20241004103143.397413
                           sysdate(6): 2024-10-04 10:31:43.397786
                 current_timestamp(6): 2024-10-04 10:31:43.397413
               unix_timestamp(now(6)): 1728030703.397413
           unix_timestamp(sysdate(6)): 1728030703.397790
from_unixtime(unix_timestamp(now(6))): 2024-10-04 10:31:43.397413
                  microsecond(now(6)): 397413
1 row in set (0.0009 sec)

Maybe add some tests in tests/integrationtest

ti-chi-bot · 2024-10-04T08:40:09Z

[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 xuhuaiyu 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-10-04T08:40:10Z

@dveeden: Your lgtm message is repeated, so it is ignored.

In response to this:

So this is not yet fixing SYSDATE() right?

sql> select utc_timestamp(6), utc_timestamp(), now(), now(6), utc_timestamp(6), utc_timestamp(), now(1), now(3), utc_timestamp(1), utc_timestamp(3), now(1), now(3), utc_timestamp(6)+0, now(6)+0, sysdate(6), current_timestamp(6), unix_timestamp(now(6)), unix_timestamp(sysdate(6)), from_unixtime(unix_timestamp(now(6))), microsecond(now(6))\G
*************************** 1. row ***************************
                    utc_timestamp(6): 2024-10-04 08:31:43.397413
                     utc_timestamp(): 2024-10-04 08:31:43
                               now(): 2024-10-04 10:31:43
                              now(6): 2024-10-04 10:31:43.397413
                    utc_timestamp(6): 2024-10-04 08:31:43.397413
                     utc_timestamp(): 2024-10-04 08:31:43
                              now(1): 2024-10-04 10:31:43.3
                              now(3): 2024-10-04 10:31:43.397
                    utc_timestamp(1): 2024-10-04 08:31:43.3
                    utc_timestamp(3): 2024-10-04 08:31:43.397
                              now(1): 2024-10-04 10:31:43.3
                              now(3): 2024-10-04 10:31:43.397
                  utc_timestamp(6)+0: 20241004083143.397413
                            now(6)+0: 20241004103143.397413
                          sysdate(6): 2024-10-04 10:31:43.397786
                current_timestamp(6): 2024-10-04 10:31:43.397413
              unix_timestamp(now(6)): 1728030703.397413
          unix_timestamp(sysdate(6)): 1728030703.397790
from_unixtime(unix_timestamp(now(6))): 2024-10-04 10:31:43.397413
                 microsecond(now(6)): 397413
1 row in set (0.0009 sec)

Maybe add some tests in tests/integrationtest

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.

mjonss · 2024-10-04T10:21:52Z

So this is not yet fixing SYSDATE() right?

It should fix SYSDATE() as well, where do you see it is not fixed?

tidb> select sysdate(6)+0, sysdate(3)+0, sysdate(), sysdate(3), sysdate(6), current_timestamp(6),  unix_timestamp(sysdate(3)), unix_timestamp(sysdate(6)\G
*************************** 1. row ***************************
                         sysdate(6)+0: 20241004121402.971827
                         sysdate(3)+0: 20241004121402.971
                            sysdate(): 2024-10-04 12:14:02
                           sysdate(3): 2024-10-04 12:14:02.971
                           sysdate(6): 2024-10-04 12:14:02.971842
                 current_timestamp(6): 2024-10-04 12:14:02.971174
           unix_timestamp(sysdate(3)): 1728036842.971
           unix_timestamp(sysdate(6)): 1728036842.971851

Here you see that both fsp == 0 and fsp == 3 is truncating instead of rounding.

Maybe add some tests in tests/integrationtest

It is tested here, doing it in an integration test is harder, since SYSDATE() is evaluated every time, using the current wall clock time, not the start of the statement as NOW(), CURRENT_TIMESTAMP() etc.

dveeden · 2024-10-04T14:36:29Z

It should fix SYSDATE() as well, where do you see it is not fixed?

Please ignore my earlier comment. SYSDATE() is working correctly.

dveeden · 2024-10-04T14:44:03Z

https://github.com/mysql/mysql-server/blob/trunk/mysql-test/t/func_time.test seems to use SET TIMESTAMP=... for testing. But it seems like TiDB doesn't support that?

mjonss · 2024-10-07T10:18:33Z

https://github.com/mysql/mysql-server/blob/trunk/mysql-test/t/func_time.test seems to use SET TIMESTAMP=... for testing. But it seems like TiDB doesn't support that?

TiDB also supports SET TIMESTAMP=<unix timestamp, possibly with fsp>

dveeden · 2024-10-07T10:34:56Z

https://github.com/mysql/mysql-server/blob/trunk/mysql-test/t/func_time.test seems to use SET TIMESTAMP=... for testing. But it seems like TiDB doesn't support that?

TiDB also supports SET TIMESTAMP=<unix timestamp, possibly with fsp>

I guess I must have made a typo. I tried again with TiDB and it indeed works.

XuHuaiyu

Please improve the description and title of the PR. This commit not only modifies the behavior of the utc_timestamp method.

XuHuaiyu · 2024-10-21T07:58:53Z

pkg/types/time.go

+// fractional seconds part, returns the duration type Time value and bool to indicate
+// whether the result is null. It also truncates FSP part instead of rounding it as
+// ParseDuration above does.
+func ParseDurationTruncateFsp(ctx Context, str string, fsp int) (Duration, bool, error) {


Do we need to add a test for this?

Truncate FSP instead of rounding for utc_timestamp()

1418285

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2024

A more extended fix, also for sysdate() and from_unixtime().

b361e22

Updated test

5d9cd8d

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2024

Only fixing truncation for functions returning current time.

76cbb3d

dveeden approved these changes Oct 2, 2024

View reviewed changes

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

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Oct 2, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Oct 2, 2024

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 2, 2024

mjonss added a commit to mjonss/tidb that referenced this pull request Oct 3, 2024

Reverted changes from pingcap#56431

1d01a4a

More tests and fixed CURTIME(1-6) rounding issue.

69749e5

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2024

mjonss commented Oct 3, 2024

View reviewed changes

mjonss added 9 commits October 3, 2024 17:32

Fixed copy/paste and test.

761913e

Added unit test

afdd618

Linting

c9706bb

Added Function Comment

c61cb35

Linting

4e33a1c

Linting

8ba03b9

Missed to trunate for CURTIME()

8c754e2

Fix for UTC_TIME().

5d74223

Fixes for UTC_TIME() and CURTIME().

2400cf9

mjonss changed the title ~~expression: Truncate FSP instead of rounding for utc_timestamp()~~ expression: Truncate FSP instead of rounding for utc_timestamp() | tidb-test=pr/2405 Oct 3, 2024

mjonss requested a review from dveeden October 3, 2024 21:51

dveeden approved these changes Oct 4, 2024

View reviewed changes

mjonss requested a review from XuHuaiyu October 15, 2024 11:35

XuHuaiyu reviewed Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: Truncate FSP instead of rounding for utc_timestamp() | tidb-test=pr/2405 #56431

expression: Truncate FSP instead of rounding for utc_timestamp() | tidb-test=pr/2405 #56431

mjonss commented Oct 1, 2024 •

edited

Loading

tiprow bot commented Oct 1, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

mjonss commented Oct 1, 2024

dveeden commented Oct 2, 2024

ti-chi-bot bot commented Oct 2, 2024

tiprow bot commented Oct 2, 2024

dveeden commented Oct 2, 2024

dveeden commented Oct 2, 2024

dveeden commented Oct 2, 2024

mjonss commented Oct 2, 2024

mjonss Oct 3, 2024 •

edited

Loading

mjonss commented Oct 3, 2024

dveeden left a comment

ti-chi-bot bot commented Oct 4, 2024

ti-chi-bot bot commented Oct 4, 2024

mjonss commented Oct 4, 2024

dveeden commented Oct 4, 2024

dveeden commented Oct 4, 2024

mjonss commented Oct 7, 2024

dveeden commented Oct 7, 2024

XuHuaiyu left a comment

XuHuaiyu Oct 21, 2024

expression: Truncate FSP instead of rounding for utc_timestamp() | tidb-test=pr/2405 #56431

Are you sure you want to change the base?

expression: Truncate FSP instead of rounding for utc_timestamp() | tidb-test=pr/2405 #56431

Conversation

mjonss commented Oct 1, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Oct 1, 2024

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

mjonss commented Oct 1, 2024

dveeden commented Oct 2, 2024

ti-chi-bot bot commented Oct 2, 2024

[LGTM Timeline notifier]

tiprow bot commented Oct 2, 2024

dveeden commented Oct 2, 2024

dveeden commented Oct 2, 2024

dveeden commented Oct 2, 2024

mjonss commented Oct 2, 2024

mjonss Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

mjonss commented Oct 3, 2024

dveeden left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 4, 2024

ti-chi-bot bot commented Oct 4, 2024

mjonss commented Oct 4, 2024

dveeden commented Oct 4, 2024

dveeden commented Oct 4, 2024

mjonss commented Oct 7, 2024

dveeden commented Oct 7, 2024

XuHuaiyu left a comment

Choose a reason for hiding this comment

XuHuaiyu Oct 21, 2024

Choose a reason for hiding this comment

mjonss commented Oct 1, 2024 •

edited

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading

mjonss Oct 3, 2024 •

edited

Loading