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

Driver should not round time #1121

Closed
methane opened this issue Jun 1, 2020 · 9 comments · Fixed by #1172
Closed

Driver should not round time #1121

methane opened this issue Jun 1, 2020 · 9 comments · Fixed by #1172
Labels

Comments

@methane
Copy link
Member

methane commented Jun 1, 2020

Issue

See this comment:
https://github.com/go-sql-driver/mysql/pull/1118/files#r433293822

MySQL rounds the received datetime. (ref). So the time may be rounded twice.

Example:

  1. Assume time = 1.444_999_5, and the column type has 2 digit fraction (e.g. TIME(2), DATETIME(2), or TIMESTAMP(2))
  2. The driver rounds the time to 1.445_000
  3. MySQL rounds the time to 1.45

Proposal

I just want to drop the rounding behavior in the driver.

@dolmen
Copy link
Contributor

dolmen commented Nov 23, 2020

Direct link to the code that must be fixed: https://github.com/go-sql-driver/mysql/blob/master/utils.go#L282

Instead of rounding to micro-second in the driver, the driver should just pass digits down to nanoseconds and let the server handle them. The only optimization should be to drop zeroes at the end.

This issue doesn't matter just for sub-seconds data types as the rounding will affect TIME, DATETIME or TIMESTAMP too. The example given in the description is misleading.

@dolmen
Copy link
Contributor

dolmen commented Nov 24, 2020

Here are some SQL expression that could be used as test cases:

SELECT(?, TIME);
SELECT(?, TIME(2));
SELECT(?, TIME(6));

shogo82148 added a commit to shogo82148/mysql that referenced this issue Nov 26, 2020
@shogo82148 shogo82148 mentioned this issue Nov 26, 2020
5 tasks
@shogo82148
Copy link
Contributor

I quote some comments in #1172 for aggregating information about this issue.

shogo82148 commented #1172 (comment)

MySQL supports only milliseconds.

You mean microseconds?

At least, we need to send 100 nanoseconds (7 digits) precision.
because the DATETIME converter can handle them.

Some examples:

mysql> SELECT VERSION();
+-----------+
| VERSION() |
+-----------+
| 8.0.22    |
+-----------+
1 row in set (0.00 sec)

mysql> SELECT CONVERT('2020-11-27 10:00:00.1234564', DATETIME(6));
+-----------------------------------------------------+
| CONVERT('2020-11-27 10:00:00.1234564', DATETIME(6)) |
+-----------------------------------------------------+
| 2020-11-27 10:00:00.123456                          |
+-----------------------------------------------------+
1 row in set (0.01 sec)

mysql> SELECT CONVERT('2020-11-27 10:00:00.1234565', DATETIME(6));
+-----------------------------------------------------+
| CONVERT('2020-11-27 10:00:00.1234565', DATETIME(6)) |
+-----------------------------------------------------+
| 2020-11-27 10:00:00.123457                          |
+-----------------------------------------------------+
1 row in set (0.01 sec)

mysql> SET @@sql_mode = sys.list_add(@@sql_mode, 'TIME_TRUNCATE_FRACTIONAL');
Query OK, 0 rows affected (0.02 sec)

mysql> SELECT CONVERT('2020-11-27 10:00:00.1234564', DATETIME(6));
+-----------------------------------------------------+
| CONVERT('2020-11-27 10:00:00.1234564', DATETIME(6)) |
+-----------------------------------------------------+
| 2020-11-27 10:00:00.123456                          |
+-----------------------------------------------------+
1 row in set (0.01 sec)

mysql> SELECT CONVERT('2020-11-27 10:00:00.1234565', DATETIME(6));
+-----------------------------------------------------+
| CONVERT('2020-11-27 10:00:00.1234565', DATETIME(6)) |
+-----------------------------------------------------+
| 2020-11-27 10:00:00.123456                          |
+-----------------------------------------------------+
1 row in set (0.01 sec)

As you say, currently MySQL only supports up to microseconds.
However, there is no guarantee that it will not support nanoseconds in the future.
So, I think that we should send all digits we know.

methane commented #1172 (comment)

I think you are right, but still feel it is a bit too much.

#1121 is really a bug. Round off 1.4449995 to 1.45 doesn't make sense at all.
Compared to it, behavior consistency between round off and round down is very minor. Should we send another 3 digits only for this minor consistency?

Let's check another connectors behavior.

methane commented #1172 (comment)

It seems Connector/J just nano/1000 when store it in binary protocol.

https://github.com/mysql/mysql-connector-j/blob/d64b664fa93e81296a377de031b8123a67e6def2/src/main/core-impl/java/com/mysql/cj/ServerPreparedQueryBindValue.java#L325
https://github.com/mysql/mysql-connector-j/blob/d64b664fa93e81296a377de031b8123a67e6def2/src/main/core-impl/java/com/mysql/cj/ServerPreparedQueryBindValue.java#L330

But they round or truncate nanoseconds based on TIME_TRUNCATE_FRACTIONAL sql_mode.

https://github.com/mysql/mysql-connector-j/blob/d64b664fa93e81296a377de031b8123a67e6def2/src/main/core-api/java/com/mysql/cj/util/TimeUtil.java#L183-L186

@methane
Copy link
Member Author

methane commented Nov 28, 2020

Options:

a. Truncate at microseconds.
b. Send nanoseconds.
c. Add timeFractions=N option where N is fraction digits. Truncate at N.

@dolmen
Copy link
Contributor

dolmen commented Dec 16, 2020

a. Truncate interacts with server truncate, so will lead to incorrect value in SOME cases (which means hard to debug for users not aware of the issue), so no
b. The best we can do.
c. Truncate interacts with server truncate (like a.), so not a good idea. In addition, a setting that applies globally to a DB connection have big impact on an application when you want to change them (you're stuck forever with a bad setting)

Correctness first, performance second.

@dolmen
Copy link
Contributor

dolmen commented Dec 16, 2020

Check also how basic TIME/DATETIME values are affected by server rounding (and so is corrupted by client-side truncation):

mysql> SELECT CONVERT('2020-11-27 10:00:00.4999994', DATETIME);
+--------------------------------------------------+
| CONVERT('2020-11-27 10:00:00.4999994', DATETIME) |
+--------------------------------------------------+
| 2020-11-27 10:00:00                              |
+--------------------------------------------------+
1 row in set (0,02 sec)
mysql> SELECT CONVERT('2020-11-27 10:00:00.4999995', DATETIME);
+--------------------------------------------------+
| CONVERT('2020-11-27 10:00:00.4999995', DATETIME) |
+--------------------------------------------------+
| 2020-11-27 10:00:01                              |
+--------------------------------------------------+
1 row in set (0,02 sec)

@methane methane added the bug label Jan 6, 2021
shogo82148 added a commit that referenced this issue Feb 2, 2021
* stop rounding times

fixes #1121

* trim trailing zeros
@bat22
Copy link

bat22 commented Jun 2, 2021

After upgrading from v1.5.0, queries with nanoseconds in params does not use index for datetime fields on MariaDB 10.4.7 ((

@methane
Copy link
Member Author

methane commented Jun 3, 2021

I didn't expect such a side effect.
We need to reconsider "c. Add timeFractions=N option where N is fraction digits. Truncate at N." idea.

@kkHAIKE
Copy link

kkHAIKE commented Nov 7, 2022

After upgrading from v1.5.0, queries with nanoseconds in params does not use index for datetime fields on MariaDB 10.4.7 ((

use time.Truncate(time.Microsecond)) before bind to sql param

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants