diff --git a/modules/drivers/redshift/src/metabase/driver/redshift.clj b/modules/drivers/redshift/src/metabase/driver/redshift.clj index 89aeca70536d249beb345ef862f0023986ea6c59..ec5708cffd3a1513de21db6919140b1893feb617 100644 --- a/modules/drivers/redshift/src/metabase/driver/redshift.clj +++ b/modules/drivers/redshift/src/metabase/driver/redshift.clj @@ -17,7 +17,9 @@ [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.util.date-2 :as u.date] [metabase.util.honey-sql-2 :as h2x] [metabase.util.i18n :refer [trs]] [metabase.util.log :as log]) @@ -208,6 +210,32 @@ y (h2x/->timestamp y)] (sql.qp/datetime-diff driver unit x y))) +(defn- use-server-side-relative-datetime? + "Server side generated timestamp in :relative-datetime should be used with following units. Units gt or eq 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]]." + [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 19e730309724130be4e627ba6765a898d2718308..0f8d5af77763bcc937ac11477ff4d3f0e99badb0 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,12 +19,14 @@ [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.random :as tu.random] + [metabase.test.util.timezone :as test.tz] [metabase.util :as u] [metabase.util.honey-sql-2 :as h2x] #_{:clj-kondo/ignore [:discouraged-namespace :deprecated-namespace]} @@ -391,3 +395,149 @@ (mt/first-row (qp/process-query (mt/native-query {:query "select interval '5 days'"})))))))) + +;;;; 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- session-id [] + (-> "select pg_backend_pid()" run-native-query mt/rows ffirst)) + +(defn- last-queries-of-a-session [session-id] + (let [query-str (str "select result_cache_hit, query_text\n" + "from sys_query_history\n" + "where session_id = ? \n" + "order by start_time desc\n" + "limit 3\n")] + (->> (run-native-query query-str session-id) + mt/rows + (map (partial zipmap [:result-cache-hit :query-text]))))) + +;; According to https://docs.aws.amazon.com/redshift/latest/dg/c_challenges_achieving_high_performance_queries.html#result-caching +;; caching is on by default. Hence check for that is skipped. +(deftest server-side-timestamp-caching-test + (mt/test-driver :redshift + (testing "Relative date time queries should be cached" + (sql-jdbc.execute/do-with-connection-with-options + :redshift + (mt/id) + nil + (fn [_] + (let [sid (session-id) + ;; :time-interval translates to :relative-datetime, which is now using server side generated timestamps + ;; for units day and greater. + query-to-cache (mt/mbql-query + test_data_users + {:fields [$id $last_login] + :filter [:time-interval $last_login -500 :day {:include-current false}]}) + compiled-sql (-> query-to-cache qp/compile :query)] + ;; BEWARE: Following expression has a flake potential. If that happens, see the discussion in attached link + ;; for more info. https://github.com/metabase/metabase/pull/35995#discussion_r1409850657 + (dotimes [_ 2] + (qp/process-query query-to-cache)) + (let [last-queries (last-queries-of-a-session sid) + ;; First row of last queries should contain "last-queries query" info. + ;; Second should contain info about query that should be returned from cache. + cached-row (second last-queries) + cached-row-adjusted-redshift-sql (str/replace (:query-text cached-row) #"\$\d+" "?") + ;; Third row contains the same query. At the point of its execution, it may have been cached, but + ;; afterwards it should. + pre-cached-row (nth last-queries 2) + pre-cached-row-adjusted-redshift-sql (str/replace (:query-text pre-cached-row) #"\$\d+" "?")] + (testing "Last queries results contain query before caching on expected postion" + (is (str/includes? pre-cached-row-adjusted-redshift-sql compiled-sql))) + (testing "Last queries results contain cached query on expected position" + (is (str/includes? cached-row-adjusted-redshift-sql compiled-sql))) + ;; BEWARE: There is a potential that following expr could make the test flaky -- hard to guarantee that db + ;; will actually cache the result. If that happens, we should reconsider testing strategy + ;; or remove this test completely. + (testing "Query was returned from cache" + (is (true? (:result-cache-hit cached-row))))))))))) + +(defn- getdate-vs-ss-ts-test-thunk-generator + ([] + (getdate-vs-ss-ts-test-thunk-generator :week -1)) + ([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 #35995, 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)] + (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)] + (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" + (doseq [unit [:day :week :month :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]))))))))))