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.

Sign up for GitHub

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

Jump to bottom

database/sql: query("now() + ?", time.Hour) is surprising #46427

Closed
adonovan opened this issue May 27, 2021 · 4 comments
Closed

database/sql: query("now() + ?", time.Hour) is surprising #46427

adonovan opened this issue May 27, 2021 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@adonovan
Copy link
Member

adonovan commented May 27, 2021 •
edited

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)

If database/sql were to treat durations specially, they could be mapped to interval 1 hour, avoiding this pitfall.
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 27, 2021
@mknyszek mknyszek added this to the Backlog milestone May 27, 2021
@mknyszek
Copy link
Contributor

😱

CC @kardianos via https://dev.golang.org/owners

Based on the OP this looks like a problem in older versions of Go as well, but could you please confirm that?

@seankhliao
Copy link
Member

database/sql passes the arguments for drivers to process, specifically, drivers may implement database/sql/driver.NamedValueChecker to handle types outside the default list. Given the varying representations of durations in different databases (as noted in #4954), it doesn't make sense to do this in database/sql or database/sql/dirver and should instead be handled by individual drivers instead.

@adonovan
Copy link
Member Author

I see. If it's impossible to support meaningfully on a range of databases, and if none support it today, then we should consider specifically disallowing it with a good error.

@seankhliao
Copy link
Member

returning an error would prevent drivers from ever supporting this, and validation was specifically removed as part of the introduction of NamedValueChecker. CL 38533 / #18417

Closing there's nothing to do for the stdlib and requests should instead be filed with the drivers.

@adonovan adonovan mentioned this issue May 28, 2021
Open
@golang golang locked and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@mknyszek @adonovan @gopherbot @seankhliao