Add `align-temporal-unit-with-param-type` multimethod (#34873)

* Add ->temporal-unit multimethod

* Change the method name and add a test

* Fix substitution.clj indentation

docs/developers-guide/driver-changelog.md

@@ -70,6 +70,11 @@ title: Driver interface changelog
   removed in 0.51.0 or later. You can easily implement `metabase.driver/db-default-timezone` directly, and use
   `metabase.driver.sql-jdbc.execute/do-with-connection-with-options` to get a `java.sql.Connection` for a Database.
 
+- Added a new multimethod `metabase.driver.sql.parameters.substitution/align-temporal-unit-with-param-type`, which 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.
+
 ## Metabase 0.47.0
 
 - A new driver feature has been added: `:schemas`. This feature signals whether the database organizes tables in

src/metabase/driver/sql/parameters/substitution.clj

@@ -94,6 +94,19 @@
   [_driver t]
   (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])}
+  driver/dispatch-on-initialized-driver
+  :hierarchy #'driver/hierarchy)
+
+(defmethod align-temporal-unit-with-param-type :default
+  [_driver _field param-type]
+  (when (params.dates/date-type? param-type)
+    :day))
+
 
 ;;; ------------------------------------------- ->replacement-snippet-info -------------------------------------------
 
@@ -224,7 +237,7 @@
      :prepared-statement-args args}))
 
 (mu/defn ^:private field->clause :- mbql.s/field
-  [_driver    :- :keyword
+  [driver     :- :keyword
    field      :- lib.metadata/ColumnMetadata
    param-type :- ::mbql.s/ParameterType]
   ;; The [[metabase.query-processor.middleware.parameters/substitute-parameters]] QP middleware actually happens before
@@ -236,8 +249,7 @@
   [:field
    (u/the-id field)
    {:base-type                (:base-type field)
-    :temporal-unit            (when (params.dates/date-type? param-type)
-                                :day)
+    :temporal-unit            (align-temporal-unit-with-param-type driver field param-type)
     ::add/source-table        (:table-id field)
     ;; in case anyone needs to know we're compiling a Field filter.
     ::compiling-field-filter? true}])

test/metabase/driver/sql/parameters/substitution_test.clj

@@ -3,13 +3,20 @@
   [[metabase.driver.sql.parameters.substitute-test]]."
   (:require
    [clojure.test :refer :all]
+   [metabase.driver :as driver]
    [metabase.driver.common.parameters :as params]
    [metabase.driver.sql.parameters.substitution
     :as sql.params.substitution]
+   [metabase.driver.sql.query-processor :as sql.qp]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.test-metadata :as meta]
    [metabase.query-processor.store :as qp.store]
-   [metabase.test :as mt]))
+   [metabase.test :as mt]
+   [metabase.util.honey-sql-2 :as h2x])
+  (:import
+   (java.time LocalDateTime)))
+
+(set! *warn-on-reflection* true)
 
 (deftest ^:parallel field->clause-test
   (is (=? [:field
@@ -60,3 +67,44 @@
              {:card-id 1
               :query   "SELECT * FROM table WHERE x LIKE ?"
               :params  ["G%"]}))))))
+
+;;; -------------------------------------- align-temporal-unit-with-param-type test ----------------------------------------
+
+(driver/register! ::temporal-unit-alignment-original :abstract? true :parent :sql)
+(driver/register! ::temporal-unit-alignment-override :abstract? true :parent :sql)
+
+(doseq [driver [::temporal-unit-alignment-original ::temporal-unit-alignment-override]]
+  (defmethod driver/database-supports? [driver :set-timezone]
+    [_driver _feature _db]
+    false))
+
+(defmethod sql.params.substitution/align-temporal-unit-with-param-type ::temporal-unit-alignment-override
+  [_driver _field _param-type]
+  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 :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)))))))))