Skip to content

Add support for casting DATE values as Time objects. (:cast_dates_as_times => true) - #1012

Closed
joe07734 wants to merge 9 commits into
brianmario:masterfrom
errolgrannum:cast-datetimes
Closed

Add support for casting DATE values as Time objects. (:cast_dates_as_times => true)#1012
joe07734 wants to merge 9 commits into
brianmario:masterfrom
errolgrannum:cast-datetimes

Conversation

@joe07734

Copy link
Copy Markdown

This patch adds a new query option to cast mysql DATE result values as ruby Time objects.

At Bandcamp we've been using this patch (and the other) in our fork of the mysql2 gem since the beginning. I made this patch because we use DATETIME columns in almost all our tables, but occasionally we'll use DATE. With this patch we don't have to care about mixed results.

This is a default for all our queries:

opts = Mysql2::Client.default_query_options
opts[:cast_dates_as_times] = true

I've been meaning to make these two pull requests since forever. We've had years of experience with them and think they're useful. If you have any questions about how Bandcamp's Linux/MySQL/Ruby stack works, just ask.

@sodabrew

Copy link
Copy Markdown
Collaborator

This is awesome! Do you have time to also ensure this works for Prepared Statements? Start with adding the same unit test with a statement, it's possible no further work is needed beyond that.

@joe07734

Copy link
Copy Markdown
Author

Glad you dig it. I'll add the prepared statements test(s) in a bit.

@joe07734

joe07734 commented Nov 1, 2018

Copy link
Copy Markdown
Author

Added and tested.

@sodabrew

sodabrew commented Nov 1, 2018

Copy link
Copy Markdown
Collaborator

Usually I'd squash to merge rather than keeping the history of little-edit-commits, but to preserve the two authors, would you squash down to two commits? I suggest one with the main thrust of work and the other with the fixups to get it upstreamed.

@sodabrew sodabrew added this to the 0.6.0 milestone Nov 1, 2018
@joe07734

joe07734 commented Nov 7, 2018

Copy link
Copy Markdown
Author

It's fine by me if you squash the merge. I'm unclear how I'd squash it in the pull request, is that something you can explain?

@sodabrew

Copy link
Copy Markdown
Collaborator

Closing -- the feature is real (mixed DATE/DATETIME columns are genuinely awkward to handle uniformly), but the implementation borrows machinery that doesn't semantically fit, and the feature doesn't need to live in the C extension at all.

DATE is a pure calendar value -- no time-of-day, no timezone. This PR routes it through the exact same database_timezone/application_timezone conversion path used for DATETIME/TIMESTAMP, which legitimately need it because they represent an instant. Applying that same conversion to a value that has no "when," only a "which day," means a stored 2024-04-04 can come back as 2024-04-03 23:00:00 or the 5th depending on the app/db timezone gap -- silently shifting the calendar date on a value that was never supposed to have one. That's a foot-gun on data like invoice dates or birthdays, and it's inherent to reusing the DATETIME construction path, not a bug in the diff itself.

Separately: every other cast option mysql2 has at the C level exists because the C layer has performance leverage Ruby-space doesn't (cast: false/cast: :fast specifically exist to skip allocating BigDecimal/Time). This option doesn't save anything -- it still constructs one object per row, just a Time instead of a Date. The same effect is achievable today with zero new C code and more flexibility (any target representation, not one hardcoded choice) as a one-line map at the call site:

result.each { |row| row['date_col'] = row['date_col'].to_time }

If there's still appetite for shipping this as a first-class option, I'd want it to construct at plain local midnight with no timezone math involved, so the calendar date is guaranteed stable regardless of db_timezone/app_timezone -- that closes the silent-shift risk regardless of naming. Happy to help think through that version if it comes back up.

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

Successfully merging this pull request may close these issues.

3 participants