Add server side generated relative datetime capabilities to Snowflake (#38779)

* Remove duplicate definition

* Extend server side relative datetime generation to Snowflake

* Cleanup

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

@@ -18,11 +18,10 @@
    [metabase.mbql.util :as mbql.u]
    [metabase.public-settings :as public-settings]
    [metabase.query-processor.store :as qp.store]
-   [metabase.query-processor.timezone :as qp.timezone]
    [metabase.query-processor.util :as qp.util]
+   [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.i18n :refer [trs]]
    [metabase.util.log :as log])
@@ -217,33 +216,9 @@
         y (h2x/->timestamp y)]
     (sql.qp/datetime-diff driver unit x y)))
 
-(defn- use-server-side-relative-datetime?
-  "Check whether server side :relative-datetime clause should be computed server side.
-   Units are [[metabase.util.date-2/add-units]] greater or equal to day."
-  [unit]
-  (contains? #{:day :week :month :quarter :year} unit))
-
-(defn- server-side-relative-datetime-honeysql-form
-  "Compute `:relative-datetime` clause value server-side. Value is sql formatted (and not passed as date time) to avoid
-   jdbc driver's timezone adjustments. Use of `qp.timezone/now` ensures correct timezone is used for the calculation.
-   For details see the [[metabase.driver.redshift-test/server-side-relative-datetime-truncation-test]]. Use of
-   server-side generated values instead of `getdate` Redshift function enables caching."
-  [amount unit]
-  [:cast
-   (-> (qp.timezone/now)
-       (u.date/truncate unit)
-       (u.date/add unit amount)
-       (u.date/format-sql))
-   :timestamp])
-
 (defmethod sql.qp/->honeysql [:redshift :relative-datetime]
   [driver [_ amount unit]]
-  (if (use-server-side-relative-datetime? unit)
-    (server-side-relative-datetime-honeysql-form amount unit)
-    (let [now-hsql (sql.qp/current-datetime-honeysql-form driver)]
-      (sql.qp/date driver unit (if (zero? amount)
-                                 now-hsql
-                                 (sql.qp/add-interval-honeysql-form driver now-hsql amount unit))))))
+  (qp.relative-datetime/maybe-cacheable-relative-datetime-honeysql driver unit amount))
 
 (defmethod sql.qp/datetime-diff [:redshift :year]
   [driver _unit x y]

modules/drivers/redshift/test/metabase/driver/redshift_test.clj

@@ -3,8 +3,6 @@
    [clojure.java.jdbc :as jdbc]
    [clojure.string :as str]
    [clojure.test :refer :all]
-   [honey.sql :as sql]
-   [java-time.api :as t]
    [metabase.driver :as driver]
    [metabase.driver.redshift :as redshift]
    [metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
@@ -19,13 +17,11 @@
    [metabase.plugins.jdbc-proxy :as jdbc-proxy]
    [metabase.public-settings :as public-settings]
    [metabase.query-processor :as qp]
-   [metabase.query-processor.test-util :as qp.test-util]
    [metabase.sync :as sync]
    [metabase.test :as mt]
    [metabase.test.data.interface :as tx]
    [metabase.test.data.redshift :as redshift.test]
    [metabase.test.fixtures :as fixtures]
-   [metabase.test.util.timezone :as test.tz]
    [metabase.util :as u]
    [metabase.util.honey-sql-2 :as h2x]
    [metabase.util.log :as log]
@@ -484,94 +480,3 @@
                        qual-view-name
                        qual-mview-name
                        username)))))))))
