-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Add Time type support for SQLite #8
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
feat: Add Time type support for SQLite #8
Conversation
3ff54f7 to
a5628ab
Compare
| module Quoting | ||
| class Sqlite3 | ||
| include ActiveRecord::ConnectionAdapters::Quoting | ||
| include ActiveRecord::ConnectionAdapters::SQLite3::Quoting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test passes even with only the abstract module, but in that case, the resulting string will have the year and month information removed. (Reference: https://github.com/rails/rails/blob/90a1eaa1b30ba1f2d524e197460e549c03cf5698/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb#L191-L194)
However, SQLite3 explicitly appends 2000-01-01, so the SQLite3 module is included to maintain this behavior.
(Reference: https://github.com/rails/rails/blob/90a1eaa1b30ba1f2d524e197460e549c03cf5698/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb#L74-L77)
| def sqlite3_quoting_proxy | ||
| @_sqlite3_quoting_proxy ||= begin | ||
| require_relative "quoting/sqlite3" | ||
|
|
||
| Quoting::Sqlite3.new(self) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can we move
require_relative "quoting/sqlite3"to the top-level? - How about renaming the cached instance variable to
@quoting_sqlite3to align withQuoting::Sqlite3name?
| def sqlite3_quoting_proxy | |
| @_sqlite3_quoting_proxy ||= begin | |
| require_relative "quoting/sqlite3" | |
| Quoting::Sqlite3.new(self) | |
| end | |
| def quoting_sqlite3 | |
| @quoting_sqlite3 ||= Quoting::Sqlite3.new(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move require_relative "quoting/sqlite3" to the top-level?
The Quoting::Sqlite3 class is unnecessary outside of sqlite3, so it was not required at the top level.
Is top-level better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming the cached instance variable to @quoting_sqlite3 to align with Quoting::Sqlite3 name?
fix by 81552e2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is top-level better?
Yes. We should avoid needless dynamic require/require_relative as much as possible. It may be slow. e.g.: ruby/rexml#219
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's very educational!
fix by 8d3f2b9
| if time_str.nil? | ||
| nil | ||
| else | ||
| dt = time_str.to_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does dt stand for "date time"?
time may be better because we use to_time here.
BTW, should we refer default_timezone here? It seems that String#to_time uses local time by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, should we refer default_timezone here? It seems that String#to_time uses local time by default.
Exactly.
When reading from or writing to the database through ActiveRecord, the value of default_timezone is used.
fix by 4b25d0f 🙆🏻♂️
Co-authored-by: Sutou Kouhei <[email protected]>
… handling in tests
|
Thanks! |
Summary of Changes
This pull request introduces support for the
timedata type when using the SQLite backend. The ADBC SQLite driver does not natively support atimetype, so this change implements a workaround by quotingTimeobjects as strings, which is consistent with how Rails' built-in SQLite3 adapter handles them.The changes include:
quoted_timemethod has been added toquoting.rb. For the SQLite backend, it delegates to Rails' internalSQLite3::Quotingmodule to ensure the time is formatted into the standard2000-01-01 HH:MM:SS.SSSSSSstring format.Resultclass inresult.rbnow correctly converts these time strings back into anArrow::Time64Arraywhen processing query results for a model.Timetype tests intest_type.rb, which were previously omitted for SQLite, are now enabled and passing.Reason for Changes
Without this change, using
ActiveRecord::Type::Timewith the SQLite backend would fail because the ADBC driver lacks native support for this data type. This implementation bridges the gap by adopting the same string-based serialization strategy used by the standard Rails SQLite3 adapter, enabling seamless use of thetimetype.Affected Components
lib/activerecord_adbc_adapter/quoting.rblib/activerecord_adbc_adapter/result.rbtest/test_type.rbTesting
The existing tests for the
timetype intest/test_type.rbnow run successfully against the SQLite backend.To verify:
test_time_active_recordandtest_time_arrow.bundle exec ruby test/run.rb -n /test_time/All tests related to the Time type should pass.
Related issue
#7