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

Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864

Merged
merged 12 commits into from
May 1, 2024
Merged

Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f24d35e
Fix Snowflake filtering on date for `timestamp with time zone` columns
camsaul Apr 25, 2024
994c6e2
Snowflake should just generate raw SQL for literals
camsaul Apr 25, 2024
b7e3d1c
Go ahead and fix other Snowflake stuff and reenable a bunch of disabl…
camsaul Apr 26, 2024
e97ab7d
Merge branch 'master' into fix-snowflake-filtering-for-timestamp-tz-c…
camsaul Apr 29, 2024
589d38a
Merge branch 'master' into fix-snowflake-filtering-for-timestamp-tz-c…
camsaul Apr 30, 2024
a659256
Snowflake test fixes :wrench:
camsaul Apr 30, 2024
2b10084
Minor test improvements
camsaul Apr 30, 2024
d8652ba
Fix Redshift and Vertica as well.
camsaul Apr 30, 2024
5aca3c3
Address PR feedback
camsaul May 1, 2024
3759a5a
Test fixes :wrench:
camsaul May 1, 2024
11bfe63
Better Snowflake fix :wrench:
camsaul May 1, 2024
3e0903e
More test fixes :wrench:
camsaul May 1, 2024
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
89 changes: 30 additions & 59 deletions modules/drivers/snowflake/src/metabase/driver/snowflake.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@
[ring.util.codec :as codec])
(:import
(java.io File)
(java.sql Connection DatabaseMetaData ResultSet Types)
(java.time OffsetDateTime OffsetTime ZonedDateTime)
(java.sql Connection DatabaseMetaData ResultSet ResultSetMetaData Types)
(java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime)
(net.snowflake.client.jdbc SnowflakeSQLException)))

