diff --git a/modules/drivers/bigquery/src/metabase/driver/bigquery.clj b/modules/drivers/bigquery/src/metabase/driver/bigquery.clj index 914173636f0ce5a10de37c576ef923de284bb366..2adaa81e7900a34d05bb038395160592238cad49 100644 --- a/modules/drivers/bigquery/src/metabase/driver/bigquery.clj +++ b/modules/drivers/bigquery/src/metabase/driver/bigquery.clj @@ -406,8 +406,9 @@ ;;; | Other Driver / SQLDriver Method Implementations | ;;; +----------------------------------------------------------------------------------------------------------------+ -(defmethod driver/date-interval :bigquery [driver unit amount] - (sql.qp/->honeysql driver (du/relative-date unit amount))) +(defmethod driver/date-add :bigquery + [_ dt amount unit] + (hsql/call :datetime_add (hx/->datetime dt) (hsql/raw (format "INTERVAL %d %s" (int amount) (name unit))))) (defmethod driver/mbql->native :bigquery [driver diff --git a/modules/drivers/oracle/src/metabase/driver/oracle.clj b/modules/drivers/oracle/src/metabase/driver/oracle.clj index dbd69386b13d7eebfbed8347f3847874c8cc3a68..48496ad6f02e23cfcaf3945e1d25dc9ef7a962f4 100644 --- a/modules/drivers/oracle/src/metabase/driver/oracle.clj +++ b/modules/drivers/oracle/src/metabase/driver/oracle.clj @@ -134,18 +134,17 @@ (defn- num-to-ds-interval [unit v] (hsql/call :numtodsinterval v (hx/literal unit))) (defn- num-to-ym-interval [unit v] (hsql/call :numtoyminterval v (hx/literal unit))) -;; e.g. (SYSDATE + NUMTODSINTERVAL(?, 'second')) -(defmethod driver/date-interval :oracle [_ unit amount] - (hx/+ now (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 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)))) (defmethod sql.qp/unix-timestamp->timestamp [:oracle :seconds] [_ _ field-or-value] (hx/+ date-1970-01-01 (num-to-ds-interval :second field-or-value))) diff --git a/modules/drivers/presto/src/metabase/driver/presto.clj b/modules/drivers/presto/src/metabase/driver/presto.clj index 40e3db611a9a4db301649721ec316fccecb8305e..dea01d759bbc1a8e7eac2759bf64dbb4ece5a040 100644 --- a/modules/drivers/presto/src/metabase/driver/presto.clj +++ b/modules/drivers/presto/src/metabase/driver/presto.clj @@ -166,9 +166,9 @@ (format "SHOW SCHEMAS FROM %s LIKE 'information_schema'" (sql.u/quote-name driver :database catalog)))] (= v "information_schema"))) -(defmethod driver/date-interval :presto - [_ unit amount] - (hsql/call :date_add (hx/literal unit) amount :%now)) +(defmethod driver/date-add :presto + [_ dt amount unit] + (hsql/call :date_add (hx/literal unit) amount dt)) (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 13c0f90fd8ca80b5efd7962d96651f6719ca6900..333f32ab04337f5ce404abf795c78d350465ab11 100644 --- a/modules/drivers/redshift/src/metabase/driver/redshift.clj +++ b/modules/drivers/redshift/src/metabase/driver/redshift.clj @@ -72,8 +72,8 @@ ;;; | metabase.driver.sql impls | ;;; +----------------------------------------------------------------------------------------------------------------+ -(defmethod driver/date-interval :redshift [_ unit amount] - (hsql/call :+ :%getdate (hsql/raw (format "INTERVAL '%d %s'" (int amount) (name unit))))) +(defmethod driver/date-add :redshift [_ dt amount unit] + (hsql/call :dateadd (hx/literal unit) amount (hx/->timestamp dt))) (defmethod sql.qp/unix-timestamp->timestamp [:redshift :seconds] [_ _ expr] (hx/+ (hsql/raw "TIMESTAMP '1970-01-01T00:00:00Z'") diff --git a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj index 2cbf23a9c59b501c4b4ed6e0c42e78080ac3eb79..3735632c876657cbbc2f5632e433730ae11219d0 100644 --- a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj +++ b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj @@ -97,11 +97,14 @@ (defmethod sql.qp/unix-timestamp->timestamp [:snowflake :seconds] [_ _ expr] (hsql/call :to_timestamp expr)) (defmethod sql.qp/unix-timestamp->timestamp [:snowflake :milliseconds] [_ _ expr] (hsql/call :to_timestamp expr 3)) -(defmethod driver/date-interval :snowflake [_ unit amount] +(defmethod sql.qp/current-datetime-fn :snowflake [_] + :%current_timestamp) + +(defmethod driver/date-add :snowflake [_ dt amount unit] (hsql/call :dateadd (hsql/raw (name unit)) (hsql/raw (int amount)) - :%current_timestamp)) + (hx/->timestamp dt))) (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 198457de93082559155a6d9916f51d329e5d982a..41dc40f7a23162eb7c5e0f0bc6b12f9551e2fa8b 100644 --- a/modules/drivers/sparksql/src/metabase/driver/hive_like.clj +++ b/modules/drivers/sparksql/src/metabase/driver/hive_like.clj @@ -94,8 +94,8 @@ 1) 3))) -(defmethod driver/date-interval :hive-like [_ unit amount] - (hsql/raw (format "(NOW() + INTERVAL '%d' %s)" (int amount) (name unit)))) +(defmethod driver/date-add :hive-like [_ dt amount unit] + (hx/+ (hx/->timestamp dt) (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 94f15164bc4018fc22c4404ac5e2380bc5ba1fab..c994a4cf1f6c510bc245c588a7897be4745f4f4e 100644 --- a/modules/drivers/sqlite/src/metabase/driver/sqlite.clj +++ b/modules/drivers/sqlite/src/metabase/driver/sqlite.clj @@ -118,7 +118,7 @@ (defmethod sql.qp/date [:sqlite :year] [_ _ expr] (hx/->integer (strftime "%Y" (ts->str expr)))) -(defmethod driver/date-interval :sqlite [driver unit amount] +(defmethod driver/date-add :sqlite [driver dt amount unit] (let [[multiplier sqlite-unit] (case unit :second [1 "seconds"] :minute [1 "minutes"] @@ -140,7 +140,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 (hx/literal "now")) + (->datetime (sql.qp/date driver unit dt) (hx/literal (format "%+d %s" (* amount multiplier) sqlite-unit))))) (defmethod sql.qp/unix-timestamp->timestamp [:sqlite :seconds] [_ _ expr] diff --git a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj index aa2e643e4376a8af272367756a121f0702d8e046..e92e00b4cd1f1536762354f1693d2eb2f45390e6 100644 --- a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj +++ b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj @@ -164,8 +164,8 @@ (date-part :year expr)) -(defmethod driver/date-interval :sqlserver [_ unit amount] - (date-add unit amount :%getdate)) +(defmethod driver/date-add :sqlserver [_ dt amount unit] + (date-add unit amount dt)) (defmethod sql.qp/unix-timestamp->timestamp [:sqlserver :seconds] [_ _ expr] ;; The second argument to DATEADD() gets casted to a 32-bit integer. BIGINT is 64 bites, so we tend to run into diff --git a/modules/drivers/vertica/src/metabase/driver/vertica.clj b/modules/drivers/vertica/src/metabase/driver/vertica.clj index e706d3aa83f2b50c7388a762b0875a1938d585cd..9df6bde952cb2c54147821f69e66e1c0348d9352 100644 --- a/modules/drivers/vertica/src/metabase/driver/vertica.clj +++ b/modules/drivers/vertica/src/metabase/driver/vertica.clj @@ -91,8 +91,8 @@ one-day)) one-day)) -(defmethod driver/date-interval :vertica [_ unit amount] - (hsql/raw (format "(NOW() + INTERVAL '%d %s')" (int amount) (name unit)))) +(defmethod driver/date-add :vertica [_ dt amount unit] + (hx/+ (hx/->timestamp dt) (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 59d2b329f95b3725e196907cd596ce433ad7312a..a53dd8679f93ce5ce9cc047d331bf01e32942b68 100644 --- a/src/metabase/driver.clj +++ b/src/metabase/driver.clj @@ -368,20 +368,12 @@ ;; 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-interval - "Return an driver-appropriate representation of a moment relative to the current moment in time. By default, this - returns an `Timestamp` by calling `metabase.util.date/relative-date`; but when possible drivers should return a - native form so we can be sure the correct timezone is applied. For example, SQL drivers should return a HoneySQL - form to call the appropriate SQL fns: - - (date-interval :postgres :month 1) -> (hsql/call :+ :%now (hsql/raw \"INTERVAL '1 month'\"))" - {:arglists '([driver unit amount])} +(defmulti date-add + "Return an driver-appropriate representation of a moment relative to the given time." + {:arglists '([driver dt amount unit])} dispatch-on-initialized-driver :hierarchy #'hierarchy) -(defmethod date-interval ::driver [_ unit amount] - (du/relative-date unit amount)) - (defmulti describe-database "Return a map containing information that describes all of the tables in a `database`, an instance of the `Database` diff --git a/src/metabase/driver/h2.clj b/src/metabase/driver/h2.clj index 526bc7da8425bcfbe982bbe3a2dba7506369533c..ba2be86dc000ce0b6ada0d5861fec6b3fb61c352 100644 --- a/src/metabase/driver/h2.clj +++ b/src/metabase/driver/h2.clj @@ -70,10 +70,10 @@ (comp qp check-native-query-not-using-default-user)) -(defmethod driver/date-interval :h2 [driver unit amount] +(defmethod driver/date-add :h2 [driver dt amount unit] (if (= unit :quarter) - (recur driver :month (hx/* amount 3)) - (hsql/call :dateadd (hx/literal unit) amount :%now))) + (recur driver dt (hx/* amount 3) :month) + (hsql/call :dateadd (hx/literal unit) amount dt))) (defmethod driver/humanize-connection-error-message :h2 [_ message] diff --git a/src/metabase/driver/mysql.clj b/src/metabase/driver/mysql.clj index 9f9af5747f1616444a1bc8bd39057e5cc9f02b53..9a00d8b8bcb57e1fc8eafbb875027c3ea4e405b4 100644 --- a/src/metabase/driver/mysql.clj +++ b/src/metabase/driver/mysql.clj @@ -54,10 +54,8 @@ :placeholder "tinyInt1isBit=false")])) -(defmethod driver/date-interval :mysql [_ unit amount] - (hsql/call :date_add - :%now - (hsql/raw (format "INTERVAL %d %s" (int amount) (name unit))))) +(defmethod driver/date-add :mysql [_ dt amount unit] + (hsql/call :date_add dt (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 3e467bd6fbff864795e6fa709ff954950bfa2ed3..821a498c60c1255260e792481502a443055938be 100644 --- a/src/metabase/driver/postgres.clj +++ b/src/metabase/driver/postgres.clj @@ -32,8 +32,9 @@ (defmethod driver/display-name :postgres [_] "PostgreSQL") -(defmethod driver/date-interval :postgres [_ unit amount] - (hsql/raw (format "(NOW() + INTERVAL '%d %s')" (int amount) (name unit)))) +(defmethod driver/date-add :postgres [_ dt amount unit] + (hx/+ (hx/->timestamp dt) + (hsql/raw (format "(INTERVAL '%d %s')" (int amount) (name unit))))) (defmethod driver/humanize-connection-error-message :postgres [_ message] (condp re-matches message diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj index a199357e753d96d7eba4362f4712cc7571cd1020..12afab53945d873ef093ee20bb6c9eb880a77ba7 100644 --- a/src/metabase/driver/sql/query_processor.clj +++ b/src/metabase/driver/sql/query_processor.clj @@ -230,7 +230,15 @@ (defmethod ->honeysql [:sql :min] [driver [_ field]] (hsql/call :min (->honeysql driver field))) (defmethod ->honeysql [:sql :max] [driver [_ field]] (hsql/call :max (->honeysql driver field))) -(defmethod ->honeysql [:sql :+] [driver [_ & args]] (apply hsql/call :+ (map (partial ->honeysql driver) args))) +(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)) + (->honeysql driver field) + intervals)) + (apply hsql/call :+ (map (partial ->honeysql driver) args)))) + (defmethod ->honeysql [:sql :-] [driver [_ & args]] (apply hsql/call :- (map (partial ->honeysql driver) args))) (defmethod ->honeysql [:sql :*] [driver [_ & args]] (apply hsql/call :* (map (partial ->honeysql driver) args))) @@ -300,7 +308,7 @@ [driver [_ amount unit]] (date driver unit (if (zero? amount) (current-datetime-fn driver) - (driver/date-interval driver unit amount)))) + (driver/date-add driver (current-datetime-fn driver) amount unit)))) ;;; +----------------------------------------------------------------------------------------------------------------+ diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj index 161f2c033ebba73e1c8dd7a12ae2917c45888df9..36f621f6710e2491e9420007c546cf5bbb3ae26f 100644 --- a/src/metabase/query_processor/middleware/annotate.clj +++ b/src/metabase/query_processor/middleware/annotate.clj @@ -106,8 +106,16 @@ join-alias)] (format "%s → %s" join-display-name field-display-name))) +(defn- infer-expression-type + [expression] + (if (mbql.u/datetime-arithmetics? expression) + {:base_type :type/DateTime + :special_type nil} + {:base_type :type/Float + :special_type :type/Number})) + (s/defn ^:private col-info-for-field-clause :- su/Map - [{:keys [source-metadata], :as inner-query} :- su/Map, clause :- mbql.s/Field] + [{:keys [source-metadata expressions], :as inner-query} :- su/Map, clause :- mbql.s/Field] ;; for various things that can wrap Field clauses recurse on the wrapped Field but include a little bit of info ;; about the clause doing the wrapping (mbql.u/match-one clause @@ -143,12 +151,14 @@ :display_name (humanization/name->human-readable-name field-name)}) [:expression expression-name] - {:name expression-name - :display_name expression-name - :base_type :type/Float - :special_type :type/Number - ;; provided so the FE can add easily add sorts and the like when someone clicks a column header - :expression_name expression-name} + (merge + ;; There's some inconsistency when expression names are keywords and when strings. + ;; TODO: remove this duality once https://github.com/metabase/mbql/issues/5 is resolved. + (infer-expression-type (some expressions ((juxt identity keyword) expression-name))) + {:name expression-name + :display_name expression-name + ;; provided so the FE can add easily add sorts and the like when someone clicks a column header + :expression_name expression-name}) [:field-id id] (let [{parent-id :parent_id, :as field} (dissoc (qp.store/field id) :database_type)] @@ -330,8 +340,7 @@ ;; for the purposes of a patch release. [(_ :guard #{:expression :+ :- :/ :*}) & _] (merge - {:base_type :type/Float - :special_type :type/Number} + (infer-expression-type &match) (when (mbql.preds/Aggregation? &match) (ag->name-info &match))) diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj index 9f95ddcac90e83cc1af594409ffc4edb63450a67..6cc37387195bb888d3c7a26940a431a4c73526ee 100644 --- a/test/metabase/query_processor/middleware/annotate_test.clj +++ b/test/metabase/query_processor/middleware/annotate_test.clj @@ -423,12 +423,29 @@ (-> (qp.test-util/with-everything-store ((annotate/add-column-info (constantly {})) (data/mbql-query venues - {:expressions {"discount_price" [:* 0.9 [:field-id $price]]} + {:expressions {:discount_price [:* 0.9 [:field-id $price]]} :fields [$name [:expression "discount_price"]] :limit 10}))) :cols second)) +(expect + {:name "prev_month" + :display_name "prev_month" + :base_type :type/DateTime + :special_type nil + :expression_name "prev_month" + :source :fields + :field_ref [:expression "prev_month"]} + (-> (qp.test-util/with-everything-store + ((annotate/add-column-info (constantly {})) + (data/mbql-query users + {:expressions {:prev_month [:+ $last_login [:interval -1 :month]]} + :fields [[:expression "prev_month"]] + :limit 10}))) + :cols + first)) + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | result-rows-maps->vectors | diff --git a/test/metabase/query_processor_test/date_bucketing_test.clj b/test/metabase/query_processor_test/date_bucketing_test.clj index f4b0758c91c75b19d89667165905ae73a7e8c135..8567e39c6a6be93347e111d6b14d56f1fe5d958a 100644 --- a/test/metabase/query_processor_test/date_bucketing_test.clj +++ b/test/metabase/query_processor_test/date_bucketing_test.clj @@ -21,6 +21,7 @@ [driver :as driver] [query-processor-test :as qp.test :refer :all] [util :as u]] + [metabase.driver.sql.query-processor :as sql.qp] [metabase.test [data :as data] [util :as tu]] @@ -790,7 +791,12 @@ ;; Create timestamps using relative dates (e.g. `DATEADD(second, -195, GETUTCDATE())` instead of ;; generating `java.sql.Timestamps` here so they'll be in the DB's native timezone. Some DBs refuse to use ;; the same timezone we're running the tests from *cough* SQL Server *cough* - [(u/prog1 (driver/date-interval driver/*driver* :second (* i interval-seconds)) + [(u/prog1 (if (isa? driver/hierarchy driver/*driver* :sql) + (driver/date-add driver/*driver* + (sql.qp/current-datetime-fn driver/*driver*) + (* i interval-seconds) + :second) + (du/relative-date :second (* i interval-seconds))) (assert <>))]))]))) (defn- dataset-def-with-timestamps [interval-seconds] diff --git a/test/metabase/query_processor_test/expressions_test.clj b/test/metabase/query_processor_test/expressions_test.clj index fe989c4d8d29fc16793e0c9faeed50461cbb29c8..47238370288d3bed6de83f0003a3158036669122 100644 --- a/test/metabase/query_processor_test/expressions_test.clj +++ b/test/metabase/query_processor_test/expressions_test.clj @@ -1,10 +1,17 @@ (ns metabase.query-processor-test.expressions-test "Tests for expressions (calculated columns)." - (:require [metabase + (:require [clj-time + [coerce :as tcoerce] + [core :as time] + [format :as tformat]] + [metabase [driver :as driver] [query-processor-test :as qp.test]] - [metabase.test.data :as data] - [metabase.test.data.datasets :as datasets])) + [metabase.test + [data :as data] + [util :as tu]] + [metabase.test.data.datasets :as datasets] + [metabase.util.date :as du])) ;; Do a basic query including an expression (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions) @@ -184,3 +191,55 @@ (datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions) [[nil] [0.0] [0.0] [10.0] [8.0] [5.0] [5.0] [nil] [0.0] [0.0]] (calculate-bird-scarcity [:* 1 [:field-id $count]])) + + +;;; +----------------------------------------------------------------------------------------------------------------+ +;;; | DATETIME EXTRACTION AND MANIPULATION | +;;; +----------------------------------------------------------------------------------------------------------------+ + +(def ^:private utc-tz (time/time-zone-for-id "UTC")) + +(defn- maybe-truncate + [dt] + (if (= :sqlite driver/*driver*) + (->> dt (du/date-trunc :day) tcoerce/from-sql-date) + dt)) + +(defn- robust-dates + [dates] + (let [output-format (if (= :sqlite driver/*driver*) + :mysql + :date-time)] + (for [d dates] + [(->> d + (tformat/parse (tformat/with-zone (tformat/formatters :date-hour-minute-second-fraction) utc-tz)) + maybe-truncate + (tformat/unparse (tformat/with-zone (tformat/formatters output-format) utc-tz)))]))) + +;; Test that we can do datetime arithemtics using MBQL `:interval` clause in expressions +(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions) + (robust-dates + ["2014-09-02T13:45:00.000" + "2014-07-02T09:30:00.000" + "2014-07-01T10:30:00.000"]) + (tu/with-temporary-setting-values [report-timezone (.getID utc-tz)] + (-> (data/run-mbql-query users + {:expressions {:prev_month [:+ $last_login [:interval -31 :day]]} + :fields [[:expression :prev_month]] + :limit 3 + :order-by [[:asc $name]]}) + qp.test/rows))) + +;; Test interaction of datetime arithmetics with truncation +(datasets/expect-with-drivers (qp.test/non-timeseries-drivers-with-feature :expressions) + (robust-dates + ["2014-09-02T00:00:00.000" + "2014-07-02T00:00:00.000" + "2014-07-01T00:00:00.000"]) + (tu/with-temporary-setting-values [report-timezone (.getID utc-tz)] + (-> (data/run-mbql-query users + {:expressions {:prev_month [:+ [:datetime-field $last_login :day] [:interval -31 :day]]} + :fields [[:expression :prev_month]] + :limit 3 + :order-by [[:asc $name]]}) + qp.test/rows)))