Backport Extend param substitution to handle datetimes to 48 (#38695) (…

…#39062)

Co-authored-by: Case Nelson <case@metabase.com>

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

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")

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,36 +283,34 @@
   "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."
   [driver {{param-type :type, value :value, :as params} :value, field :field, :as _field-filter}]
   (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,15 +349,13 @@
     :else
     (field-filter->replacement-snippet-info driver field-filter)))
 
-
 ;;; ------------------------------------ Referenced Card replacement snippet info ------------------------------------
 
 (defmethod ->replacement-snippet-info [:sql ReferencedCardQuery]
   [_ {:keys [query params]}]
   {: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]

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

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,33 +79,55 @@
     [_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))
 
 (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)))))))))))