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 3 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
140 changes: 68 additions & 72 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 @@ -22,9 +22,9 @@
[metabase.driver.sql-jdbc.sync.describe-table :as sql-jdbc.describe-table]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.driver.sql.util :as sql.u]
[metabase.driver.sql.util.unprepare :as unprepare]
[metabase.driver.sync :as driver.s]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.schema.temporal-bucketing :as lib.schema.temporal-bucketing]
[metabase.models.secret :as secret]
[metabase.public-settings :as public-settings]
[metabase.query-processor.error-type :as qp.error-type]
Expand All @@ -42,7 +42,7 @@
(:import
(java.io File)
(java.sql Connection DatabaseMetaData ResultSet Types)
(java.time OffsetDateTime OffsetTime ZonedDateTime)
(java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime)
(net.snowflake.client.jdbc SnowflakeSQLException)))

(set! *warn-on-reflection* true)
Expand Down Expand Up @@ -194,36 +194,49 @@
:DATETIME :type/DateTime
:TIME :type/Time
:TIMESTAMP :type/DateTime
;; This is a weird one. A timestamp with local time zone, stored without time zone but treated as being in the
;; Session time zone for filtering purposes etc.
:TIMESTAMPLTZ :type/DateTime
;; timestamp with no time zone
:TIMESTAMPNTZ :type/DateTime
:TIMESTAMPTZ :type/DateTimeWithTZ
;; timestamp with time zone normalized to UTC, similar to Postgres
:TIMESTAMPTZ :type/DateTimeWithLocalTZ
:VARIANT :type/*
;; Maybe also type *
:OBJECT :type/Dictionary
:ARRAY :type/*} base-type))

(defmethod sql.qp/unix-timestamp->honeysql [:snowflake :seconds] [_ _ expr] [:to_timestamp expr])
(defmethod sql.qp/unix-timestamp->honeysql [:snowflake :milliseconds] [_ _ expr] [:to_timestamp expr 3])
(defmethod sql.qp/unix-timestamp->honeysql [:snowflake :microseconds] [_ _ expr] [:to_timestamp expr 6])
(defmethod sql.qp/unix-timestamp->honeysql [:snowflake :seconds] [_ _ expr] [:to_timestamp_tz expr])
(defmethod sql.qp/unix-timestamp->honeysql [:snowflake :milliseconds] [_ _ expr] [:to_timestamp_tz expr 3])
(defmethod sql.qp/unix-timestamp->honeysql [:snowflake :microseconds] [_ _ expr] [:to_timestamp_tz expr 6])

(defmethod sql.qp/add-interval-honeysql-form :snowflake
[_ hsql-form amount unit]
[:dateadd
[:raw (name unit)]
[:raw (int amount)]
(h2x/->timestamp hsql-form)])
[_driver hsql-form amount unit]
;; return type is always the same as expr type, unless expr is a DATE and you're adding something not in a DATE e.g.
;; `:seconds`, in which case it returns `timestamp_ntz`. See
;; https://docs.snowflake.com/en/sql-reference/functions/dateadd
(let [db-type (h2x/database-type hsql-form)
return-type (if (and (= db-type "date")
(not (contains? lib.schema.temporal-bucketing/date-bucketing-units unit)))
"timestamp_ntz"
db-type)]
(-> [:dateadd
[:raw (name unit)]
[:inline (int amount)]
hsql-form]
(h2x/with-database-type-info return-type))))

(defn- extract
[unit expr]
(-> [:date_part unit (h2x/->timestamp expr)]
(-> [:date_part unit expr]
(h2x/with-database-type-info "integer")))

(defn- date-trunc
[unit expr]
(let [acceptable-types (case unit
(:millisecond :second :minute :hour) #{"time" "timestamp"}
(:day :week :month :quarter :year) #{"date" "timestamp"})
expr (h2x/cast-unless-type-in "timestamp" acceptable-types expr)]
(:millisecond :second :minute :hour) #{"time" "timestampltz" "timestampntz" "timestamptz"}
(:day :week :month :quarter :year) #{"date" "timestampltz" "timestampntz" "timestamptz"})
expr (h2x/cast-unless-type-in "timestampntz" acceptable-types expr)]
(-> [:date_trunc unit expr]
(h2x/with-database-type-info (h2x/database-type expr)))))

Expand All @@ -232,7 +245,7 @@
(defmethod sql.qp/date [:snowflake :minute-of-hour] [_ _ expr] (extract :minute expr))
(defmethod sql.qp/date [:snowflake :hour] [_ _ expr] (date-trunc :hour expr))
(defmethod sql.qp/date [:snowflake :hour-of-day] [_ _ expr] (extract :hour expr))
(defmethod sql.qp/date [:snowflake :day] [_ _ expr] (date-trunc :day expr))
(defmethod sql.qp/date [:snowflake :day] [_ _ expr] (h2x/->date expr))
(defmethod sql.qp/date [:snowflake :day-of-month] [_ _ expr] (extract :day expr))
(defmethod sql.qp/date [:snowflake :day-of-year] [_ _ expr] (extract :dayofyear expr))
(defmethod sql.qp/date [:snowflake :month] [_ _ expr] (date-trunc :month expr))
Expand All @@ -256,7 +269,7 @@
;; we don't support it at the moment because the implementation in (defmethod date [:sql :week-of-year-us])
;; relies on the ability to dynamicall change `start-of-week` setting, but with snowflake we set the
;; start-of-week in connection session instead of manipulate in MBQL
(throw (ex-info (tru "sqlite doesn't support extract us week")
(throw (ex-info (tru "Snowflake doesn''t support extract us week")
{:driver driver
:form expr
:type qp.error-type/invalid-query})))
Expand Down Expand Up @@ -413,6 +426,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" (u.date/format t))])

(defmethod sql.qp/->honeysql [:snowflake LocalTime]
[_driver t]
[:raw (format "'%s'::time" (u.date/format "HH:mm:ss.SSS" 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'::timestamp_ntz" (u.date/format "yyyy-MM-dd HH:mm:ss.SSS" t))])

(defmethod sql.qp/->honeysql [:snowflake OffsetDateTime]
[_driver t]
[:raw (format "'%s'::timestamp_tz" (u.date/format "yyyy-MM-dd HH:mm:ss.SSS xx" 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 +614,23 @@
(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

;;; If you try to read a Snowflake `timestamptz` as a String with `.getString` it always comes back in
;;; `America/Los_Angeles` for some reason I cannot figure out. Let's just read them out as UTC, which is what they're
;;; stored as internally anyway, and let the format-rows middleware adjust the timezone as needed
(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))))

;; 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

[_driver ^ResultSet rs _rsmeta ^Integer i]
;; if we don't explicitly specify the Calendar then it looks like it defaults to the system timezone, we don't really
;; want that now do we.
(let [utc-calendar (java.util.Calendar/getInstance (java.util.TimeZone/getTimeZone "UTC"))]
(fn []
(some-> (.getTimestamp rs i utc-calendar)
t/instant
(t/offset-date-time (t/zone-offset 0))))))

;;; --------------------------------------------------- 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
120 changes: 91 additions & 29 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 @@ -60,7 +64,7 @@
(is (= [100]
(mt/first-row
(mt/run-mbql-query venues
{:aggregation [[:count]]}))))))
{:aggregation [[:count]]}))))))

(deftest ^:parallel quote-name-test
(is (nil? (#'driver.snowflake/quote-name nil)))
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