From ee219e5c93479871b4226b5796d73be74ba35795 Mon Sep 17 00:00:00 2001
From: lbrdnk <lbrdnk@users.noreply.github.com>
Date: Tue, 13 Feb 2024 15:38:20 +0100
Subject: [PATCH] Use server side generated timestamp for relative datetime
computation on Redshift v2 (#38604)
* Restore commit 16988f50d6, server side generated timestamps for Redshift (#35995)
* Remove redundant test -- caching is best effort
* Make comments more descriptive
* Remove 0 arity from testcase generator function
* Move units into `use-server-side-relative-datetime?` definition
---
.../redshift/src/metabase/driver/redshift.clj | 30 ++++++
.../test/metabase/driver/redshift_test.clj | 95 +++++++++++++++++++
2 files changed, 125 insertions(+)
diff --git a/modules/drivers/redshift/src/metabase/driver/redshift.clj b/modules/drivers/redshift/src/metabase/driver/redshift.clj
index 71590c5c3fb..c4a78847082 100644
--- a/modules/drivers/redshift/src/metabase/driver/redshift.clj
+++ b/modules/drivers/redshift/src/metabase/driver/redshift.clj
@@ -18,9 +18,11 @@
[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.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])
@@ -215,6 +217,34 @@
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))))))
+
(defmethod sql.qp/datetime-diff [:redshift :year]
[driver _unit x y]
(h2x// (sql.qp/datetime-diff driver :month x y) 12))
diff --git a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
index 02dcc585ccf..ae7e24cd61d 100644
--- a/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
+++ b/modules/drivers/redshift/test/metabase/driver/redshift_test.clj
@@ -3,6 +3,8 @@
[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]
@@ -17,11 +19,13 @@
[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]
@@ -480,3 +484,94 @@
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]))))))))))
--
GitLab