-
-;;;; Server side generated timestamps for :relative-datetime tests follow.
-
-(defn- run-native-query [sql & params]
-  (-> (mt/native-query {:query sql
-                        :params params})
-      qp/process-query))
-
-(defn- getdate-vs-ss-ts-test-thunk-generator
-  [unit value]
-  (fn []
-    ;; `with-redefs` forces use of `gettime()` in :relative-datetime transformation even for units gte or eq to :day.
-    ;; This was standard before PR #38604, now server side timestamps are used for that. This test confirms that
-    ;; server side generated timestamp (ie. new code path) results are equal to old code path results, that were not
-    ;; cacheable.
-    (let [honey {:select [[(with-redefs [redshift/use-server-side-relative-datetime? (constantly false)]
-                             (sql.qp/->honeysql :redshift [:relative-datetime value unit]))]
-                          [(sql.qp/->honeysql :redshift [:relative-datetime value unit])]]}
-          sql (sql/format honey)
-          result (apply run-native-query sql)
-          [db-generated ss-generated] (-> result mt/rows first)]
-      (is (= db-generated ss-generated)))))
-
-(deftest server-side-relative-datetime-test
-  (mt/test-driver
-   :redshift
-   (testing "Values of getdate() and server side generated timestamp are equal"
-     (mt/with-metadata-provider (mt/id)
-       (let [test-thunk (getdate-vs-ss-ts-test-thunk-generator :week -1)]
-         (doseq [tz-setter [qp.test-util/do-with-report-timezone-id
-                            test.tz/do-with-system-timezone-id
-                            qp.test-util/do-with-database-timezone-id
-                            qp.test-util/do-with-results-timezone-id]
-                 timezone ["America/Los_Angeles"
-                           "Europe/Prague"
-                           "UTC"]]
-           (testing (str tz-setter " " timezone)
-             (tz-setter timezone test-thunk))))))))
-
-;; Other configurations of timezone settings were also tested with values UTC America/Los_Angeles Europe/Prague.
-;; Test containing all configurations took ~500 seconds. Leaving here only one random configuration to be
-;; included in CI tests.
-(deftest server-side-relative-datetime-multiple-tz-settings-test
-  (mt/test-driver
-   :redshift
-   (mt/with-metadata-provider (mt/id)
-     (testing "Value of server side generated timestamp matches the one from getdate() with multiple timezone settings"
-       (mt/with-results-timezone-id "UTC"
-         (mt/with-database-timezone-id "America/Los_Angeles"
-           (mt/with-report-timezone-id "America/Los_Angeles"
-             (mt/with-system-timezone-id "Europe/Prague"
-               (let [test-thunk (getdate-vs-ss-ts-test-thunk-generator :week -1)]
-                 (test-thunk))))))))))
-
-(deftest server-side-relative-datetime-various-units-test
-  (mt/test-driver
-   :redshift
-   (mt/with-metadata-provider (mt/id)
-     (testing "Value of server side generated timestamp matches the one from getdate() with multiple timezone settings"
-       ;; Units are [[metabase.driver.redshift/server-side-relative-datetime-units]] in defined order.
-       (doseq [unit [:day :week :month :quarter :year]
-               value [-30 0 7]
-               :let [test-thunk (getdate-vs-ss-ts-test-thunk-generator unit value)]]
-         (test-thunk))))))
-
-(deftest server-side-relative-datetime-truncation-test
-  (mt/test-driver
-   :redshift
-   (testing "Datetime _truncation_ works correctly over different timezones"
-     ;; Sunday is the first week day. System is in UTC and has 2014 Aug 10 Sunday 12:30:01 AM. Report is required
-     ;; for New York, where there's still Saturday. So the time span that we'd like to see the results for
-     ;; is 2014 Jul 27 12:00 AM <= x < 2014 Aug 03 12:00 AM. If we were using local date as a base
-     ;; (in redshift/server-side-relative-datetime-honeysql-form), that would be correctly adjusted by the jdbc driver
-     ;; to match timezone of the session. However that adjustment would come _after the truncation and addition_
-     ;; that :relative-datetime does, hence would produce incorrect results. This test verifies the situation
-     ;; is correctly handled.
-     (mt/with-report-timezone-id "America/New_York"
-       (mt/with-system-timezone-id "UTC"
-         (mt/with-clock (t/zoned-date-time (t/local-date-time 2014 8 10 0 30 1 0) "UTC")
-           (is (= [[13 "Dwight Gresham" "2014-08-01T10:30:00-04:00"]
-                   [15 "Rüstem Hebel" "2014-08-01T12:45:00-04:00"]
-                   [7 "Conchúr Tihomir" "2014-08-02T09:30:00-04:00"]
-                   [6 "Shad Ferdynand" "2014-08-02T12:30:00-04:00"]]
-                  (->> (mt/run-mbql-query
-                        test_data_users
-                        {:fields [$id $name $last_login]
-                         :filter [:and
-                                  [:>= $last_login [:relative-datetime -1 :week]]
-                                  [:< $last_login [:relative-datetime 0 :week]]]
-                         :order-by [[:asc $last_login]]})
-                       (mt/formatted-rows [int str str]))))))))))

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

@@ -32,6 +32,7 @@
    [metabase.query-processor.timezone :as qp.timezone]
    [metabase.query-processor.util :as qp.util]
    [metabase.query-processor.util.add-alias-info :as add]
