Add `align-temporal-unit-with-param-type-and-value` multimethod (#37813)

* Add `align-temporal-unit-with-param-type-and-value` multimethod

* Fix tests and add a changelog entry

* Update the method docs

* Rearrange the default methods call order

* Revert formatting changes

* Move the clj-kondo ignore

* Update docs/developers-guide/driver-changelog.md

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>

---------

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>

docs/developers-guide/driver-changelog.md

@@ -21,6 +21,10 @@ title: Driver interface changelog
   an additional argument: `database`.
   The new function arglist is `[driver database connection schema-inclusion-filters schema-exclusion-filters]`.
 
+- 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/sql/parameters/substitution.clj

@@ -90,18 +90,34 @@
   (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.49.0" :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.49.0" :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]
+  #_{:clj-kondo/ignore [:deprecated-var]}
+  (align-temporal-unit-with-param-type driver field param-type))
 
 ;;; ------------------------------------------- ->replacement-snippet-info -------------------------------------------
 
@@ -195,7 +211,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 --------------------------------------
 
 (mu/defn ^:private combine-replacement-snippet-maps :- ParamSnippetInfo
@@ -231,7 +246,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
@@ -241,7 +257,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}])
@@ -250,8 +266,8 @@
   "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]
-  (->> (field->clause driver field param-type)
+  [driver field param-type value]
+  (->> (field->clause driver field param-type value)
        (sql.qp/->honeysql driver)
        (honeysql->replacement-snippet-info driver)
        :replacement-snippet))
@@ -262,12 +278,12 @@
   (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/->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
@@ -277,7 +293,7 @@
       (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)]
+      (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
@@ -316,7 +332,6 @@
     :else
     (field-filter->replacement-snippet-info driver field-filter)))
 
-
 ;;; ------------------------------------ Referenced Card replacement snippet info ------------------------------------
 
 (defmethod ->replacement-snippet-info [:sql ReferencedCardQuery]
@@ -324,7 +339,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]

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

@@ -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,8 +79,8 @@
     [_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.