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

stop rounding times #1172

Merged
merged 4 commits into from Feb 2, 2021
Merged

Conversation

@shogo82148
Copy link
Contributor

@shogo82148 shogo82148 commented Nov 26, 2020

Description

fixes #1121

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file
fixes #1121
@methane
Copy link
Member

@methane methane commented Nov 27, 2020

https://dev.mysql.com/doc/refman/8.0/en/fractional-seconds.html

MySQL supports only milliseconds. Do we need to send nanoseconds?

@shogo82148
Copy link
Contributor Author

@shogo82148 shogo82148 commented Nov 27, 2020

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
Copy link
Member

@methane methane commented Nov 28, 2020

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.

@shogo82148
Copy link
Contributor Author

@shogo82148 shogo82148 commented Nov 28, 2020

OK. I'll check another connectors behavior, too.

There is the issue about this, so we should discuss in #1121.
I'll convert this pull request to draft.
Let's continue discussion in #1121.

@shogo82148 shogo82148 marked this pull request as draft Nov 28, 2020
@methane
Copy link
Member

@methane methane commented Jan 6, 2021

I think this pull request is good for v1.6. @shogo82148 would you merge master branch to run CI?

@shogo82148 shogo82148 marked this pull request as ready for review Jan 31, 2021
@shogo82148
Copy link
Contributor Author

@shogo82148 shogo82148 commented Jan 31, 2021

@methane I did!

@methane
Copy link
Member

@methane methane commented Feb 1, 2021

Is it safe to trim trailing 0s?

@shogo82148
Copy link
Contributor Author

@shogo82148 shogo82148 commented Feb 1, 2021

Maybe, it is.

@shogo82148
Copy link
Contributor Author

@shogo82148 shogo82148 commented Feb 1, 2021

It looks that starting MySQL server fails on macOS. it is not due to my pull request.

https://github.com/go-sql-driver/mysql/pull/1172/checks?check_run_id=1806566232

To have launchd start mysql now and restart at login:
  brew services start mysql
Or, if you don't want/need a background service you can just run:
  mysql.server start
Starting MySQL
/usr/local/Cellar/mysql/8.0.23/bin/mysqld_safe: line 653: /usr/local/var/mysql/Mac-1612182949676.local.err: No such file or directory
Logging to '/usr/local/var/mysql/Mac-1612182949676.local.err'.
/usr/local/Cellar/mysql/8.0.23/bin/mysqld_safe: line 144: /usr/local/var/mysql/Mac-1612182949676.local.err: No such file or directory
/usr/local/Cellar/mysql/8.0.23/bin/mysqld_safe: line 199: /usr/local/var/mysql/Mac-1612182949676.local.err: No such file or directory
/usr/local/Cellar/mysql/8.0.23/bin/mysqld_safe: line 916: /usr/local/var/mysql/Mac-1612182949676.local.err: No such file or directory
/usr/local/Cellar/mysql/8.0.23/bin/mysqld_safe: line 144: /usr/local/var/mysql/Mac-1612182949676.local.err: No such file or directory
. ERROR! The server quit without updating PID file (/usr/local/var/mysql/Mac-1612182949676.local.pid).
Error: Process completed with exit code 1.
@methane
methane approved these changes Feb 2, 2021
@shogo82148
Copy link
Contributor Author

@shogo82148 shogo82148 commented Feb 2, 2021

Thank you for your review!

