-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix Snowflake/Redshift filtering on date for timestamp with time zone columns #41864
Merged
camsaul
merged 12 commits into
master
from
fix-snowflake-filtering-for-timestamp-tz-columns
May 1, 2024
Merged
Changes from 4 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 994c6e2
Snowflake should just generate raw SQL for literals
camsaul b7e3d1c
Go ahead and fix other Snowflake stuff and reenable a bunch of disabl…
camsaul e97ab7d
Merge branch 'master' into fix-snowflake-filtering-for-timestamp-tz-c…
camsaul 589d38a
Merge branch 'master' into fix-snowflake-filtering-for-timestamp-tz-c…
camsaul a659256
Snowflake test fixes :wrench:
camsaul 2b10084
Minor test improvements
camsaul d8652ba
Fix Redshift and Vertica as well.
camsaul 5aca3c3
Address PR feedback
camsaul 3759a5a
Test fixes :wrench:
camsaul 11bfe63
Better Snowflake fix :wrench:
camsaul 3e0903e
More test fixes :wrench:
camsaul File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] | ||
|
@@ -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) | ||
|
@@ -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))))) | ||
|
||
|
@@ -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)) | ||
|
@@ -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}))) | ||
|
@@ -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 [:*] | ||
|
@@ -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))))) | ||
|
||
;; 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
No longer needed since we don't parameterize these anymore.