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

UTC_TIMESTAMP() rounds fraction part instead of truncating it #56430

Open
mjonss opened this issue Oct 1, 2024 · 4 comments · May be fixed by #56431
Open

UTC_TIMESTAMP() rounds fraction part instead of truncating it #56430

mjonss opened this issue Oct 1, 2024 · 4 comments · May be fixed by #56431
Labels
severity/minor sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.

Comments

@mjonss
Copy link
Contributor

mjonss commented Oct 1, 2024

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

tidb> select utc_timestamp(6), utc_timestamp(), now(), now(6);
+----------------------------+---------------------+---------------------+----------------------------+
| utc_timestamp(6)           | utc_timestamp()     | now()               | now(6)                     |
+----------------------------+---------------------+---------------------+----------------------------+
| 2024-10-01 08:31:57.868668 | 2024-10-01 08:31:58 | 2024-10-01 10:31:57 | 2024-10-01 10:31:57.868667 |
+----------------------------+---------------------+---------------------+----------------------------+
1 row in set (0.00 sec)

2. What did you expect to see? (Required)

UTC_TIMESTAMP() should never round up, it is very confusing if time is rounding up, since it will be earlier than what the clock on the wall shows.

3. What did you see instead (Required)

Truncating (or always rounding down).

4. What is your TiDB version? (Required)

tidb_version(): Release Version: v8.4.0-alpha-325-g74034d4ac2
Edition: Community
Git Commit Hash: 74034d4ac243b3c14dbf5f8a9edb92e740da4212
Git Branch: now-diff-utc_timestamp
UTC Build Time: 2024-10-01 08:23:56
GoVersion: go1.22.1
Race Enabled: false
Check Table Before Drop: false
Store: unistore
@mjonss mjonss added the type/bug The issue is confirmed as a bug. label Oct 1, 2024
@mjonss
Copy link
Contributor Author

mjonss commented Oct 1, 2024

Related to #9710 but for UTC_TIMESTAMP(). I will make it truncate the fsp instead of rounding it.

@dulao5
Copy link

dulao5 commented Oct 2, 2024

Thank you very much.

I noticed that types.ModeHalfUp is also used in other code. There may be potential issues there as well.
Could you please check those as well?

e.g. https://github.com/pingcap/tidb/blob/master/pkg/expression/builtin_time.go#L1736

@mjonss
Copy link
Contributor Author

mjonss commented Oct 2, 2024

Thank you very much.

I noticed that types.ModeHalfUp is also used in other code. There may be potential issues there as well. Could you please check those as well?

e.g. https://github.com/pingcap/tidb/blob/master/pkg/expression/builtin_time.go#L1736

So there are several things that are tricky. SQL Standard and MySQL are using rounding for time in general (INSERT/UPDATE) but this specific PR if for now() type of functions, that generates a new current time, where it should not round, but truncate instead.

The use you link to is for FROM_UNIXTIME() which takes a unix_timestamp like integer/float/decimal as argument, and does rounding in MySQL.

I am working on a proof-of-concept for introducing TIME_TRUNCATE_FRACTIONAL sql_mode that would allow the user to configure using truncate instead of rounding for time fractional parts, see MySQL manual here.

@dulao5 what do you think about introducing MySQL's sql_mode TIME_TRUNCATE_FRACTIONAL?

@mjonss
Copy link
Contributor Author

mjonss commented Oct 3, 2024

Similar with curtime(), but only for the fsp, not if fsp == 0:

tidb> select now(), now(3), now(6), curtime(), curtime(6), curtime(3);
+---------------------+-------------------------+----------------------------+-----------+-----------------+--------------+
| now()               | now(3)                  | now(6)                     | curtime() | curtime(6)      | curtime(3)   |
+---------------------+-------------------------+----------------------------+-----------+-----------------+--------------+
| 2024-10-03 09:16:33 | 2024-10-03 09:16:33.743 | 2024-10-03 09:16:33.743866 | 09:16:33  | 09:16:33.743866 | 09:16:33.744 |
+---------------------+-------------------------+----------------------------+-----------+-----------------+--------------+
1 row in set (0.00 sec)

@jebter jebter added the sig/sql-infra SIG: SQL Infra label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity/minor sig/sql-infra SIG: SQL Infra type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants