Fix Snowflake/Redshift filtering on date for timestamp with time zone…

… columns (#41864)

* Fix Snowflake filtering on date for `timestamp with time zone` columns

* Snowflake should just generate raw SQL for literals

* Go ahead and fix other Snowflake stuff and reenable a bunch of disabled tests

* Snowflake test fixes 🔧

* Minor test improvements

* Fix Redshift and Vertica as well.

* Address PR feedback

modules/drivers/presto-jdbc/test/metabase/driver/presto_jdbc_test.clj

@@ -129,13 +129,8 @@
   (mt/test-driver :presto-jdbc
     (testing "Make sure date params work correctly when report timezones are set (#10487)"
       (mt/with-temporary-setting-values [report-timezone "Asia/Hong_Kong"]
-        ;; the `read-column-thunk` for `Types/TIMESTAMP` always returns an `OffsetDateTime`, not a `LocalDateTime`, as
-        ;; the original Presto version of this test expected; therefore, convert the `ZonedDateTime` corresponding to
-        ;; midnight on this date (at the report TZ) to `OffsetDateTime` for comparison's sake
-        (is (= [[(-> (t/zoned-date-time 2014 8 2 0 0 0 0 (t/zone-id "Asia/Hong_Kong"))
-                     t/offset-date-time
-                     (t/with-offset-same-instant (t/zone-offset 0)))
-                 (t/local-date 2014 8 2)]]
+        (is (= [[(t/local-date "2014-08-02")
+                 (t/local-date "2014-08-02")]]
                (mt/rows
                  (qp/process-query
                    {:database     (mt/id)

modules/drivers/redshift/src/metabase/driver/redshift.clj

@@ -22,12 +22,12 @@
    [metabase.query-processor.util.relative-datetime :as qp.relative-datetime]
    [metabase.upload :as upload]
    [metabase.util :as u]
+   [metabase.util.date-2 :as u.date]
    [metabase.util.honey-sql-2 :as h2x]
    [metabase.util.log :as log])
   (:import
    (com.amazon.redshift.util RedshiftInterval)
-   (java.sql Connection PreparedStatement ResultSet ResultSetMetaData Types)
-   (java.time OffsetTime)))
+   (java.sql Connection PreparedStatement ResultSet ResultSetMetaData Types)))
 
 (set! *warn-on-reflection* true)
 
@@ -315,6 +315,35 @@
   [driver [_ amount unit]]
   (qp.relative-datetime/maybe-cacheable-relative-datetime-honeysql driver unit amount))
 
+(defmethod sql.qp/->honeysql [:redshift java.time.LocalDate]
+  [_driver t]
+  (-> [:raw (format "date '%s'" (u.date/format t))]
+      (h2x/with-database-type-info "date")))
+
+(defmethod sql.qp/->honeysql [:redshift java.time.LocalTime]
+  [_driver t]
+  (-> [:raw (format "time '%s'" (u.date/format "HH:mm:ss.SSS" t))]
+      (h2x/with-database-type-info "time")))
+
+(defmethod sql.qp/->honeysql [:redshift java.time.OffsetTime]
+  [_driver t]
+  (-> [:raw (format "time with time zone '%s'" (u.date/format "HH:mm:ss.SSS xxx" t))]
+      (h2x/with-database-type-info "timetz")))
+
+(defmethod sql.qp/->honeysql [:redshift java.time.LocalDateTime]
+  [_driver t]
+  (-> [:raw (format "timestamp '%s'" (u.date/format "yyyy-MM-dd HH:mm:ss.SSS" t))]
+      (h2x/with-database-type-info "timestamp")))
+
+(defmethod sql.qp/->honeysql [:redshift java.time.OffsetDateTime]
+  [_driver t]
+  (-> [:raw (format "timestamp with time zone '%s'" (u.date/format "yyyy-MM-dd HH:mm:ss.SSS xxx" t))]
+      (h2x/with-database-type-info "timestamptz")))
+
+(defmethod sql.qp/->honeysql [:redshift java.time.ZonedDateTime]
+  [driver t]
+  (sql.qp/->honeysql driver (t/offset-date-time t)))
+
 (defmethod sql.qp/datetime-diff [:redshift :year]
   [driver _unit x y]
   (h2x// (sql.qp/datetime-diff driver :month x y) 12))
@@ -395,10 +424,12 @@
  [::sql-jdbc.legacy/use-legacy-classes-for-read-and-set Types/TIME]
  [:postgres Types/TIME])
 
+;;; I don't think this should actually ever get called because we should be compiling an `OffsetTime` as a `timetz`
+;;; literal
 (prefer-method
  sql-jdbc.execute/set-parameter
- [::sql-jdbc.legacy/use-legacy-classes-for-read-and-set OffsetTime]
- [:postgres OffsetTime])
+ [::sql-jdbc.legacy/use-legacy-classes-for-read-and-set java.time.OffsetTime]
+ [:postgres java.time.OffsetTime])
 
 (defn- field->parameter-value
   "Map fields used in parameters to parameter `:value`s."

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

@@ -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)
@@ -197,52 +197,83 @@
     :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- in-report-timezone
+  "Convert timestamps with time zones (`timestamp_tz`) to timestamps that should be interpreted in the session
+  timezone (`timestamp_ltz`) so various datetime extraction and truncation operations work as expected."
+  [expr]
+  (let [report-timezone (qp.timezone/report-timezone-id-if-supported)]
+    (if (and report-timezone
+             (= (h2x/database-type expr) "timestamptz"))
+      [:to_timestamp_ltz expr]
+      expr)))
 
 (defn- extract
   [unit expr]
-  (-> [:date_part unit (h2x/->timestamp expr)]
+  (-> [:date_part (h2x/literal unit) (in-report-timezone 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)]
-    (-> [:date_trunc unit 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 (h2x/literal unit) (in-report-timezone expr)]
         (h2x/with-database-type-info (h2x/database-type expr)))))
 
 (defmethod sql.qp/date [:snowflake :default]         [_ _ expr] expr)
 (defmethod sql.qp/date [:snowflake :minute]          [_ _ expr] (date-trunc :minute expr))
 (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-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))
 (defmethod sql.qp/date [:snowflake :month-of-year]   [_ _ expr] (extract :month expr))
 (defmethod sql.qp/date [:snowflake :quarter]         [_ _ expr] (date-trunc :quarter expr))
 (defmethod sql.qp/date [:snowflake :quarter-of-year] [_ _ expr] (extract :quarter expr))
 (defmethod sql.qp/date [:snowflake :year]            [_ _ expr] (date-trunc :year expr))
+(defmethod sql.qp/date [:snowflake :year-of-era]     [_ _ expr] (extract :year expr))
+
+(defmethod sql.qp/date [:snowflake :day]
+  [_driver _unit expr]
+  (if (= (h2x/database-type expr) "date")
+    expr
+    (date-trunc :day expr))
+  (-> [:to_date (date-trunc :day expr)]
+      (h2x/with-database-type-info "date")))
 
 ;; these don't need to be adjusted for start of week, since we're Setting the WEEK_START connection parameter
 (defmethod sql.qp/date [:snowflake :week]
@@ -259,7 +290,10 @@
   ;; 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")
+  ;;
+  ;; TODO -- what about the `weekofyear()` or `week()` functions in Snowflake? Would that do what we want?
+  ;; https://docs.snowflake.com/en/sql-reference/functions/year
+  (throw (ex-info (tru "Snowflake doesn''t support extract us week")
           {:driver driver
            :form   expr
            :type   qp.error-type/invalid-query})))
@@ -383,6 +417,8 @@
     (cond-> identifier
       qualify? qualify-identifier)))
 
+;;; TODO -- I don't think these actually ever get qualified since the parent method returns things wrapped
+;;; in [[h2x/with-database-type-info]] thus nothing will ever be an identifier.
 (defmethod sql.qp/->honeysql [:snowflake :field]
   [driver [_ _ {::add/keys [source-table]} :as field-clause]]
   (let [parent-method (get-method sql.qp/->honeysql [:sql :field])
@@ -416,6 +452,35 @@
   [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))]
+      (h2x/with-database-type-info "date")))
+
+(defmethod sql.qp/->honeysql [:snowflake LocalTime]
+  [_driver t]
+  (-> [:raw (format "'%s'::time" (u.date/format "HH:mm:ss.SSS" t))]
+      (h2x/with-database-type-info "time")))
+
+;;; 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))]
+     (h2x/with-database-type-info "timestampntz")))
+
+(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))]
+      (h2x/with-database-type-info "timestamptz")))
+
+(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 [:*]
@@ -579,65 +644,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")))))
+  [_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))