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

add support for time.Duration #1217

Open
adonovan opened this issue May 28, 2021 · 9 comments
Open

add support for time.Duration #1217

adonovan opened this issue May 28, 2021 · 9 comments

Comments

@adonovan
Copy link

Today I learned two fun facts:

  1. in MySQL, although now() is a datetime, now() + 0 is an integer formed from all of the decimal digits in the date. e.g. 2021-05-27 17:00:26 vs 20210527170026. (I'm not sure why one would ever want that.)
  2. in Go's database/sql API for prepared statements, "?" applied to a duration value treats the duration (thanks to reflection on the underlying type) as an integer number of nanoseconds, not an interval. I don't know why one would ever want that either, since nanoseconds are not really useful in SQL.

The combination of these two facts means that the behavior of query("now() + ?", 1*time.Hour) isn't remotely close to what one might expect:

mysql> select now(), now() + interval 1 hour, now() + 60*60*1e9;
+---------------------+-------------------------+-------------------+
| now()               | now() + interval 1 hour | now() + 60*60*1e9 |
+---------------------+-------------------------+-------------------+
| 2021-05-27 17:13:53 | 2021-05-27 18:13:53     |    23810527171353 |
+---------------------+-------------------------+-------------------+
1 row in set (0.00 sec)

Perhaps go-sql-driver could support time.Duration by mapping it to interval 1 hour, thereby avoiding this pitfall.
Alternatively, simply rejecting it with a clear error ("you cannot possibly have wanted this behavior") would be an improvement.

(See also golang/go#46427: I reported this initially against database/sql, and it was closed as a driver-specific issue.)

@shogo82148
Copy link
Contributor

The driver can't support time.Duration.

in MySQL, although now() is a datetime, now() + 0 is an integer formed from all of the decimal digits in the date. e.g. 2021-05-27 17:00:26 vs 20210527170026. (I'm not sure why one would ever want that.)

I am also surprised by this fact, however it is a MySQL specification.

https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_now
Returns the current date and time as a value in 'YYYY-MM-DD hh:mm:ss' or YYYYMMDDhhmmss format, depending on whether the function is used in string or numeric context. The value is expressed in the session time zone.

If the fsp argument is given to specify a fractional seconds precision from 0 to 6, the return value includes a fractional seconds part of that many digits.

mysql> SELECT NOW();
        -> '2007-12-15 23:50:26'
mysql> SELECT NOW() + 0;
        -> 20071215235026.000000

Perhaps go-sql-driver could support time.Duration by mapping it to interval 1 hour, thereby avoiding this pitfall.

MySQL doesn't have time duration types. https://dev.mysql.com/doc/refman/8.0/en/data-types.html
interval 1 hour is not a time duration value, but a temporal interval syntax.

https://dev.mysql.com/doc/refman/8.0/en/expressions.html#temporal-intervals
interval_expr in expressions represents a temporal interval. Intervals have this syntax:

INTERVAL expr unit

The driver doesn't know which unit you use, so you need to convert the unit yourself.
For example:

d := time.Hour
db.QueryContext(ctx, "SELECT NOW() + INTERVAL ? MICROSECOND", int64(d / time.Microsecond))
db.QueryContext(ctx, "SELECT NOW() + INTERVAL ? SECOND", d.Seconds())

@adonovan
Copy link
Author

adonovan commented Jun 1, 2021

Thanks for clarifying. If the driver can't support it, then please consider the alternative of specifically rejecting time.Duration from the generic conversion applied to other integer data types. The error message could suggest how to use either the INTERVAL syntax or an explicit integer number of nanoseconds (for the vanishing minority who might want the current behavior).

BTW, you can use d.Microseconds() in place of the int64 conversion.

@dolmen
Copy link
Contributor

dolmen commented Jun 30, 2021

time.Duration is not a valid driver.Value and doesn't implement the driver.Valuer interface. You should not expect it to work in a special way. database/sql just use the native representation (int64) as a fallback.

This kind of error in your code should be caught by a static analysis tool that would target database/sql usage and check types given to Query/QueryContext/Exec/Scan. Unfortunately I don't know a tool that does that kind of checks.

@adonovan
Copy link
Author

adonovan commented Jun 30, 2021

time.Duration is not a valid driver.Value and doesn't implement the driver.Valuer interface. You should not expect it to work in a special way. database/sql just use the native representation (int64) as a fallback.

This kind of error in your code should be caught by a static analysis tool [...]

I understand that Duration does not implement the SQL value interface, but that does not mean we cannot treat it specially, given how important it is (though I recognize there are pros and cons to doing so). A static analysis might have caught the problem, but that requires users to know that the analysis exists, to know how to run it, and to remember to run it---and if I knew that, I wouldn't have made the mistake. :)

If the library itself detects the mistake at little cost, so much the better.

@dolmen
Copy link
Contributor

dolmen commented Jul 1, 2021

If the library itself detects the mistake at little cost, so much the better.

I don't think that's a task for the MySQL driver. The same mistake could happen when using database/sql with any other SQL driver. So I don't think that's the right place for the improvement you dream of.

@adonovan
Copy link
Author

adonovan commented Jul 1, 2021

I don't think that's a task for the MySQL driver. The same mistake could happen when using database/sql with any other SQL driver. So I don't think that's the right place for the improvement you dream of.

As I pointed out in my first note, my original report against the standard database/sql package was closed on the basis that it was a driver-specific issue. :)

I don't see why the objection "the same mistake could happen when using database/sql with any other SQL driver" should present an obstacle to preventing the mistake in this MySQL driver. The worst case outcome is that both layers perform the check, making the driver's check redundant.

@zihengCat
Copy link
Contributor

zihengCat commented Jul 10, 2021

Well, time.Duration is treated as int64. When programmer types db.Query("SELECT now() + ?", 1*time.Hour), the converted SQL statement becomes SELECT now() + 3600000000000(int64 nanosecond count).

It's programmer's responsibility to know something under the hood and make sure their codes act as what they want, not driver.

It's fun MySQL implements expression datetime + integer like this, a little counterintuitive. I tried in SQLite, the behavior is different. :)
Screen Shot 2021-07-11 at 04 16 05

@adonovan
Copy link
Author

It's programmer's responsibility to know something under the hood and make sure their codes act as what they want, not driver.

It's a car-driver's responsibility to drive safely, but cars still come with seatbelts to reduce the cost of mistakes.

@zihengCat
Copy link
Contributor

zihengCat commented Jul 11, 2021

Well, SQL expression datetime + integer maybe is a kind of undefined behavior in my thought. Different SQL engine implemented in different ways.

  • MySQL concatenate and convert all decimal digits in the date and do addition operation with integer.
  • SQLite seems just cut out the year part in the date and plus integer.

I prefer static analysis solution, reported a warning alert like: WARN: undefined addition operation, mismatched types datetime and integer.

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

No branches or pull requests

4 participants