From 628a7c2c4ae3cf67e2f324d83601a887829d2f64 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Tue, 4 Feb 2020 10:29:41 -0800
Subject: [PATCH] Fix temporal type reconciliation logic for relative moments
in BigQuery (#11838)
[ci drivers]
---
.../bigquery/src/metabase/driver/bigquery.clj | 15 +-
.../driver/bigquery/query_processor.clj | 175 ++++++++++++++----
.../driver/bigquery/query_processor_test.clj | 141 +++++++++++++-
.../google/src/metabase/driver/google.clj | 15 +-
.../oracle/src/metabase/driver/oracle.clj | 22 ++-
.../presto/src/metabase/driver/presto.clj | 4 +-
.../redshift/src/metabase/driver/redshift.clj | 4 +-
.../src/metabase/driver/snowflake.clj | 4 +-
.../src/metabase/driver/hive_like.clj | 4 +-
.../sqlite/src/metabase/driver/sqlite.clj | 4 +-
.../src/metabase/driver/sqlserver.clj | 4 +-
.../vertica/src/metabase/driver/vertica.clj | 4 +-
src/metabase/driver.clj | 11 +-
src/metabase/driver/h2.clj | 6 +-
src/metabase/driver/mysql.clj | 4 +-
src/metabase/driver/postgres.clj | 4 +-
src/metabase/driver/sql/query_processor.clj | 37 +++-
src/metabase/util/date_2.clj | 2 +
18 files changed, 365 insertions(+), 95 deletions(-)
diff --git a/modules/drivers/bigquery/src/metabase/driver/bigquery.clj b/modules/drivers/bigquery/src/metabase/driver/bigquery.clj
index 745286e2dea..e99388ae242 100644
--- a/modules/drivers/bigquery/src/metabase/driver/bigquery.clj
+++ b/modules/drivers/bigquery/src/metabase/driver/bigquery.clj
@@ -12,6 +12,7 @@
[metabase.driver.google :as google]
[metabase.driver.sql.util.unprepare :as unprepare]
[metabase.query-processor
+ [error-type :as error-type]
[store :as qp.store]
[timezone :as qp.timezone]
[util :as qputil]]
@@ -20,8 +21,7 @@
(:import com.google.api.client.googleapis.auth.oauth2.GoogleCredential
com.google.api.client.http.HttpRequestInitializer
[com.google.api.services.bigquery Bigquery Bigquery$Builder BigqueryScopes]
- [com.google.api.services.bigquery.model QueryRequest QueryResponse Table TableCell TableFieldSchema TableList
- TableList$Tables TableReference TableRow TableSchema]
+ [com.google.api.services.bigquery.model QueryRequest QueryResponse Table TableCell TableFieldSchema TableList TableList$Tables TableReference TableRow TableSchema]
java.util.Collections))
(driver/register! :bigquery, :parent #{:google :sql})
@@ -204,9 +204,14 @@
(defn- process-native* [database query-string]
{:pre [(map? database) (map? (:details database))]}
;; automatically retry the query if it times out or otherwise fails. This is on top of the auto-retry added by
- ;; `execute` so operations going through `process-native*` may be retried up to 3 times.
- (u/auto-retry 1
- (post-process-native (execute-bigquery database query-string))))
+ ;; `execute`
+ (letfn [(thunk []
+ (post-process-native (execute-bigquery database query-string)))]
+ (try
+ (thunk)
+ (catch Throwable e
+ (when-not (error-type/client-error? (:type (u/all-ex-data e)))
+ (thunk))))))
(defn- effective-query-timezone-id [database]
(if (get-in database [:details :use-jvm-timezone])
diff --git a/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj b/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj
index 51440946dd9..54d7f2487da 100644
--- a/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj
+++ b/modules/drivers/bigquery/src/metabase/driver/bigquery/query_processor.clj
@@ -3,6 +3,7 @@
[clojure.tools.logging :as log]
[honeysql
[core :as hsql]
+ [format :as hformat]
[helpers :as h]]
[java-time :as t]
[metabase
@@ -154,7 +155,10 @@
(defmethod temporal-type :datetime-field
[[_ field unit]]
;; date extraction operations result in integers, so the type of the expression shouldn't be a temporal type
- (if (u.date/extract-units unit)
+ ;;
+ ;; `:year` is both an extract unit and a truncate unit in terms of `u.date` capabilities, but in MBQL it should be a
+ ;; truncation operation
+ (if ((disj u.date/extract-units :year) unit)
nil
(temporal-type field)))
@@ -166,7 +170,13 @@
[:field-id id] (temporal-type (qp.store/field id))
[:field-literal _ base-type] (base-type->temporal-type base-type))))
+(defn- with-temporal-type [x new-type]
+ (if (= (temporal-type x) new-type)
+ x
+ (vary-meta x assoc :bigquery/temporal-type new-type)))
+
(defmulti ^:private ->temporal-type
+ "Coerce `x` to target temporal type."
{:arglists '([target-type x])}
(fn [target-type x]
[target-type (mbql.u/dispatch-by-clause-name-or-class x)])
@@ -212,7 +222,7 @@
nil
(= (temporal-type x) target-type)
- (vary-meta x assoc :bigquery/temporal-type target-type)
+ (with-temporal-type x target-type)
:else
(let [hsql-form (sql.qp/->honeysql :bigquery x)
@@ -227,13 +237,12 @@
nil
(= (temporal-type hsql-form) target-type)
- (vary-meta hsql-form assoc :bigquery/temporal-type target-type)
+ (with-temporal-type hsql-form target-type)
bigquery-type
(do
- (log/tracef "Casting %s (temporal type = %s) to %s" (binding [*print-meta* true] (pr-str x)) (temporal-type x) bigquery-type)
- (with-meta (hx/cast bigquery-type (sql.qp/->honeysql :bigquery x))
- {:bigquery/temporal-type target-type}))
+ (log/tracef "Coercing %s (temporal type = %s) to %s" (binding [*print-meta* true] (pr-str x)) (pr-str (temporal-type x)) bigquery-type)
+ (with-temporal-type (hx/cast bigquery-type (sql.qp/->honeysql :bigquery x)) target-type))
:else
x))))
@@ -242,21 +251,49 @@
[target-type [_ t unit]]
[:absolute-datetime (->temporal-type target-type t) unit])
+(def ^:private temporal-type->supported-units
+ {:timestamp #{:microsecond :millisecond :second :minute :hour :day}
+ :datetime #{:microsecond :millisecond :second :minute :hour :day :week :month :quarter :year}
+ :date #{:day :week :month :quarter :year}
+ :time #{:microsecond :millisecond :second :minute :hour}})
+
+(defmethod ->temporal-type [:temporal-type :relative-datetime]
+ [target-type [_ _ unit :as clause]]
+ {:post [(= target-type (temporal-type %))]}
+ (with-temporal-type
+ ;; check and see whether we need to do a conversion. If so, use the parent method which will just wrap this in a
+ ;; cast statement.
+ (if ((temporal-type->supported-units target-type) unit)
+ clause
+ ((get-method ->temporal-type :default) target-type clause))
+ target-type))
+
+(defrecord ^:private TruncForm [hsql-form unit]
+ hformat/ToSql
+ (to-sql [_]
+ (let [t (or (temporal-type hsql-form) :datetime)
+ f (case t
+ :date :date_trunc
+ :time :time_trunc
+ :datetime :datetime_trunc
+ :timestamp :timestamp_trunc)]
+ (hformat/to-sql (hsql/call f (->temporal-type t hsql-form) (hsql/raw (name unit)))))))
+
+(defmethod temporal-type TruncForm
+ [trunc-form]
+ (temporal-type (:hsql-form trunc-form)))
+
+(defmethod ->temporal-type [:temporal-type TruncForm]
+ [target-type trunc-form]
+ (map->TruncForm (update trunc-form :hsql-form (partial ->temporal-type target-type))))
+
(defn- trunc
"Generate a SQL call an appropriate truncation function, depending on the temporal type of `expr`."
- [unit expr]
- (let [expr-type (or (temporal-type expr) :datetime)
- f (case expr-type
- :date :date_trunc
- :time :time_trunc
- :datetime :datetime_trunc
- :timestamp :timestamp_trunc)]
- (with-meta (hsql/call f (->temporal-type expr-type expr) (hsql/raw (name unit)))
- {:bigquery/temporal-type expr-type})))
+ [unit hsql-form]
+ (TruncForm. hsql-form unit))
(defn- extract [unit expr]
- (with-meta (hsql/call :extract unit (->temporal-type :timestamp expr))
- {:bigquery/temporal-type nil}))
+ (with-temporal-type (hsql/call :extract unit (->temporal-type :timestamp expr)) nil))
(defmethod sql.qp/date [:bigquery :minute] [_ _ expr] (trunc :minute expr))
(defmethod sql.qp/date [:bigquery :minute-of-hour] [_ _ expr] (extract :minute expr))
@@ -279,7 +316,7 @@
:milliseconds :timestamp_millis}]
(defmethod sql.qp/unix-timestamp->timestamp [:bigquery unix-timestamp-type]
[_ _ expr]
- (vary-meta (hsql/call bigquery-fn expr) assoc :bigquery/temporal-type :timestamp)))
+ (with-temporal-type (hsql/call bigquery-fn expr) :timestamp)))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -316,7 +353,7 @@
[driver field]
(let [parent-method (get-method sql.qp/->honeysql [:sql (class Field)])
identifier (parent-method driver field)]
- (vary-meta identifier assoc :bigquery/temporal-type (temporal-type field))))
+ (with-temporal-type identifier (temporal-type field))))
(defmethod sql.qp/->honeysql [:bigquery Identifier]
[_ identifier]
@@ -332,7 +369,14 @@
(defmethod sql.qp/->honeysql [:bigquery clause-type]
[driver clause]
(let [hsql-form ((get-method sql.qp/->honeysql [:sql clause-type]) driver clause)]
- (vary-meta hsql-form assoc :bigquery/temporal-type (temporal-type clause)))))
+ (with-temporal-type hsql-form (temporal-type clause)))))
+
+(defmethod sql.qp/->honeysql [:bigquery :relative-datetime]
+ [driver clause]
+ ;; wrap the parent method, converting the result if `clause` itself is typed
+ (let [t (temporal-type clause)]
+ (cond->> ((get-method sql.qp/->honeysql [:sql :relative-datetime]) driver clause)
+ t (->temporal-type t))))
(s/defn ^:private honeysql-form->sql :- s/Str
[driver, honeysql-form :- su/Map]
@@ -403,8 +447,7 @@
;;
;; TODO - we should make sure these are in the QP store somewhere and then could at least batch the calls
(let [table-name (db/select-one-field :name table/Table :id (u/get-id table-id))]
- (with-meta (hx/identifier :field table-name field-name)
- {:bigquery/temporal-type (temporal-type field)})))
+ (with-temporal-type (hx/identifier :field table-name field-name) (temporal-type field))))
(defmethod sql.qp/apply-top-level-clause [:bigquery :breakout]
[driver _ honeysql-form {breakouts :breakout, fields :fields}]
@@ -437,8 +480,9 @@
(if-let [target-type (or (temporal-type f) (some temporal-type args))]
(do
(log/tracef "Coercing args in %s to temporal type %s" (binding [*print-meta* true] (pr-str clause)) target-type)
- (u/prog1 (into [clause-type] (map (partial ->temporal-type target-type) (cons f args)))
- (when-not (= clause <>)
+ (u/prog1 (into [clause-type] (map (partial ->temporal-type target-type)
+ (cons f args)))
+ (when (not= [clause (meta clause)] [<> (meta <>)])
(log/tracef "Coerced -> %s" (binding [*print-meta* true] (pr-str <>))))))
clause))
@@ -454,18 +498,55 @@
;;; | Other Driver / SQLDriver Method Implementations |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defmethod driver/date-add :bigquery
- [driver expr amount unit]
- (let [add-fn (case (temporal-type expr)
- :timestamp :timestamp_add
- :datetime :datetime_add
- :date :date_add
- :time :time_add
- nil)]
- (if-not add-fn
- (driver/date-add driver (->temporal-type :datetime expr) amount unit)
- (with-meta (hsql/call add-fn expr (hsql/raw (format "INTERVAL %d %s" (int amount) (name unit))))
- {:bigquery/temporal-type (temporal-type expr)}))))
+(defn- interval [amount unit]
+ (hsql/raw (format "INTERVAL %d %s" (int amount) (name unit))))
+
+(defn- assert-addable-unit [t-type unit]
+ (when-not (contains? (temporal-type->supported-units t-type) unit)
+ ;; trying to add an `hour` to a `date` or a `year` to a `time` is something we shouldn't be allowing in the UI in
+ ;; the first place
+ (throw (ex-info (tru "Invalid query: you cannot add a {0} to a {1} column."
+ (name unit) (name t-type))
+ {:type error-type/invalid-query}))))
+
+;; We can coerce the HoneySQL form this wraps to whatever we want and generate the appropriate SQL.
+;; Thus for something like filtering against a relative datetime
+;;
+;; [:time-interval <datetime field> -1 :day]
+;;
+;;
+(defrecord ^:private AddIntervalForm [hsql-form amount unit]
+ hformat/ToSql
+ (to-sql [_]
+ (loop [hsql-form hsql-form]
+ (let [t (temporal-type hsql-form)
+ add-fn (case t
+ :timestamp :timestamp_add
+ :datetime :datetime_add
+ :date :date_add
+ :time :time_add
+ nil)]
+ (if-not add-fn
+ (recur (->temporal-type :datetime hsql-form))
+ (do
+ (assert-addable-unit t unit)
+ (hformat/to-sql (hsql/call add-fn hsql-form (interval amount unit)))))))))
+
+(defmethod temporal-type AddIntervalForm
+ [add-interval]
+ (temporal-type (:hsql-form add-interval)))
+
+(defmethod ->temporal-type [:temporal-type AddIntervalForm]
+ [target-type add-interval-form]
+ (let [current-type (temporal-type (:hsql-form add-interval-form))]
+ (when (#{[:date :time] [:time :date]} [current-type target-type])
+ (throw (ex-info (tru "It doesn''t make sense to convert between DATEs and TIMEs!")
+ {:type error-type/invalid-query}))))
+ (map->AddIntervalForm (update add-interval-form :hsql-form (partial ->temporal-type target-type))))
+
+(defmethod sql.qp/add-interval-honeysql-form :bigquery
+ [_ hsql-form amount unit]
+ (AddIntervalForm. hsql-form amount unit))
(defmethod driver/mbql->native :bigquery
[driver
@@ -484,9 +565,27 @@
sql.qp/source-query-alias))
:mbql? true})))
-(defmethod sql.qp/current-datetime-fn :bigquery
+(defrecord ^:private CurrentMomentForm [t]
+ hformat/ToSql
+ (to-sql [_]
+ (hformat/to-sql
+ (case (or t :timestamp)
+ :time :%current_time
+ :date :%current_date
+ :datetime :%current_datetime
+ :timestamp :%current_timestamp))))
+
+(defmethod temporal-type CurrentMomentForm
+ [^CurrentMomentForm current-moment]
+ (.t current-moment))
+
+(defmethod ->temporal-type [:temporal-type CurrentMomentForm]
+ [t _]
+ (CurrentMomentForm. t))
+
+(defmethod sql.qp/current-datetime-honeysql-form :bigquery
[_]
- (with-meta (hsql/call :current_timestamp) {:bigquery/temporal-type :timestamp}))
+ (CurrentMomentForm. nil))
(defmethod sql.qp/quote-style :bigquery
[_]
diff --git a/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj b/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj
index 0d4715288a7..d288029ee9d 100644
--- a/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj
+++ b/modules/drivers/bigquery/test/metabase/driver/bigquery/query_processor_test.clj
@@ -1,6 +1,10 @@
(ns metabase.driver.bigquery.query-processor-test
- (:require [clojure.test :refer :all]
- [honeysql.core :as hsql]
+ (:require [clojure
+ [string :as str]
+ [test :refer :all]]
+ [honeysql
+ [core :as hsql]
+ [format :as hformat]]
[java-time :as t]
[metabase
[driver :as driver]
@@ -305,6 +309,36 @@
(is (= [:= (hsql/call :extract :dayofweek expected-identifier) 1]
(sql.qp/->honeysql :bigquery [:= [:datetime-field [:field-id (:id field)] :day-of-week] 1]))))))))))
+(deftest reconcile-relative-datetimes-test
+ (testing "relative-datetime clauses on their own"
+ (doseq [[t [unit expected-sql]]
+ {:time [:hour "time_trunc(time_add(current_time(), INTERVAL -1 hour), hour)"]
+ :date [:year "date_trunc(date_add(current_date(), INTERVAL -1 year), year)"]
+ :datetime [:year "datetime_trunc(datetime_add(current_datetime(), INTERVAL -1 year), year)"]
+ ;; timestamp_add doesn't support `year` so this should cast a datetime instead
+ :timestamp [:year "CAST(datetime_trunc(datetime_add(current_datetime(), INTERVAL -1 year), year) AS timestamp)"]}]
+ (testing t
+ (let [reconciled-clause (#'bigquery.qp/->temporal-type t [:relative-datetime -1 unit])]
+ (is (= t
+ (#'bigquery.qp/temporal-type reconciled-clause))
+ "Should have correct type metadata after reconciliation")
+ (is (= [(str "WHERE " expected-sql)]
+ (sql.qp/format-honeysql :bigquery
+ {:where (sql.qp/->honeysql :bigquery reconciled-clause)}))
+ "Should get converted to the correct SQL")))))
+
+ (testing "relative-datetime clauses inside filter clauses"
+ (doseq [[expected-type t] {:date #t "2020-01-31"
+ :datetime #t "2020-01-31T20:43:00.000"
+ :timestamp #t "2020-01-31T20:43:00.000-08:00"}]
+ (testing expected-type
+ (let [[_ _ relative-datetime] (sql.qp/->honeysql :bigquery
+ [:=
+ t
+ [:relative-datetime -1 :year]])]
+ (is (= expected-type
+ (#'bigquery.qp/temporal-type relative-datetime))))))))
+
(deftest between-test
(testing "Make sure :between clauses reconcile the temporal types of their args"
(letfn [(between->sql [clause]
@@ -413,8 +447,8 @@
:filter filter-clause})))))))))))))
(deftest datetime-parameterized-sql-test
- (testing "Make sure Field filters against temporal fields generates correctly-typed SQL (#11578)"
- (mt/test-driver :bigquery
+ (mt/test-driver :bigquery
+ (testing "Make sure Field filters against temporal fields generates correctly-typed SQL (#11578)"
(mt/dataset attempted-murders
(doseq [field [:datetime
:date
@@ -440,3 +474,102 @@
:name "d"
:target [:dimension [:template-tag "d"]]
:value value}]})))))))))))
+
+(deftest current-datetime-honeysql-form-test
+ (testing (str "The object returned by `current-datetime-honeysql-form` should be a magic object that can take on "
+ "whatever temporal type we want.")
+ (let [form (sql.qp/current-datetime-honeysql-form :bigquery)]
+ (is (= nil
+ (#'bigquery.qp/temporal-type form))
+ "When created the temporal type should be unspecified. The world's your oyster!")
+ (is (= ["current_timestamp()"]
+ (hformat/format form))
+ "Should fall back to acting like a timestamp if we don't coerce it to something else first")
+ (doseq [[temporal-type expected-sql] {:date "current_date()"
+ :time "current_time()"
+ :datetime "current_datetime()"
+ :timestamp "current_timestamp()"}]
+ (testing (format "temporal type = %s" temporal-type)
+ (is (= temporal-type
+ (#'bigquery.qp/temporal-type (#'bigquery.qp/->temporal-type temporal-type form)))
+ "Should be possible to convert to another temporal type/should report its type correctly")
+ (is (= [expected-sql]
+ (hformat/format (#'bigquery.qp/->temporal-type temporal-type form)))
+ "Should convert to the correct SQL"))))))
+
+(deftest add-interval-honeysql-form-test
+ ;; this doesn't test conversion to/from time because there's no unit we can use that works for all for. So we'll
+ ;; just test the 3 that support `:day` and that should be proof the logic is working. (The code that actually uses
+ ;; this is tested e2e by `filter-by-relative-date-ranges-test` anyway.)
+ (doseq [initial-type [:date :datetime :timestamp]
+ :let [form (sql.qp/add-interval-honeysql-form
+ :bigquery
+ (#'bigquery.qp/->temporal-type
+ initial-type
+ (sql.qp/current-datetime-honeysql-form :bigquery))
+ -1
+ :day)]]
+ (testing (format "initial form = %s" (pr-str form))
+ (is (= initial-type
+ (#'bigquery.qp/temporal-type form))
+ "Should have the temporal-type of the form it wraps when created.")
+ (doseq [[new-type expected-sql] {:date "date_add(current_date(), INTERVAL -1 day)"
+ :datetime "datetime_add(current_datetime(), INTERVAL -1 day)"
+ :timestamp "timestamp_add(current_timestamp(), INTERVAL -1 day)"}]
+ (testing (format "\nconvert from %s -> %s" initial-type new-type)
+ (is (= new-type
+ (#'bigquery.qp/temporal-type (#'bigquery.qp/->temporal-type new-type form)))
+ "Should be possible to convert to another temporal type/should report its type correctly")
+ (is (= [expected-sql]
+ (hformat/format (#'bigquery.qp/->temporal-type new-type form)))
+ "Should convert to the correct SQL"))))))
+
+(defn- can-we-filter-against-relative-datetime? [field unit]
+ (let [{:keys [error]} (mt/run-mbql-query attempts
+ {:aggregation [[:count]]
+ :filter [:time-interval (mt/id :attempts field) :last unit]})]
+ (not error)))
+
+(deftest filter-by-relative-date-ranges-test
+ (testing "Make sure the SQL we generate for filters against relative-datetimes is typed correctly"
+ (mt/with-everything-store
+ (binding [sql.qp/*table-alias* "ABC"]
+ (doseq [[field-type [unit expected-sql]]
+ {:type/Time [:hour (str "WHERE time_trunc(ABC.time, hour)"
+ " = time_trunc(time_add(current_time(), INTERVAL -1 hour), hour)")]
+ :type/Date [:year (str "WHERE date_trunc(ABC.date, year)"
+ " = date_trunc(date_add(current_date(), INTERVAL -1 year), year)")]
+ :type/DateTime [:year (str "WHERE datetime_trunc(ABC.datetime, year)"
+ " = datetime_trunc(datetime_add(current_datetime(), INTERVAL -1 year), year)")]
+ ;; `timestamp_add` doesn't support `year` so it should cast a `datetime_trunc` instead
+ :type/DateTimeWithLocalTZ [:year (str "WHERE timestamp_trunc(ABC.datetimewithlocaltz, year)"
+ " = CAST(datetime_trunc(datetime_add(current_datetime(), INTERVAL -1 year), year) AS timestamp)")]}]
+ (mt/with-temp Field [f {:name (str/lower-case (name field-type)), :base_type field-type}]
+ (testing (format "%s field" field-type)
+ (is (= [expected-sql]
+ (hsql/format {:where (sql.qp/->honeysql :bigquery [:=
+ [:datetime-field [:field-id (:id f)] unit]
+ [:relative-datetime -1 unit]])}))))))))))
+
+(deftest filter-by-relative-date-ranges-e2e-test
+ (mt/test-driver :bigquery
+ (testing (str "Make sure filtering against relative date ranges works correctly regardless of underlying column "
+ "type (#11725)")
+ (mt/dataset attempted-murders
+ (is (= [[nil :minute :hour :day :week :month :quarter :year]
+ [:time true true false false false false false]
+ [:datetime true true true true true true true]
+ [:date false false true true true true true]
+ [:datetime_tz true true true true true true true]]
+ (let [units [:minute :hour :day :week :month :quarter :year]
+ fields [:time :datetime :date :datetime_tz]]
+
+ (into
+ [(into [nil] units)]
+ (pmap
+ (fn [field]
+ (into [field] (pmap
+ (fn [unit]
+ (boolean (can-we-filter-against-relative-datetime? field unit)))
+ units)))
+ fields)))))))))
diff --git a/modules/drivers/google/src/metabase/driver/google.clj b/modules/drivers/google/src/metabase/driver/google.clj
index 176cc78f2e4..8ca1bebe8d7 100644
--- a/modules/drivers/google/src/metabase/driver/google.clj
+++ b/modules/drivers/google/src/metabase/driver/google.clj
@@ -6,10 +6,10 @@
[driver :as driver]
[util :as u]]
[metabase.models.database :refer [Database]]
+ [metabase.query-processor.error-type :as error-type]
[ring.util.codec :as codec]
[toucan.db :as db])
- (:import [com.google.api.client.googleapis.auth.oauth2 GoogleAuthorizationCodeFlow GoogleAuthorizationCodeFlow$Builder
- GoogleCredential GoogleCredential$Builder GoogleTokenResponse]
+ (:import [com.google.api.client.googleapis.auth.oauth2 GoogleAuthorizationCodeFlow GoogleAuthorizationCodeFlow$Builder GoogleCredential GoogleCredential$Builder GoogleTokenResponse]
com.google.api.client.googleapis.javanet.GoogleNetHttpTransport
[com.google.api.client.googleapis.json GoogleJsonError GoogleJsonResponseException]
com.google.api.client.googleapis.services.AbstractGoogleClientRequest
@@ -30,7 +30,7 @@
(def ^:private ^:const ^String redirect-uri "urn:ietf:wg:oauth:2.0:oob")
(defn execute-no-auto-retry
- "`execute` REQUEST, and catch any `GoogleJsonResponseException` is throws, converting them to `ExceptionInfo` and
+ "Execute `request`, and catch any `GoogleJsonResponseException` is throws, converting them to `ExceptionInfo` and
rethrowing them."
[^AbstractGoogleClientRequest request]
(try (.execute request)
@@ -44,10 +44,13 @@
"Execute `request`, and catch any `GoogleJsonResponseException` is throws, converting them to `ExceptionInfo` and
rethrowing them.
- This automatically retries any failed requests up to 2 times."
+ This automatically retries any failed requests."
[^AbstractGoogleClientRequest request]
- (u/auto-retry 2
- (execute-no-auto-retry request)))
+ (try
+ (execute-no-auto-retry request)
+ (catch Throwable e
+ (when-not (error-type/client-error? (:type (u/all-ex-data e)))
+ (execute-no-auto-retry request)))))
(defn- create-application-name
"Creates the application name string, separated out from the `def` below so it's testable with different values"
diff --git a/modules/drivers/oracle/src/metabase/driver/oracle.clj b/modules/drivers/oracle/src/metabase/driver/oracle.clj
index 27f4be986e5..bfb0b35547b 100644
--- a/modules/drivers/oracle/src/metabase/driver/oracle.clj
+++ b/modules/drivers/oracle/src/metabase/driver/oracle.clj
@@ -134,16 +134,18 @@
(defn- num-to-ym-interval [unit v] (hsql/call :numtoyminterval v (hx/literal unit)))
(defmethod driver/date-add :oracle
- [_ dt amount unit]
- (hx/+ (hx/->timestamp dt) (case unit
- :second (num-to-ds-interval :second amount)
- :minute (num-to-ds-interval :minute amount)
- :hour (num-to-ds-interval :hour amount)
- :day (num-to-ds-interval :day amount)
- :week (num-to-ds-interval :day (hx/* amount (hsql/raw 7)))
- :month (num-to-ym-interval :month amount)
- :quarter (num-to-ym-interval :month (hx/* amount (hsql/raw 3)))
- :year (num-to-ym-interval :year amount))))
+ [_ hsql-form amount unit]
+ (hx/+
+ (hx/->timestamp hsql-form)
+ (case unit
+ :second (num-to-ds-interval :second amount)
+ :minute (num-to-ds-interval :minute amount)
+ :hour (num-to-ds-interval :hour amount)
+ :day (num-to-ds-interval :day amount)
+ :week (num-to-ds-interval :day (hx/* amount (hsql/raw 7)))
+ :month (num-to-ym-interval :month amount)
+ :quarter (num-to-ym-interval :month (hx/* amount (hsql/raw 3)))
+ :year (num-to-ym-interval :year amount))))
(defmethod sql.qp/unix-timestamp->timestamp [:oracle :seconds]
[_ _ field-or-value]
diff --git a/modules/drivers/presto/src/metabase/driver/presto.clj b/modules/drivers/presto/src/metabase/driver/presto.clj
index 9a2cf94f60e..9cf915499c6 100644
--- a/modules/drivers/presto/src/metabase/driver/presto.clj
+++ b/modules/drivers/presto/src/metabase/driver/presto.clj
@@ -140,8 +140,8 @@
(= v "information_schema")))
(defmethod driver/date-add :presto
- [_ dt amount unit]
- (hsql/call :date_add (hx/literal unit) amount dt))
+ [_ hsql-form amount unit]
+ (hsql/call :date_add (hx/literal unit) amount hsql-form))
(s/defn ^:private database->all-schemas :- #{su/NonBlankString}
"Return a set of all schema names in this `database`."
diff --git a/modules/drivers/redshift/src/metabase/driver/redshift.clj b/modules/drivers/redshift/src/metabase/driver/redshift.clj
index 7e40a31b610..44d976f1e26 100644
--- a/modules/drivers/redshift/src/metabase/driver/redshift.clj
+++ b/modules/drivers/redshift/src/metabase/driver/redshift.clj
@@ -82,8 +82,8 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defmethod driver/date-add :redshift
- [_ dt amount unit]
- (hsql/call :dateadd (hx/literal unit) amount (hx/->timestamp dt)))
+ [_ hsql-form amount unit]
+ (hsql/call :dateadd (hx/literal unit) amount (hx/->timestamp hsql-form)))
(defmethod sql.qp/unix-timestamp->timestamp [:redshift :seconds]
[_ _ expr]
diff --git a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
index 5bf66119366..756755625da 100644
--- a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
+++ b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
@@ -106,11 +106,11 @@
:%current_timestamp)
(defmethod driver/date-add :snowflake
- [_ dt amount unit]
+ [_ hsql-form amount unit]
(hsql/call :dateadd
(hsql/raw (name unit))
(hsql/raw (int amount))
- (hx/->timestamp dt)))
+ (hx/->timestamp hsql-form)))
(defn- extract [unit expr] (hsql/call :date_part unit (hx/->timestamp expr)))
(defn- date-trunc [unit expr] (hsql/call :date_trunc unit (hx/->timestamp expr)))
diff --git a/modules/drivers/sparksql/src/metabase/driver/hive_like.clj b/modules/drivers/sparksql/src/metabase/driver/hive_like.clj
index 6b679330ab8..fd05e378fe8 100644
--- a/modules/drivers/sparksql/src/metabase/driver/hive_like.clj
+++ b/modules/drivers/sparksql/src/metabase/driver/hive_like.clj
@@ -113,8 +113,8 @@
3)))
(defmethod driver/date-add :hive-like
- [_ dt amount unit]
- (hx/+ (hx/->timestamp dt) (hsql/raw (format "(INTERVAL '%d' %s)" (int amount) (name unit)))))
+ [_ hsql-form amount unit]
+ (hx/+ (hx/->timestamp hsql-form) (hsql/raw (format "(INTERVAL '%d' %s)" (int amount) (name unit)))))
;; ignore the schema when producing the identifier
(defn qualified-name-components
diff --git a/modules/drivers/sqlite/src/metabase/driver/sqlite.clj b/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
index c4ce0e037f8..4a0d941c054 100644
--- a/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
+++ b/modules/drivers/sqlite/src/metabase/driver/sqlite.clj
@@ -162,7 +162,7 @@
(->date (sql.qp/->honeysql driver expr) (hx/literal "start of year")))
(defmethod driver/date-add :sqlite
- [driver dt amount unit]
+ [driver hsql-form amount unit]
(let [[multiplier sqlite-unit] (case unit
:second [1 "seconds"]
:minute [1 "minutes"]
@@ -184,7 +184,7 @@
;; The SQL we produce instead (for "last month") ends up looking something like:
;; DATE(DATETIME(DATE('2015-03-30', 'start of month'), '-1 month'), 'start of month').
;; It's a little verbose, but gives us the correct answer (Feb 1st).
- (->datetime (sql.qp/date driver unit dt)
+ (->datetime (sql.qp/date driver unit hsql-form)
(hx/literal (format "%+d %s" (* amount multiplier) sqlite-unit)))))
(defmethod sql.qp/unix-timestamp->timestamp [:sqlite :seconds]
diff --git a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
index a5e8e65b875..82919180d29 100644
--- a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
+++ b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
@@ -180,8 +180,8 @@
(hsql/call :datefromparts (hx/year expr) 1 1))
(defmethod driver/date-add :sqlserver
- [_ dt amount unit]
- (date-add unit amount dt))
+ [_ hsql-form amount unit]
+ (date-add unit amount hsql-form))
(defmethod sql.qp/unix-timestamp->timestamp [:sqlserver :seconds]
[_ _ expr]
diff --git a/modules/drivers/vertica/src/metabase/driver/vertica.clj b/modules/drivers/vertica/src/metabase/driver/vertica.clj
index fbec1bde969..79d70d25a38 100644
--- a/modules/drivers/vertica/src/metabase/driver/vertica.clj
+++ b/modules/drivers/vertica/src/metabase/driver/vertica.clj
@@ -96,8 +96,8 @@
one-day))
(defmethod driver/date-add :vertica
- [_ dt amount unit]
- (hx/+ (hx/->timestamp dt) (hsql/raw (format "(INTERVAL '%d %s')" (int amount) (name unit)))))
+ [_ hsql-form amount unit]
+ (hx/+ (hx/->timestamp hsql-form) (hsql/raw (format "(INTERVAL '%d %s')" (int amount) (name unit)))))
(defn- materialized-views
"Fetch the Materialized Views for a Vertica `database`.
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 7a7933e944c..06aa491f06a 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -230,12 +230,13 @@
dispatch-on-initialized-driver
:hierarchy #'hierarchy)
+(defmulti ^{:deprecated "0.34.2"} date-add
+ "DEPRECATED -- this method is only used or implemented by `:sql` drivers. It has been superseded by
+ `metabase.driver.sql.query-processor/add-interval-honeysql-form`. Use/implement that method instead. DO NOT use or
+ implement this method for non-`:sql` drivers.
-;; TODO - this is only used (or implemented for that matter) by SQL drivers. This should probably be moved into the
-;; `:sql` driver. Don't bother to implement this for non-SQL drivers.
-(defmulti date-add
- "Return an driver-appropriate representation of a moment relative to the given time."
- {:arglists '([driver dt amount unit])}
+ This method will be removed at some point in the future."
+ {:arglists '([driver hsql-form amount unit])}
dispatch-on-initialized-driver
:hierarchy #'hierarchy)
diff --git a/src/metabase/driver/h2.clj b/src/metabase/driver/h2.clj
index b08da9b2fb1..025e0fdfa6f 100644
--- a/src/metabase/driver/h2.clj
+++ b/src/metabase/driver/h2.clj
@@ -70,10 +70,10 @@
(defmethod driver/process-query-in-context :h2 [_ qp]
(comp qp check-native-query-not-using-default-user))
-(defmethod driver/date-add :h2 [driver dt amount unit]
+(defmethod driver/date-add :h2 [driver hsql-form amount unit]
(if (= unit :quarter)
- (recur driver dt (hx/* amount 3) :month)
- (hsql/call :dateadd (hx/literal unit) amount dt)))
+ (recur driver hsql-form (hx/* amount 3) :month)
+ (hsql/call :dateadd (hx/literal unit) amount hsql-form)))
(defmethod driver/humanize-connection-error-message :h2 [_ message]
(condp re-matches message
diff --git a/src/metabase/driver/mysql.clj b/src/metabase/driver/mysql.clj
index cc0fcd6e663..bcd14409f14 100644
--- a/src/metabase/driver/mysql.clj
+++ b/src/metabase/driver/mysql.clj
@@ -72,8 +72,8 @@
:placeholder "tinyInt1isBit=false")]))
(defmethod driver/date-add :mysql
- [_ dt amount unit]
- (hsql/call :date_add dt (hsql/raw (format "INTERVAL %d %s" (int amount) (name unit)))))
+ [_ hsql-form amount unit]
+ (hsql/call :date_add hsql-form (hsql/raw (format "INTERVAL %d %s" (int amount) (name unit)))))
(defmethod driver/humanize-connection-error-message :mysql
[_ message]
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index 0727f7b5416..482d090fc35 100644
--- a/src/metabase/driver/postgres.clj
+++ b/src/metabase/driver/postgres.clj
@@ -41,8 +41,8 @@
(defmethod driver/display-name :postgres [_] "PostgreSQL")
(defmethod driver/date-add :postgres
- [_ dt amount unit]
- (hx/+ (hx/->timestamp dt)
+ [_ hsql-form amount unit]
+ (hx/+ (hx/->timestamp hsql-form)
(hsql/raw (format "(INTERVAL '%d %s')" (int amount) (name unit)))))
(defmethod driver/humanize-connection-error-message :postgres
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index 4d320f4805c..43780aa777b 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -60,14 +60,28 @@
;;; | Interface (Multimethods) |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defmulti current-datetime-fn
- "HoneySQL form that should be used to get the current `datetime` (or equivalent). Defaults to `:%now`."
+(defmulti ^{:deprecated "0.34.2"} current-datetime-fn
+ "HoneySQL form that should be used to get the current `datetime` (or equivalent). Defaults to `:%now`.
+
+ DEPRECATED: `current-datetime-fn` is a misnomer, since the result can actually be any valid HoneySQL form.
+ `current-datetime-honeysql-form` replaces this method; implement and call that method instead. This method will be
+ removed in favor of `current-datetime-honeysql-form` at some point in the future."
{:arglists '([driver])}
driver/dispatch-on-initialized-driver
:hierarchy #'driver/hierarchy)
(defmethod current-datetime-fn :sql [_] :%now)
+(defmulti current-datetime-honeysql-form
+ "HoneySQL form that should be used to get the current `datetime` (or equivalent). Defaults to `:%now`."
+ {:arglists '([driver])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmethod current-datetime-honeysql-form :sql
+ [driver]
+ (current-datetime-fn driver))
+
;; TODO - rename this to `date-bucket` or something that better describes what it actually does
(defmulti date
"Return a HoneySQL form for truncating a date or timestamp field or value to a given resolution, or extracting a date
@@ -80,6 +94,17 @@
(defmethod date [:sql :default] [_ _ expr] expr)
+(defmulti add-interval-honeysql-form
+ "Return a HoneySQL form that performs represents addition of some temporal interval to the original `hsql-form`.
+
+ (add-interval-honeysql-form :my-driver hsql-form 1 :day) -> (hsql/call :date_add hsql-form 1 (hx/literal 'day'))"
+ {:arglists '([driver hsql-form amount unit])}
+ driver/dispatch-on-initialized-driver
+ :hierarchy #'driver/hierarchy)
+
+(defmethod add-interval-honeysql-form :sql [driver hsql-form amount unit]
+ (driver/date-add driver hsql-form amount unit))
+
(defmulti field->identifier
"Return a HoneySQL form that should be used as the identifier for `field`, an instance of the Field model. The default
implementation returns a keyword generated by from the components returned by `field/qualified-name-components`.
@@ -266,8 +291,8 @@
(defmethod ->honeysql [:sql :+] [driver [_ & args]]
(if (mbql.u/datetime-arithmetics? args)
(let [[field & intervals] args]
- (reduce (fn [result [_ amount unit]]
- (driver/date-add driver result amount unit))
+ (reduce (fn [hsql-form [_ amount unit]]
+ (add-interval-honeysql-form driver hsql-form amount unit))
(->honeysql driver field)
intervals))
(apply hsql/call :+ (map (partial ->honeysql driver) args))))
@@ -343,8 +368,8 @@
(defmethod ->honeysql [:sql :relative-datetime]
[driver [_ amount unit]]
(date driver unit (if (zero? amount)
- (current-datetime-fn driver)
- (driver/date-add driver (current-datetime-fn driver) amount unit))))
+ (current-datetime-honeysql-form driver)
+ (add-interval-honeysql-form driver (current-datetime-honeysql-form driver) amount unit))))
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/src/metabase/util/date_2.clj b/src/metabase/util/date_2.clj
index 9eca9c6e161..66122314cad 100644
--- a/src/metabase/util/date_2.clj
+++ b/src/metabase/util/date_2.clj
@@ -115,6 +115,8 @@
:iso-week-of-year
:month-of-year
:quarter-of-year
+ ;; TODO - in this namespace `:year` is something you can both extract and truncate to. In MBQL `:year` is a truncation
+ ;; operation. Maybe we should rename this unit to clear up the potential confusion (?)
:year})
(def ^:private week-fields*
--
GitLab