+   [metabase.query-processor.util.relative-datetime :as qp.relative-datetime]
    [metabase.util :as u]
    [metabase.util.date-2 :as u.date]
    [metabase.util.honey-sql-2 :as h2x]
@@ -199,10 +200,6 @@
 (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/current-datetime-honeysql-form :snowflake
-  [_]
-  (h2x/with-database-type-info :%current_timestamp :TIMESTAMPTZ))
-
 (defmethod sql.qp/add-interval-honeysql-form :snowflake
   [_ hsql-form amount unit]
   [:dateadd
@@ -406,6 +403,10 @@
            [:convert_timezone (or source-timezone (qp.timezone/results-timezone-id)) target-timezone hsql-form]])
         (h2x/with-database-type-info "timestampntz"))))
 
+(defmethod sql.qp/->honeysql [:snowflake :relative-datetime]
+  [driver [_ amount unit]]
+  (qp.relative-datetime/maybe-cacheable-relative-datetime-honeysql driver unit amount))
+
 (defmethod driver/table-rows-seq :snowflake
   [driver database table]
   (sql-jdbc/query driver database {:select [:*]

src/metabase/query_processor/util/relative_datetime.clj

+(ns metabase.query-processor.util.relative-datetime
+  "Utility function for server side relative datetime computation."
+  (:require
+   [metabase.driver.sql.query-processor :as sql.qp]
+   [metabase.query-processor.timezone :as qp.timezone]
+   [metabase.util.date-2 :as u.date]))
+
+(defn- use-server-side-relative-datetime?
+  "Check whether :relative-datetime clause could be computed server side. True for [[u.date/add-units]] greater than
+   or equal to day."
+  [unit]
+  (contains? #{:day :week :month :quarter :year} unit))
+
+(defn- relative-datetime-sql-str
+  "Compute relative datetime from [[qp.timezone/now]] shifted by `unit` and `amount`. Format the resulting value
+   to literal string compatible with most sql databases, to avoid possible jdbc driver timezone conversions."
+  [unit amount]
+  (-> (qp.timezone/now)
+      (u.date/truncate unit)
+      (u.date/add unit amount)
+      (u.date/format-sql)))
+
+;; NOTE: At the time of writing, we are supporting Snowflake and Redshift which could share the logic.
+(defn maybe-cacheable-relative-datetime-honeysql
+  "Return honeysql form for relative datetime clasue, that is cacheable -- values of `getdate` or `current_timestamp`
+   are computed server side."
+  [driver unit amount]
+  (if (use-server-side-relative-datetime? unit)
+    ;; In Snowflake, timestamp is user-specified alias to timestamp_ntz (default), timestamp_ltz or timestamp_tz.
+    ;; For more info see the docs: https://docs.snowflake.com/en/sql-reference/data-types-datetime#timestamp
+    ;; We do not have to care which timestamp is currently aliased, because [[relative-datetime-sql-str]]
+    ;; generates the now timestamp in the same timezone as `current_timestamp` or `getdate` function would.
+    [:cast (relative-datetime-sql-str unit amount) :timestamp]
+    ((get-method sql.qp/->honeysql [:sql :relative-datetime]) driver [:relative-datetime amount unit])))

test/metabase/query_processor/util/relative_datetime_test.clj

+(ns metabase.query-processor.util.relative-datetime-test
+  (:require
+   [clojure.test :refer :all]
+   [honey.sql :as sql]
+   [java-time.api :as t]
+   [metabase.driver :as driver]
+   [metabase.driver.sql.query-processor :as sql.qp]
+   [metabase.query-processor :as qp]
+   [metabase.query-processor.test-util :as qp.test-util]
+   [metabase.query-processor.util.relative-datetime :as qp.relative-datetime]
+   [metabase.test :as mt]
+   [metabase.test.util.timezone :as test.tz]))
+
+(defn- run-native-query [sql & params]
+  (-> (mt/native-query {:query sql
+                        :params params})
+      qp/process-query))
+
+(defn- getdate-vs-ss-ts-test-thunk-generator
+  [unit value]
+  (fn []
+    ;; `with-redefs` forces use of `gettime()` in :relative-datetime transformation even for units gte or eq to :day.
+    ;; This was standard before PR #38604, now server side timestamps are used for that. This test confirms that
+    ;; server side generated timestamp (ie. new code path) results are equal to old code path results, that were not
+    ;; cacheable.
+    (let [honey {:select [[(with-redefs [qp.relative-datetime/use-server-side-relative-datetime? (constantly false)]
+                             (sql.qp/->honeysql driver/*driver* [:relative-datetime value unit]))]
+                          [(sql.qp/->honeysql driver/*driver* [:relative-datetime value unit])]]}
+          sql (sql/format honey)
+          result (apply run-native-query sql)
+          [db-generated ss-generated] (-> result mt/rows first)]
+      (is (= db-generated ss-generated)))))
+
+(deftest server-side-relative-datetime-test
+  (mt/test-drivers
+   #{:redshift :snowflake}
+   (testing "Values of getdate() and server side generated timestamp are equal"
+     (mt/with-metadata-provider (mt/id)
+       (let [test-thunk (getdate-vs-ss-ts-test-thunk-generator :week -1)]
+         (doseq [tz-setter [qp.test-util/do-with-report-timezone-id
+                            test.tz/do-with-system-timezone-id
+                            qp.test-util/do-with-database-timezone-id
+                            qp.test-util/do-with-results-timezone-id]
+                 timezone ["America/Los_Angeles"
+                           "Europe/Prague"
+                           "UTC"]]
+           (testing (str tz-setter " " timezone)
+             (tz-setter timezone test-thunk))))))))
+
+(deftest server-side-relative-datetime-multiple-tz-settings-test
+  (mt/test-drivers
+   #{:redshift :snowflake}
+   (mt/with-metadata-provider (mt/id)
+     (testing "Value of server side generated timestamp matches the one from getdate() with multiple timezone settings"
+       (mt/with-results-timezone-id "UTC"
+         (mt/with-database-timezone-id "America/Los_Angeles"
+           (mt/with-report-timezone-id "America/Los_Angeles"
+             (mt/with-system-timezone-id "Europe/Prague"
+               (let [test-thunk (getdate-vs-ss-ts-test-thunk-generator :week -1)]
+                 (test-thunk))))))))))
+
+(deftest server-side-relative-datetime-various-units-test
+  (mt/test-drivers
+   #{:redshift :snowflake}
+   (mt/with-metadata-provider (mt/id)
+     (testing "Value of server side generated timestamp matches the one from getdate() with multiple timezone settings"
+       (doseq [unit [:day :week :month :quarter :year]
+               value [-30 0 7]
+               :let [test-thunk (getdate-vs-ss-ts-test-thunk-generator unit value)]]
+         (test-thunk))))))
+
+(deftest server-side-relative-datetime-truncation-test
+  (mt/test-drivers
+   #{:redshift :snowflake}
+   (testing "Datetime _truncation_ works correctly over different timezones"
+     ;; Sunday is the first week day. System is in UTC and has 2014 Aug 10 Sunday 12:30:01 AM. Report is required
+     ;; for New York, where there's still Saturday. So the time span that we'd like to see the results for
+     ;; is 2014 Jul 27 12:00 AM <= x < 2014 Aug 03 12:00 AM. If we were using local date as a base
+     ;; (in driver.snowflake/server-side-relative-datetime-honeysql-form), that would be correctly adjusted by the jdbc driver
+     ;; to match timezone of the session. However that adjustment would come _after the truncation and addition_
+     ;; that :relative-datetime does, hence would produce incorrect results. This test verifies the situation
+     ;; is correctly handled.
+     (mt/with-report-timezone-id "America/New_York"
+       (mt/with-system-timezone-id "UTC"
+         (mt/with-clock (t/zoned-date-time (t/local-date-time 2014 8 10 0 30 1 0) "UTC")
+           (is (= [[13 "Dwight Gresham" "2014-08-01T10:30:00-04:00"]
+                   [15 "Rüstem Hebel" "2014-08-01T12:45:00-04:00"]
+                   [7 "Conchúr Tihomir" "2014-08-02T09:30:00-04:00"]
+                   [6 "Shad Ferdynand" "2014-08-02T12:30:00-04:00"]]
+                  (->> (mt/run-mbql-query
+                        users
+                        {:fields [$id $name $last_login]
+                         :filter [:and
+                                  [:>= $last_login [:relative-datetime -1 :week]]
+                                  [:< $last_login [:relative-datetime 0 :week]]]
+                         :order-by [[:asc $last_login]]})
+                       (mt/formatted-rows [int str str]))))))))))