(set! *warn-on-reflection* true)
Expand Down Expand Up @@ -194,7 +194,7 @@
:DATETIME :type/DateTime
:TIME :type/Time
:TIMESTAMP :type/DateTime
:TIMESTAMPLTZ :type/DateTime
:TIMESTAMPLTZ :type/DateTimeWithLocalTZ
:TIMESTAMPNTZ :type/DateTime
:TIMESTAMPTZ :type/DateTimeWithTZ
:VARIANT :type/*
Expand Down Expand Up @@ -413,6 +413,31 @@
[driver [_ amount unit]]
(qp.relative-datetime/maybe-cacheable-relative-datetime-honeysql driver unit amount))

(defmethod sql.qp/->honeysql [:snowflake LocalDate]
[_driver t]
[:raw (format "'%s'::date" (t/local-date t))])

(defmethod sql.qp/->honeysql [:snowflake LocalTime]
[_driver t]
[:raw (format "'%s'::time" (t/local-time t))])

;;; Snowflake doesn't have `timetz`, so just convert to an equivalent local time.
(defmethod sql.qp/->honeysql [:snowflake OffsetTime]
[driver t]
(sql.qp/->honeysql driver (t/local-time (t/with-offset-same-instant t (t/zone-offset 0)))))

(defmethod sql.qp/->honeysql [:snowflake LocalDateTime]
[_driver t]
[:raw (format "'%s %s'::timestamp_ntz" (t/local-date t) (t/local-time t))])

(defmethod sql.qp/->honeysql [:snowflake OffsetDateTime]
[_driver t]
[:raw (format "'%s %s %s'::timestamp_ltz" (t/local-date t) (t/local-time t) (t/zone-offset t))])

(defmethod sql.qp/->honeysql [:snowflake ZonedDateTime]
[driver t]
(sql.qp/->honeysql driver (t/offset-date-time t)))

(defmethod driver/table-rows-seq :snowflake
[driver database table]
(sql-jdbc/query driver database {:select [:*]
Expand Down Expand Up @@ -576,65 +601,11 @@
(m/dissoc-in [:details :regionid]))
database))

(defmethod unprepare/unprepare-value [:snowflake OffsetDateTime]
[_ t]
(format "'%s %s %s'::timestamp_tz" (t/local-date t) (t/local-time t) (t/zone-offset t)))

(defmethod unprepare/unprepare-value [:snowflake ZonedDateTime]
[driver t]
(unprepare/unprepare-value driver (t/offset-date-time t)))

(defmethod unprepare/unprepare-value [:snowflake OffsetTime]
[driver t]
(unprepare/unprepare-value driver (t/local-time (t/with-offset-same-instant t (t/zone-offset 0)))))
Comment on lines -582 to -592
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed since we don't parameterize these anymore.


;; Like Vertica, Snowflake doesn't seem to be able to return a LocalTime/OffsetTime like everyone else, but it can
;; return a String that we can parse

(defmethod sql-jdbc.execute/read-column-thunk [:snowflake Types/TIMESTAMP_WITH_TIMEZONE]
[_ ^ResultSet rs _ ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs %d) [TIMESTAMP_WITH_TIMEZONE] -> %s -> %s" i (pr-str s) (pr-str t))
t))))

(defmethod sql-jdbc.execute/read-column-thunk [:snowflake Types/TIME]
[_ ^ResultSet rs _ ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs %d) [TIME] -> %s -> %s" i (pr-str s) (pr-str t))
t))))

(defmethod sql-jdbc.execute/read-column-thunk [:snowflake Types/TIME_WITH_TIMEZONE]
[_ ^ResultSet rs _ ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs %d) [TIME_WITH_TIMEZONE] -> %s -> %s" i (pr-str s) (pr-str t))
t))))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TIME impl is exactly the same as the version in metabase.driver.sql-jdbc.execute.legacy-impl, which Snowflake would have used anyway, so I removed it. Snowflake doesn't have TIME_WITH_TIMEZONE, so this was dead code. But metabase.driver.sql-jdbc.execute.legacy-impl was missing TIME_WITH_TIMEZONE and TIMESTAMP_WITH_TIMEZONE impls so I moved them there since they look almost exactly like the other impls.


;; TODO ­ would it make more sense to use functions like `timestamp_tz_from_parts` directly instead of JDBC parameters?

;; Snowflake seems to ignore the calendar parameter of `.setTime` and `.setTimestamp` and instead uses the session
;; timezone; normalize temporal values to UTC so we end up with the right values
(defmethod sql-jdbc.execute/set-parameter [::use-legacy-classes-for-read-and-set java.time.OffsetTime]
[driver ps i t]
(sql-jdbc.execute/set-parameter driver ps i (t/sql-time (t/with-offset-same-instant t (t/zone-offset 0)))))

(defmethod sql-jdbc.execute/set-parameter [::use-legacy-classes-for-read-and-set java.time.OffsetDateTime]
[driver ps i t]
(sql-jdbc.execute/set-parameter driver ps i (t/sql-timestamp (t/with-offset-same-instant t (t/zone-offset 0)))))

(defmethod sql-jdbc.execute/set-parameter [:snowflake java.time.ZonedDateTime]
[driver ps i t]
(sql-jdbc.execute/set-parameter driver ps i (t/sql-timestamp (t/with-zone-same-instant t (t/zone-id "UTC")))))
Comment on lines -621 to -635
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed since we're generating SQL literals instead


;;; --------------------------------------------------- Query remarks ---------------------------------------------------

; Snowflake strips comments prepended to the SQL statement (default remark injection behavior). We should append the
; remark instead.
;; Snowflake strips comments prepended to the SQL statement (default remark injection behavior). We should append the
;; remark instead.
(defmethod sql-jdbc.execute/inject-remark :snowflake
[_ sql remark]
(str sql "\n\n-- " remark))
Expand Down
118 changes: 90 additions & 28 deletions modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,19 @@
[clojure.string :as str]
[clojure.test :refer :all]
[metabase.driver :as driver]
[metabase.driver.common.parameters :as params]
[metabase.driver.snowflake :as driver.snowflake]
[metabase.driver.sql :as driver.sql]
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
[metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
[metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
[metabase.driver.sql-jdbc.sync.describe-database :as sql-jdbc.describe-database]
[metabase.driver.sql.parameters.substitution :as sql.params.substitution]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.jvm :as lib.metadata.jvm]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
[metabase.models :refer [Table]]
[metabase.models.database :refer [Database]]
Expand Down Expand Up @@ -357,33 +361,40 @@
(merge {:db pk-db :user pk-user} to-merge))]
(is (can-connect? details)))))))))))

(deftest ^:parallel replacement-snippet-date-param-test
(mt/test-driver :snowflake
(qp.store/with-metadata-provider meta/metadata-provider
(is (= {:replacement-snippet "'2014-08-02'::date"
:prepared-statement-args nil}
(sql.params.substitution/->replacement-snippet-info :snowflake (params/->Date "2014-08-02")))))))

(deftest report-timezone-test
(mt/test-driver :snowflake
(testing "Make sure temporal parameters are set and returned correctly when report-timezone is set (#11036)"
(letfn [(run-query []
(mt/rows
(qp/process-query
{:database (mt/id)
(testing "Make sure temporal parameters are set and returned correctly when report-timezone is set (#11036, #39769)"
(let [query {:database (mt/id)
:type :native
:native {:query (str "SELECT {{filter_date}}")
:template-tags {:filter_date {:name "filter_date"
:display_name "Just A Date"
:type "date"}}}
:parameters [{:type "date/single"
:target ["variable" ["template-tag" "filter_date"]]
:value "2014-08-02"}]})))]
(testing "baseline"
(is (= [["2014-08-02T00:00:00Z"]]
(run-query))))
(testing "with report-timezone"
(mt/with-temporary-setting-values [report-timezone "US/Pacific"]
(is (= [["2014-08-02T00:00:00-07:00"]]
(run-query)))))))
(testing "Make sure temporal values are returned correctly when report-timezone is set (#11036)"
(letfn [(run-query []
(mt/rows
(qp/process-query
{:database (mt/id)
:value "2014-08-02"}]}]
(mt/with-native-query-testing-context query
(letfn [(run-query []
(mt/rows (qp/process-query query)))]
(testing "baseline"
(is (= [["2014-08-02T00:00:00Z"]]
(run-query))))
(testing "with report-timezone"
(mt/with-temporary-setting-values [report-timezone "US/Pacific"]
(is (= [["2014-08-02T00:00:00-07:00"]]
(run-query)))))))))))

(deftest report-timezone-test-2
(mt/test-driver :snowflake
(testing "Make sure temporal values are returned correctly when report-timezone is set (#11036, #39769)"
(let [query {:database (mt/id)
:type :native
:native {:query (str "SELECT {{filter_date}}, \"last_login\" "
(format "FROM \"%stest-data\".\"PUBLIC\".\"users\" "
Expand All @@ -395,16 +406,67 @@
:type "date"}}}
:parameters [{:type "date/single"
:target ["variable" ["template-tag" "filter_date"]]
:value "2014-08-02"}]})))]
(testing "baseline (no report-timezone set)"
(is (= [["2014-08-02T00:00:00Z" "2014-08-02T12:30:00Z"]
["2014-08-02T00:00:00Z" "2014-08-02T09:30:00Z"]]
(run-query))))
(testing "with report timezone set"
(is (= [["2014-08-02T00:00:00-07:00" "2014-08-02T12:30:00-07:00"]
["2014-08-02T00:00:00-07:00" "2014-08-02T09:30:00-07:00"]]
(mt/with-temporary-setting-values [report-timezone "US/Pacific"]
(run-query)))))))))
:value "2014-08-02"}]}]
(mt/with-native-query-testing-context query
(letfn [(run-query []
(mt/rows (qp/process-query query)))]
(testing "baseline (no report-timezone set)"
(is (= [["2014-08-02T00:00:00Z" "2014-08-02T12:30:00Z"]
["2014-08-02T00:00:00Z" "2014-08-02T09:30:00Z"]]
(run-query))))
(testing "with report timezone set"
(mt/with-temporary-setting-values [report-timezone "US/Pacific"]
(is (= [["2014-08-02T00:00:00-07:00" "2014-08-02T12:30:00-07:00"]
["2014-08-02T00:00:00-07:00" "2014-08-02T09:30:00-07:00"]]
(run-query)))))))))))

(defn- test-temporal-instance
"Test that `java.time` instance `t` is set correctly (as a parameter) and returned correctly."
[t expected]
(mt/test-driver :snowflake
(testing "(#11036, #39769)"
(let [[sql & params] (sql.qp/format-honeysql :snowflake {:select [[(sql.qp/->honeysql :snowflake t) :t]]})
query {:database (mt/id)
:type :native
:native {:query sql
:params (vec params)}}]
(mt/with-native-query-testing-context query
(testing (format "\nt = ^%s %s" (.getName (class t)) (pr-str t))
(is (= [expected]
(mt/first-row (qp/process-query query))))))))))

(deftest ^:parallel local-date-time-parameter-test
(test-temporal-instance
#t "2024-04-25T14:44:00"
"2024-04-25T14:44:00Z"))

(deftest local-date-time-parameter-report-timezone-test
(mt/with-temporary-setting-values [report-timezone "US/Pacific"]
(test-temporal-instance
#t "2024-04-25T14:44:00"
"2024-04-25T14:44:00-07:00")))

(deftest ^:parallel offset-date-time-parameter-test
(test-temporal-instance
#t "2024-04-25T14:44:00-07:00"
"2024-04-25T21:44:00Z"))

(deftest offset-date-time-parameter-report-timezone-test
(mt/with-temporary-setting-values [report-timezone "US/Pacific"]
(test-temporal-instance
#t "2024-04-25T14:44:00-07:00"
"2024-04-25T14:44:00-07:00")))

(deftest ^:parallel zoned-date-time-parameter-test
(test-temporal-instance
#t "2024-04-25T14:44:00-07:00[US/Pacific]"
"2024-04-25T21:44:00Z"))

(deftest zoned-date-time-parameter-report-timezone-test
(mt/with-temporary-setting-values [report-timezone "US/Pacific"]
(test-temporal-instance
#t "2024-04-25T14:44:00-07:00[US/Pacific]"
"2024-04-25T14:44:00-07:00")))

(deftest week-start-test
(mt/test-driver :snowflake
Expand Down
9 changes: 5 additions & 4 deletions src/metabase/driver/sql/parameters/substitution.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
:prepared-statement-args [#t \"2017-01-01\"]}"
(:require
[clojure.string :as str]
[honey.sql :as sql]
[metabase.driver :as driver]
[metabase.driver.common.parameters :as params]
[metabase.driver.common.parameters.dates :as params.dates]
Expand Down Expand Up @@ -58,7 +59,8 @@
(defn- honeysql->prepared-stmt-subs
"Convert X to a replacement snippet info map by passing it to HoneySQL's `format` function."
[driver x]
(let [[snippet & args] (sql.qp/format-honeysql driver x)]
(let [honeysql (sql.qp/->honeysql driver x)
[snippet & args] (sql.qp/format-honeysql driver honeysql)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter compilation code should call ->honeysql on java.time arguments

(make-stmt-subs snippet args)))

(mu/defmethod ->prepared-substitution [:sql nil] :- PreparedStatementSubstitution
Expand All @@ -81,14 +83,13 @@
[driver kwd]
(honeysql->prepared-stmt-subs driver kwd))

;; TIMEZONE FIXME - remove this since we aren't using `Date` anymore
(mu/defmethod ->prepared-substitution [:sql Date] :- PreparedStatementSubstitution
[_driver date]
(make-stmt-subs "?" [date]))

(mu/defmethod ->prepared-substitution [:sql Temporal] :- PreparedStatementSubstitution
[_driver t]
(make-stmt-subs "?" [t]))
[driver t]
(honeysql->prepared-stmt-subs driver t))

(defmulti align-temporal-unit-with-param-type
"Returns a suitable temporal unit conversion keyword for `field`, `param-type` and the given driver.
Expand Down
22 changes: 19 additions & 3 deletions src/metabase/driver/sql_jdbc/execute/legacy_impl.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,29 +60,45 @@
(.setTimestamp ps i t cal)))

(defmethod sql-jdbc.execute/read-column-thunk [::use-legacy-classes-for-read-and-set Types/TIME]
[_ ^ResultSet rs _ ^Integer i]
[_driver ^ResultSet rs _rsmeta ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs i) [TIME] -> %s -> %s" (pr-str s) (pr-str t))
t))))

(defmethod sql-jdbc.execute/read-column-thunk [::use-legacy-classes-for-read-and-set Types/TIME_WITH_TIMEZONE]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from Snowflake

[_driver ^ResultSet rs _rsmeta ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs i) [TIME_WITH_TIMEZONE] -> %s -> %s" (pr-str s) (pr-str t))
t))))

(defmethod sql-jdbc.execute/read-column-thunk [::use-legacy-classes-for-read-and-set Types/DATE]
[_ ^ResultSet rs _ ^Integer i]
[_driver ^ResultSet rs _rsmeta ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs i) [DATE] -> %s -> %s" (pr-str s) (pr-str t))
t))))

(defmethod sql-jdbc.execute/read-column-thunk [::use-legacy-classes-for-read-and-set Types/TIMESTAMP]
[_ ^ResultSet rs _ ^Integer i]
[_driver ^ResultSet rs _rsmeta ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs i) [TIMESTAMP] -> %s -> %s" (pr-str s) (pr-str t))
t))))

(defmethod sql-jdbc.execute/read-column-thunk [::use-legacy-classes-for-read-and-set Types/TIMESTAMP_WITH_TIMEZONE]
[_driver ^ResultSet rs _rsmeta ^Integer i]
(fn []
(when-let [s (.getString rs i)]
(let [t (u.date/parse s)]
(log/tracef "(.getString rs i) [TIMESTAMP_WITH_TIMEZONE] -> %s -> %s" (pr-str s) (pr-str t))
t))))

(doseq [dispatch-val (keys (methods sql-jdbc.execute/read-column-thunk))
:when (sequential? dispatch-val)
:let [[driver jdbc-type] dispatch-val]
Expand Down