From c192db1143ed010936b65342e21436f1e9652ba7 Mon Sep 17 00:00:00 2001 From: Cal Herries <39073188+calherries@users.noreply.github.com> Date: Thu, 22 Feb 2024 20:31:42 +0200 Subject: [PATCH] Backport Extend param substitution to handle datetimes to 48 (#38695) (#39062) Co-authored-by: Case Nelson --- docs/developers-guide/driver-changelog.md | 6 ++ .../driver/common/parameters/dates.clj | 20 ++++-- .../driver/sql/parameters/substitution.clj | 61 ++++++++++------ .../driver/sql/parameters/substitute_test.clj | 50 ++++++------- .../sql/parameters/substitution_test.clj | 71 ++++++++++++------- .../date_bucketing_test.clj | 58 +++++++++++++++ 6 files changed, 193 insertions(+), 73 deletions(-) diff --git a/docs/developers-guide/driver-changelog.md b/docs/developers-guide/driver-changelog.md index caa0a9a784e7e..f5520237507b3 100644 --- a/docs/developers-guide/driver-changelog.md +++ b/docs/developers-guide/driver-changelog.md @@ -4,6 +4,12 @@ title: Driver interface changelog # Driver Interface Changelog +## Metabase 0.48.7 + +- The method `metabase.driver.sql.parameters.substitution/align-temporal-unit-with-param-type` is now deprecated. + Use `metabase.driver.sql.parameters.substitution/align-temporal-unit-with-param-type-and-value` instead, + which has access to `value` and therefore provides more flexibility for choosing the right conversion unit. + ## Metabase 0.48.0 - The MBQL schema in `metabase.mbql.schema` now uses [Malli](https://github.com/metosin/malli) instead of diff --git a/src/metabase/driver/common/parameters/dates.clj b/src/metabase/driver/common/parameters/dates.clj index 18b4fdc65f7f5..bfdfc305b685d 100644 --- a/src/metabase/driver/common/parameters/dates.clj +++ b/src/metabase/driver/common/parameters/dates.clj @@ -17,6 +17,12 @@ (set! *warn-on-reflection* true) +(def ^:private temporal-units-regex #"(millisecond|second|minute|hour|day|week|month|quarter|year)") + +(def date-exclude-regex + "Regex to match date exclusion values, e.g. exclude-days-Mon, exclude-months-Jan, etc." + (re-pattern (str "exclude-" temporal-units-regex #"s-([-\p{Alnum}]+)"))) + (mu/defn date-type? "Is param type `:date` or some subtype like `:date/month-year`?" [param-type :- :keyword] @@ -29,6 +35,15 @@ (and (date-type? param-type) (not (#{:date/single :date} param-type)))) +(defn exclusion-date-type + "When date `param-type` represent an exclusion of dates returns the temporal unit that's excluded." + [param-type value] + (when (and (date-type? param-type) + (string? value)) + (some->> (re-matches date-exclude-regex value) + second + keyword))) + ;; Both in MBQL and SQL parameter substitution a field value is compared to a date range, either relative or absolute. ;; Currently the field value is casted to a day (ignoring the time of day), so the ranges should have the same ;; granularity level. @@ -147,7 +162,6 @@ ;; 2) Range decoder which takes the parser output and produces a date range relative to the given datetime ;; 3) Filter decoder which takes the parser output and produces a mbql clause for a given mbql field reference -(def ^:private temporal-units-regex #"(millisecond|second|minute|hour|day|week|month|quarter|year)") (def ^:private relative-suffix-regex (re-pattern (format "(|~|-from-([0-9]+)%ss)" temporal-units-regex))) (defn- include-current? @@ -288,10 +302,6 @@ :month :month-of-year :quarter :quarter-of-year}) -(def date-exclude-regex - "Regex to match date exclusion values, e.g. exclude-days-Mon, exclude-months-Jan, etc." - (re-pattern (str "exclude-" temporal-units-regex #"s-([-\p{Alnum}]+)"))) - (defn- absolute-date->unit [date-string] (if (str/includes? date-string "T") diff --git a/src/metabase/driver/sql/parameters/substitution.clj b/src/metabase/driver/sql/parameters/substitution.clj index 52f1db778b8f9..908a2494074ee 100644 --- a/src/metabase/driver/sql/parameters/substitution.clj +++ b/src/metabase/driver/sql/parameters/substitution.clj @@ -22,6 +22,7 @@ :as qp.wrap-value-literals] [metabase.query-processor.timezone :as qp.timezone] [metabase.query-processor.util.add-alias-info :as add] + [metabase.shared.util.time :as shared.ut] [metabase.util :as u] [metabase.util.date-2 :as u.date] [metabase.util.i18n :refer [tru]] @@ -95,18 +96,42 @@ (make-stmt-subs "?" [t])) (defmulti align-temporal-unit-with-param-type - "Returns a suitable temporal unit conversion keyword for `field`, `param-type` and the given driver. The resulting keyword - will be used to call the corresponding `metabase.driver.sql.query-processor/date` implementation to convert the `field`. - Returns `nil` if the conversion is not necessary for this `field` and `param-type` combination." - {:added "0.48.0" :arglists '([driver field param-type])} + "Returns a suitable temporal unit conversion keyword for `field`, `param-type` and the given driver. + The resulting keyword will be used to call the corresponding `metabase.driver.sql.query-processor/date` + implementation to convert the `field`. + Returns `nil` if the conversion is not necessary for this `field` and `param-type` combination. + Deprecated: use `align-temporal-unit-with-param-type-and-value` instead, as it has access to `value`." + {:added "0.48.0" :deprecated "0.48.7" :arglists '([driver field param-type])} driver/dispatch-on-initialized-driver :hierarchy #'driver/hierarchy) +(defmulti align-temporal-unit-with-param-type-and-value + "Returns a suitable temporal unit conversion keyword for `field`, `param-type`, `value` and the given driver. + The resulting keyword will be used to call the corresponding `metabase.driver.sql.query-processor/date` + implementation to convert the `field`. + Returns `nil` if the conversion is not necessary for this `field`, `param-type` and `value` combination." + {:added "0.48.7" :arglists '([driver field param-type value])} + driver/dispatch-on-initialized-driver + :hierarchy #'driver/hierarchy) + +#_{:clj-kondo/ignore [:deprecated-var]} (defmethod align-temporal-unit-with-param-type :default [_driver _field param-type] (when (params.dates/date-type? param-type) :day)) +(defmethod align-temporal-unit-with-param-type-and-value :default + [_driver _field param-type value] + (when (params.dates/date-type? param-type) + (if-let [exclusion-type (params.dates/exclusion-date-type param-type value)] + exclusion-type + (let [value* (if (params.dates/not-single-date-type? param-type) + (let [param-range (params.dates/date-string->range value)] + (or (:start param-range) (:end param-range))) ;; Before or after filters only have one of these + value)] + (if (re-matches shared.ut/local-date-regex value*) + :day + :minute))))) ;;; ------------------------------------------- ->replacement-snippet-info ------------------------------------------- @@ -203,7 +228,6 @@ {:replacement-snippet (format "BETWEEN %s AND %s" (:sql-string start) (:sql-string end)) :prepared-statement-args (concat (:param-values start) (:param-values end))}))) - ;;; ------------------------------------- Field Filter replacement snippet info -------------------------------------- (s/defn ^:private combine-replacement-snippet-maps :- ParamSnippetInfo @@ -239,7 +263,8 @@ (mu/defn ^:private field->clause :- mbql.s/field [driver :- :keyword field :- lib.metadata/ColumnMetadata - param-type :- ::mbql.s/ParameterType] + param-type :- ::mbql.s/ParameterType + value] ;; The [[metabase.query-processor.middleware.parameters/substitute-parameters]] QP middleware actually happens before ;; the [[metabase.query-processor.middleware.resolve-fields/resolve-fields]] middleware that would normally fetch all ;; the Fields we need in a single pass, so this is actually necessary here. I don't think switching the order of the @@ -249,7 +274,7 @@ [:field (u/the-id field) {:base-type (:base-type field) - :temporal-unit (align-temporal-unit-with-param-type driver field param-type) + :temporal-unit (align-temporal-unit-with-param-type-and-value driver field param-type value) ::add/source-table (:table-id field) ;; in case anyone needs to know we're compiling a Field filter. ::compiling-field-filter? true}]) @@ -258,12 +283,12 @@ "Return an approprate snippet to represent this `field` in SQL given its param type. For non-date Fields, this is just a quoted identifier; for dates, the SQL includes appropriately bucketing based on the `param-type`." - [driver field param-type] + [driver field param-type value] (sql.qp/with-driver-honey-sql-version driver - (->> (field->clause driver field param-type) - (sql.qp/->honeysql driver) - (honeysql->replacement-snippet-info driver) - :replacement-snippet))) + (->> (field->clause driver field param-type value) + (sql.qp/->honeysql driver) + (honeysql->replacement-snippet-info driver) + :replacement-snippet))) (s/defn ^:private field-filter->replacement-snippet-info :- ParamSnippetInfo "Return `[replacement-snippet & prepared-statement-args]` appropriate for a field filter parameter." @@ -271,23 +296,21 @@ (assert (:id field) (format "Why doesn't Field have an ID?\n%s" (u/pprint-to-str field))) (letfn [(prepend-field [x] (update x :replacement-snippet - (partial str (field->identifier driver field param-type) " "))) + (partial str (field->identifier driver field param-type value) " "))) (->honeysql [form] (sql.qp/with-driver-honey-sql-version driver (sql.qp/->honeysql driver form)))] (cond (params.ops/operator? param-type) - (->> (assoc params :target [:template-tag (field->clause driver field param-type)]) + (->> (assoc params :target [:template-tag (field->clause driver field param-type value)]) params.ops/to-clause mbql.u/desugar-filter-clause qp.wrap-value-literals/wrap-value-literals-in-mbql ->honeysql (honeysql->replacement-snippet-info driver)) - (and (params.dates/date-type? param-type) - (string? value) - (re-matches params.dates/date-exclude-regex value)) - (let [field-clause (field->clause driver field param-type)] + (params.dates/exclusion-date-type param-type value) + (let [field-clause (field->clause driver field param-type value)] (->> (params.dates/date-string->filter value field-clause) mbql.u/desugar-filter-clause qp.wrap-value-literals/wrap-value-literals-in-mbql @@ -326,7 +349,6 @@ :else (field-filter->replacement-snippet-info driver field-filter))) - ;;; ------------------------------------ Referenced Card replacement snippet info ------------------------------------ (defmethod ->replacement-snippet-info [:sql ReferencedCardQuery] @@ -334,7 +356,6 @@ {:prepared-statement-args (not-empty params) :replacement-snippet (sql.qp/make-nestable-sql query)}) - ;;; ---------------------------------- Native Query Snippet replacement snippet info --------------------------------- (defmethod ->replacement-snippet-info [:sql ReferencedQuerySnippet] diff --git a/test/metabase/driver/sql/parameters/substitute_test.clj b/test/metabase/driver/sql/parameters/substitute_test.clj index bc6f1a3a30111..d31b0cff011d3 100644 --- a/test/metabase/driver/sql/parameters/substitute_test.clj +++ b/test/metabase/driver/sql/parameters/substitute_test.clj @@ -83,31 +83,32 @@ information about" [] (params/map->FieldFilter - {:field (meta/field-metadata :checkins :date) + {:field (meta/field-metadata :orders :created-at) :value {:type :date/single - :value (t/offset-date-time "2019-09-20T19:52:00.000-07:00")}})) + :value (str (t/offset-date-time "2019-09-20T19:52:00.000-07:00"))}})) (deftest ^:parallel substitute-field-filter-test - (testing "field-filters" - (testing "non-optional" - (let [query ["select * from checkins where " (param "date")]] - (testing "param is present" - (is (= ["select * from checkins where \"PUBLIC\".\"CHECKINS\".\"DATE\" = ?" - [(t/offset-date-time "2019-09-20T19:52:00.000-07:00")]] - (substitute query {"date" (date-field-filter-value)})))) - (testing "param is missing" - (is (= ["select * from checkins where 1 = 1" []] - (substitute query {"date" (assoc (date-field-filter-value) :value params/no-value)})) - "should be replaced with 1 = 1")))) - (testing "optional" - (let [query ["select * from checkins " (optional "where " (param "date"))]] - (testing "param is present" - (is (= ["select * from checkins where \"PUBLIC\".\"CHECKINS\".\"DATE\" = ?" - [#t "2019-09-20T19:52:00.000-07:00"]] - (substitute query {"date" (date-field-filter-value)})))) - (testing "param is missing — should be omitted entirely" - (is (= ["select * from checkins" nil] - (substitute query {"date" (assoc (date-field-filter-value) :value params/no-value)})))))))) + (mt/dataset sample-dataset + (testing "field-filters" + (testing "non-optional" + (let [query ["select * from orders where " (param "created_at")]] + (testing "param is present" + (is (= ["select * from orders where DATE_TRUNC('minute', \"PUBLIC\".\"ORDERS\".\"CREATED_AT\") = ?" + [(t/offset-date-time "2019-09-20T19:52:00.000-07:00")]] + (substitute query {"created_at" (date-field-filter-value)})))) + (testing "param is missing" + (is (= ["select * from orders where 1 = 1" []] + (substitute query {"created_at" (assoc (date-field-filter-value) :value params/no-value)})) + "should be replaced with 1 = 1")))) + (testing "optional" + (let [query ["select * from orders " (optional "where " (param "created_at"))]] + (testing "param is present" + (is (= ["select * from orders where DATE_TRUNC('minute', \"PUBLIC\".\"ORDERS\".\"CREATED_AT\") = ?" + [#t "2019-09-20T19:52:00.000-07:00"]] + (substitute query {"created_at" (date-field-filter-value)})))) + (testing "param is missing — should be omitted entirely" + (is (= ["select * from orders" nil] + (substitute query {"created_at" (assoc (date-field-filter-value) :value params/no-value)}))))))))) (deftest ^:parallel substitute-field-filter-test-2 (testing "new operators" @@ -740,8 +741,9 @@ (testing (str "test that excluding date parts work correctly. It should be enough to try just one type of exclusion " "here, since handling them gets delegated to the functions in `metabase.driver.common.parameters.dates`, " "which is fully-tested :D") - (doseq [[exclusion-string expected] {"exclude-months-Jan" 14 - "exclude-months-Jan-Feb" 13}] + (doseq [[exclusion-string expected] {"exclude-months-Jan" 14 + "exclude-months-Jan-Feb" 13 + "exclude-hours-0-1-2-3-4-5-6-7-8-9-10-11-12" 5}] (testing (format "test that excluding dates with %s works correctly" exclusion-string) (is (= [expected] (mt/first-row diff --git a/test/metabase/driver/sql/parameters/substitution_test.clj b/test/metabase/driver/sql/parameters/substitution_test.clj index a413b5dc47072..eae8beabd56cf 100644 --- a/test/metabase/driver/sql/parameters/substitution_test.clj +++ b/test/metabase/driver/sql/parameters/substitution_test.clj @@ -14,7 +14,7 @@ [metabase.test :as mt] [metabase.util.honey-sql-2 :as h2x]) (:import - (java.time LocalDateTime))) + (java.time LocalDate LocalDateTime))) (set! *warn-on-reflection* true) @@ -28,7 +28,8 @@ (#'sql.params.substitution/field->clause :h2 (meta/field-metadata :venues :id) - :number/=)))) + :number/= + nil)))) (deftest ^:parallel honeysql->replacement-snippet-info-test (testing "make sure we handle quotes inside names correctly!" @@ -68,7 +69,7 @@ :query "SELECT * FROM table WHERE x LIKE ?" :params ["G%"]})))))) -;;; -------------------------------------- align-temporal-unit-with-param-type test ---------------------------------------- +;;; ------------------------------------ align-temporal-unit-with-param-type-and-value test ------------------------------------ (driver/register! ::temporal-unit-alignment-original :abstract? true :parent :sql) (driver/register! ::temporal-unit-alignment-override :abstract? true :parent :sql) @@ -78,12 +79,14 @@ [_driver _feature _db] false)) -(defmethod sql.params.substitution/align-temporal-unit-with-param-type ::temporal-unit-alignment-override - [_driver _field _param-type] +(defmethod sql.params.substitution/align-temporal-unit-with-param-type-and-value ::temporal-unit-alignment-override + [_driver _field _param-type _value] nil) -;; The original implementation will call this method despite the value being past30minutes. This is likely a bug. -;; However, `metabase.query-processor-test.alternative-date-test/substitute-native-parameters-test` depends on it. +(defmethod sql.qp/date [::temporal-unit-alignment-original :minute] + [_driver _unit expr] + (h2x/minute expr)) + (defmethod sql.qp/date [::temporal-unit-alignment-original :day] [_driver _unit expr] (h2x/day expr)) @@ -91,20 +94,40 @@ (deftest align-temporal-unit-with-param-type-test (mt/with-clock #t "2018-07-01T12:30:00.000Z" (mt/with-metadata-provider meta/metadata-provider - (let [field-filter (params/map->FieldFilter - {:field (lib.metadata/field (qp.store/metadata-provider) (meta/id :checkins :date)) - :value {:type :date/all-options, :value "past30minutes"}}) - expected-args [(LocalDateTime/of 2018 7 1 12 00 00) - (LocalDateTime/of 2018 7 1 12 29 00)]] - (testing "default implementation" - (driver/with-driver ::temporal-unit-alignment-original - (is (= {:prepared-statement-args expected-args - ;; `sql.qp/date [driver :day]` was called due to `:day` returned from the multimethod by default - :replacement-snippet "day(\"PUBLIC\".\"CHECKINS\".\"DATE\") BETWEEN ? AND ?"} - (sql.params.substitution/->replacement-snippet-info ::temporal-unit-alignment-original field-filter))))) - (testing "override" - (driver/with-driver ::temporal-unit-alignment-override - (is (= {:prepared-statement-args expected-args - ;; no extra `sql.qp/date` calls due to `nil` returned from the override - :replacement-snippet "\"PUBLIC\".\"CHECKINS\".\"DATE\" BETWEEN ? AND ?"} - (sql.params.substitution/->replacement-snippet-info ::temporal-unit-alignment-override field-filter))))))))) + (mt/dataset sample-dataset + (testing "date" + (let [field-filter (params/map->FieldFilter + {:field (lib.metadata/field (qp.store/metadata-provider) (meta/id :orders :created-at)) + :value {:type :date/all-options, :value "next3days"}}) + expected-args [(LocalDate/of 2018 7 2) + (LocalDate/of 2018 7 4)]] + (testing "default implementation" + (driver/with-driver ::temporal-unit-alignment-original + (is (= {:prepared-statement-args expected-args + ;; `sql.qp/date [driver :day]` was called due to `:day` returned from the multimethod by default + :replacement-snippet "day(\"PUBLIC\".\"ORDERS\".\"CREATED_AT\") BETWEEN ? AND ?"} + (sql.params.substitution/->replacement-snippet-info ::temporal-unit-alignment-original field-filter))))) + (testing "override" + (driver/with-driver ::temporal-unit-alignment-override + (is (= {:prepared-statement-args expected-args + ;; no extra `sql.qp/date` calls due to `nil` returned from the override + :replacement-snippet "\"PUBLIC\".\"ORDERS\".\"CREATED_AT\" BETWEEN ? AND ?"} + (sql.params.substitution/->replacement-snippet-info ::temporal-unit-alignment-override field-filter))))))) + (testing "datetime" + (let [field-filter (params/map->FieldFilter + {:field (lib.metadata/field (qp.store/metadata-provider) (meta/id :orders :created-at)) + :value {:type :date/all-options, :value "past30minutes"}}) + expected-args [(LocalDateTime/of 2018 7 1 12 00 00) + (LocalDateTime/of 2018 7 1 12 29 00)]] + (testing "default implementation" + (driver/with-driver ::temporal-unit-alignment-original + (is (= {:prepared-statement-args expected-args + ;; `sql.qp/date [driver :day]` was called due to `:day` returned from the multimethod by default + :replacement-snippet "minute(\"PUBLIC\".\"ORDERS\".\"CREATED_AT\") BETWEEN ? AND ?"} + (sql.params.substitution/->replacement-snippet-info ::temporal-unit-alignment-original field-filter))))) + (testing "override" + (driver/with-driver ::temporal-unit-alignment-override + (is (= {:prepared-statement-args expected-args + ;; no extra `sql.qp/date` calls due to `nil` returned from the override + :replacement-snippet "\"PUBLIC\".\"ORDERS\".\"CREATED_AT\" BETWEEN ? AND ?"} + (sql.params.substitution/->replacement-snippet-info ::temporal-unit-alignment-override field-filter))))))))))) diff --git a/test/metabase/query_processor_test/date_bucketing_test.clj b/test/metabase/query_processor_test/date_bucketing_test.clj index 57e8a09aa2243..ce02206a02b3c 100644 --- a/test/metabase/query_processor_test/date_bucketing_test.clj +++ b/test/metabase/query_processor_test/date_bucketing_test.clj @@ -1189,6 +1189,64 @@ {:filter [:time-interval $date -4 :month] :breakout [!day.date]}))))))))) +(deftest ^:parallel native-query-datetime-filter-test + (testing "Field Filters with datetime values should behave like gui questions (#33492)" + (mt/dataset sample-dataset + (are [native-type native-value mbql-filter expected-row-count] + (let [mbql-rows (-> (mt/mbql-query orders {:fields [$created_at] + :filter mbql-filter + :order-by [[:asc $created_at]]}) + qp/process-query + mt/rows) + native-rows (-> (mt/native-query {:query (str "SELECT created_at " + "FROM orders " + "WHERE {{date}} " + "ORDER BY created_at") + :template-tags {"date" + {:name "date" + :display-name "Date" + :type :dimension + :widget-type native-type + :dimension (mt/$ids !minute.orders.created_at)}} + :parameters [{:type native-type + :name "date" + :target [:dimension [:template-tag "date"]] + :value native-value}]}) + qp/process-query + mt/rows)] + (is (= expected-row-count (count native-rows))) + (is (= mbql-rows native-rows))) + + :date/range + "2020-03-04~2020-03-04" + [:between !day.created_at "2020-03-04" "2020-03-04"] + 13 + + :date/range + "2020-03-04T07:19:00~2020-03-04T07:20:00" + [:between !minute.created_at "2020-03-04T07:19:00" "2020-03-04T07:20:00"] + 2 + + :date/all-options + "2020-03-04~2020-03-04" + [:between !day.created_at "2020-03-04" "2020-03-04"] + 13 + + :date/all-options + "2020-03-04T07:19:00~2020-03-04T07:20:00" + [:between !minute.created_at "2020-03-04T07:19:00" "2020-03-04T07:20:00"] + 2 + + :date/single + "2020-03-04" + [:= !day.created_at "2020-03-04"] + 13 + + :date/single + "2020-03-04T07:20:00" + [:= !minute.created_at "2020-03-04T07:20"] + 2)))) + (deftest field-filter-start-of-week-test (testing "Field Filters with relative date ranges should respect the custom start of week setting (#14294)" (mt/dataset checkins:1-per-day