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

sqlite: update sqlite-jdbc to 3.36.0.1 and improve date/time handling #17567

Merged
merged 3 commits into from Sep 1, 2021
Merged

sqlite: update sqlite-jdbc to 3.36.0.1 and improve date/time handling #17567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55eb186
update sqlite-jdbc to 3.36.0.1
pawit-metabase Aug 24, 2021
6366eaf
Improve date/time handling in SQLite
pawit-metabase Aug 25, 2021
5a777ef
Fix NULL handling
pawit-metabase Aug 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/drivers/sqlite/deps.edn
View file
Open in desktop
Expand Up @@ -2,4 +2,4 @@
["src" "resources"]

:deps
{org.xerial/sqlite-jdbc {:mvn/version "3.25.2"}}}
{org.xerial/sqlite-jdbc {:mvn/version "3.36.0.1"}}}
32 changes: 24 additions & 8 deletions modules/drivers/sqlite/src/metabase/driver/sqlite.clj
View file
Open in desktop
Expand Up @@ -376,14 +376,30 @@
(.close stmt)
(throw e)))))

;; (.getObject rs i LocalDate) doesn't seem to work, nor does `(.getDate)`; and it seems to be the case that
;; timestamps come back as `Types/DATE` as well? Fetch them as a String and then parse them
;; SQLite has no intrinsic date/time type. The sqlite-jdbc driver provides the following de-facto mappings:
;; DATE or DATETIME => Types/DATE (only if type is int or string)
;; TIMESTAMP => Types/TIMESTAMP (only if type is int)
;; The data itself can be stored either as
;; 1) integer (unix epoch) - this is "point in time", so no confusion about timezone
;; 2) float (julian days) - this is "point in time", so no confusion about timezone
;; 3) string (ISO8601) - zoned or unzoned depending on content, sqlite-jdbc always treat it as local time
;; Note that it is possible to store other invalid data in the column as SQLite does not perform any validation.
(defn- sqlite-handle-timestamp
[^ResultSet rs ^Integer i]
(let [obj (.getObject rs i)]
(cond
;; For strings, use our own parser which is more flexible than sqlite-jdbc's and handles timezones correctly
(instance? String obj) (u.date/parse obj)
;; For other types, fallback to sqlite-jdbc's parser
;; Even in DATE column, it is possible to put DATETIME, so always treat as DATETIME
(some? obj) (t/local-date-time (.getTimestamp rs i)))))

(defmethod sql-jdbc.execute/read-column-thunk [:sqlite Types/DATE]
[_ ^ResultSet rs _ ^Integer i]
(fn []
(try
(when-let [t (.getDate rs i)]
(t/local-date t))
(catch Throwable _
(when-let [s (.getString rs i)]
(u.date/parse s))))))
(sqlite-handle-timestamp rs i)))

(defmethod sql-jdbc.execute/read-column-thunk [:sqlite Types/TIMESTAMP]
[_ ^ResultSet rs _ ^Integer i]
(fn []
(sqlite-handle-timestamp rs i)))
61 changes: 61 additions & 0 deletions modules/drivers/sqlite/test/metabase/driver/sqlite_test.clj
View file
Open in desktop
Expand Up @@ -142,3 +142,64 @@
:base-type :type/DateTime
:database-position 0}}}
(driver/describe-table driver db (Table (mt/id :timestamp_table)))))))))))))

(deftest select-query-datetime
(mt/test-driver :sqlite
(let [db-name "datetime_test"
details (mt/dbdef->connection-details :sqlite :db {:database-name db-name})]
(doseq [stmt ["DROP TABLE IF EXISTS datetime_table;"
"CREATE TABLE datetime_table (
test_case varchar,
col_timestamp timestamp,
col_date date,
col_datetime datetime);"
"INSERT INTO datetime_table
(test_case, col_timestamp, col_date, col_datetime) VALUES
('epoch', 1629865104000, 1629849600000, 1629865104000),
('iso8601-ms', '2021-08-25 04:18:24.111', null, '2021-08-25 04:18:24.111'),
('iso8601-no-ms', '2021-08-25 04:18:24', null, '2021-08-25 04:18:24'),
('iso8601-no-time', null, '2021-08-25', null),
('null', null, null, null);"]]
(jdbc/execute! (sql-jdbc.conn/connection-details->spec :sqlite details)
[stmt]))
(mt/with-temp Database [db {:engine :sqlite :details (assoc details :dbname db-name)}]
(sync/sync-database! db)
;; In SQLite, you can actually store any value in any date/timestamp column,
;; let's test only values we'd reasonably run into.
;; Caveat: TIMESTAMP stored as string doesn't get parsed and is returned as-is by the driver,
;; some upper layer will handle it.
(mt/with-db db
(testing "select datetime stored as unix epoch"
(is (= [["2021-08-25T04:18:24Z" ; TIMESTAMP
"2021-08-25T00:00:00Z" ; DATE
"2021-08-25T04:18:24Z"]] ; DATETIME
(qp.test/rows
(mt/run-mbql-query :datetime_table
{:fields [$col_timestamp $col_date $col_datetime]
:filter [:= $test_case "epoch"]})))))
(testing "select datetime stored as string with milliseconds"
(is (= [["2021-08-25 04:18:24.111" ; TIMESTAMP (raw string)
"2021-08-25T04:18:24.111Z"]] ; DATETIME
(qp.test/rows
(mt/run-mbql-query :datetime_table
{:fields [$col_timestamp $col_datetime]
:filter [:= $test_case "iso8601-ms"]})))))
(testing "select datetime stored as string without milliseconds"
(is (= [["2021-08-25 04:18:24" ; TIMESTAMP (raw string)
"2021-08-25T04:18:24Z"]] ; DATETIME
(qp.test/rows
(mt/run-mbql-query :datetime_table
{:fields [$col_timestamp $col_datetime]
:filter [:= $test_case "iso8601-no-ms"]})))))
(testing "select date stored as string without time"
(is (= [["2021-08-25T00:00:00Z"]] ; DATE
(qp.test/rows
(mt/run-mbql-query :datetime_table
{:fields [$col_date]
:filter [:= $test_case "iso8601-no-time"]})))))
(testing "select NULL"
(is (= [[nil nil nil]]
(qp.test/rows
(mt/run-mbql-query :datetime_table
{:fields [$col_timestamp $col_date $col_datetime]
:filter [:= $test_case "null"]}))))))))))