@shogo82148 shogo82148 merged commit fe2230a into go-sql-driver:master Feb 2, 2021
48 checks passed
48 checks passed
list
Details
test (1.15, ubuntu-latest, 8.0)
Details
test (1.15, ubuntu-latest, 5.7)
Details
test (1.15, ubuntu-latest, 5.6)
Details
test (1.15, ubuntu-latest, mariadb-10.5)
Details
test (1.15, ubuntu-latest, mariadb-10.4)
Details
test (1.15, ubuntu-latest, mariadb-10.3)
Details
test (1.15, macos-latest, 8.0)
Details
test (1.15, macos-latest, 5.7)
Details
test (1.15, macos-latest, 5.6)
Details
test (1.15, macos-latest, mariadb-10.5)
Details
test (1.15, macos-latest, mariadb-10.4)
Details
test (1.15, macos-latest, mariadb-10.3)
Details
test (1.15, windows-latest, 8.0)
Details
test (1.15, windows-latest, 5.7)
Details
test (1.15, windows-latest, 5.6)
Details
test (1.15, windows-latest, mariadb-10.5)
Details
test (1.15, windows-latest, mariadb-10.4)
Details
test (1.15, windows-latest, mariadb-10.3)
Details
test (1.14, ubuntu-latest, 8.0)
Details
test (1.13, ubuntu-latest, 8.0)
Details
test (1.12, ubuntu-latest, 8.0)
Details
test (1.11, ubuntu-latest, 8.0)
Details
finish
Details
Coveralls - Linux-Go-1.11-DB-8.0 Coverage increased (+0.05%) to 81.91%
Details
Coveralls - Linux-Go-1.12-DB-8.0 Coverage increased (+0.05%) to 81.91%
Details
Coveralls - Linux-Go-1.13-DB-8.0 Coverage increased (+0.05%) to 81.826%
Details
Coveralls - Linux-Go-1.14-DB-8.0 Coverage decreased (-0.01%) to 81.826%
Details
Coveralls - Linux-Go-1.15-DB-5.6 Coverage increased (+0.05%) to 81.798%
Details
Coveralls - Linux-Go-1.15-DB-5.7 Coverage increased (+0.05%) to 81.798%
Details
Coveralls - Linux-Go-1.15-DB-8.0 Coverage increased (+0.05%) to 81.826%
Details
Coveralls - Linux-Go-1.15-DB-mariadb-10.3 Coverage increased (+0.05%) to 81.741%
Details
Coveralls - Linux-Go-1.15-DB-mariadb-10.4 Coverage increased (+0.05%) to 81.741%
Details
Coveralls - Linux-Go-1.15-DB-mariadb-10.5 Coverage increased (+0.05%) to 81.741%
Details
Coveralls - Windows-Go-1.15-DB-5.6 Coverage increased (+0.05%) to 81.498%
Details
Coveralls - Windows-Go-1.15-DB-5.7 Coverage increased (+0.05%) to 81.498%
Details
Coveralls - Windows-Go-1.15-DB-8.0 Coverage increased (+0.1%) to 81.555%
Details
Coveralls - Windows-Go-1.15-DB-mariadb-10.3 Coverage decreased (-0.009%) to 81.442%
Details
Coveralls - Windows-Go-1.15-DB-mariadb-10.4 Coverage increased (+0.05%) to 81.498%
Details
Coveralls - Windows-Go-1.15-DB-mariadb-10.5 Coverage increased (+0.05%) to 81.442%
Details
Coveralls - macOS-Go-1.15-DB-5.6 Coverage increased (+0.2%) to 81.741%
Details
Coveralls - macOS-Go-1.15-DB-5.7 Coverage increased (+0.05%) to 81.685%
Details
Coveralls - macOS-Go-1.15-DB-8.0 Coverage decreased (-0.01%) to 81.713%
Details
Coveralls - macOS-Go-1.15-DB-mariadb-10.3 Coverage increased (+0.05%) to 81.629%
Details
Coveralls - macOS-Go-1.15-DB-mariadb-10.4 Coverage increased (+0.05%) to 81.629%
Details
Coveralls - macOS-Go-1.15-DB-mariadb-10.5 Coverage increased (+0.05%) to 81.629%
Details
WIP Ready for review
Details
coverage/coveralls Coverage decreased (-0.01%) to 82.038%
Details
@shogo82148 shogo82148 deleted the shogo82148:stop-rounding-time branch Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants