Avoid adding temporal-unit to lhs cols (#46262)

* Avoid adding temporal-unit to lhs cols

* Use :default temporal-unit

* Simplify logic, update comments

* Update substitute-field-filter-test

* Update align-temporal-unit-with-param-type-test

* Update field-filter-date-test

* Use end-excludding gte lt filter for DateTime fields

* Update substitute-field-filter-test

* Update align-temporal-unit-with-param-type-test

* Update field-filter-date-test

* Update bigquery test + comment

* Update date-str->qp-aware-offset-dt

* Add guard for unexpected date string format

This is just for the completeness, I haven't encountered it.

* Address review remarks

* Update comment

modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj

@@ -627,21 +627,38 @@
       :bigquery-cloud-sdk
       (mt/dataset
         attempted-murders
-        (doseq [:let [expectations {["Europe/Oslo" :date "2020-01-09"] #t"2020-01-09",
-                                    ["Europe/Oslo" :datetime "2020-01-09"] #t"2020-01-09T00:00",
-                                    ["Europe/Oslo" :datetime "2020-01-09T01:03"] #t"2020-01-09T01:03",
-                                    ["Europe/Oslo" :datetime_tz "2020-01-09"] #t"2020-01-09T01:00+01:00[Europe/Oslo]",
-                                    ["Europe/Oslo" :datetime_tz "2020-01-09T01:03"] #t"2020-01-09T02:03+01:00[Europe/Oslo]",
-                                    ["UTC" :date "2020-01-09"] #t"2020-01-09",
-                                    ["UTC" :datetime "2020-01-09"] #t"2020-01-09T00:00",
-                                    ["UTC" :datetime "2020-01-09T01:03"] #t"2020-01-09T01:03",
-                                    ["UTC" :datetime_tz "2020-01-09"] #t"2020-01-09T00:00Z[UTC]",
-                                    ["UTC" :datetime_tz "2020-01-09T01:03"] #t"2020-01-09T01:03Z[UTC]",
-                                    [nil :date "2020-01-09"] #t"2020-01-09"
-                                    [nil :datetime "2020-01-09"] #t"2020-01-09T00:00",
-                                    [nil :datetime "2020-01-09T01:03"] #t"2020-01-09T01:03",
-                                    [nil :datetime_tz "2020-01-09"] (t/offset-date-time (u.date/parse "2020-01-09T00:00Z"))
-                                    [nil :datetime_tz "2020-01-09T01:03"] #t"2020-01-09T01:03Z[UTC]"}]
+        (doseq [:let [expectations {["Europe/Oslo" :date "2020-01-09"]
+                                    [#t"2020-01-09"],
+                                    ["Europe/Oslo" :datetime "2020-01-09"]
+                                    [#t "2020-01-09T00:00" #t "2020-01-10T00:00"],
+                                    ["Europe/Oslo" :datetime "2020-01-09T01:03"]
+                                    [#t "2020-01-09T01:03" #t "2020-01-09T01:04"],
+                                    ["Europe/Oslo" :datetime_tz "2020-01-09"]
+                                    [#t "2020-01-09T00:00+01:00[Europe/Oslo]" #t "2020-01-10T00:00+01:00[Europe/Oslo]"],
+                                    ["Europe/Oslo" :datetime_tz "2020-01-09T01:03"]
+                                    [#t "2020-01-09T01:03+01:00[Europe/Oslo]" #t "2020-01-09T01:04+01:00[Europe/Oslo]"],
+
+                                    ["UTC" :date "2020-01-09"]
+                                    [#t"2020-01-09"],
+                                    ["UTC" :datetime "2020-01-09"]
+                                    [#t "2020-01-09T00:00" #t "2020-01-10T00:00"],
+                                    ["UTC" :datetime "2020-01-09T01:03"]
+                                    [#t "2020-01-09T01:03" #t "2020-01-09T01:04"],
+                                    ["UTC" :datetime_tz "2020-01-09"]
+                                    [#t "2020-01-09T00:00Z[UTC]" #t "2020-01-10T00:00Z[UTC]"],
+                                    ["UTC" :datetime_tz "2020-01-09T01:03"]
+                                    [#t "2020-01-09T01:03Z[UTC]" #t "2020-01-09T01:04Z[UTC]"],
+
+                                    [nil :date "2020-01-09"]
+                                    [#t"2020-01-09"]
+                                    [nil :datetime "2020-01-09"]
+                                    [#t "2020-01-09T00:00" #t "2020-01-10T00:00"],
+                                    [nil :datetime "2020-01-09T01:03"]
+                                    [#t "2020-01-09T01:03" #t "2020-01-09T01:04"],
+                                    [nil :datetime_tz "2020-01-09"]
+                                    [#t "2020-01-09T00:00Z[UTC]" #t "2020-01-10T00:00Z[UTC]"]
+                                    [nil :datetime_tz "2020-01-09T01:03"]
+                                    [#t "2020-01-09T01:03Z[UTC]" #t "2020-01-09T01:04Z[UTC]"]}]
                 tz [nil "Europe/Oslo" "UTC"]
                 field [:date :datetime :datetime_tz]
                 value (cond-> ["2020-01-09"]
@@ -667,7 +684,7 @@
                                          :name "d"
                                          :target [:dimension [:template-tag "d"]]
                                          :value value}]}]
-                (is (= [expected] (:params (qp.compile/compile query))))))))))))
+                (is (= expected (:params (qp.compile/compile query))))))))))))
 
 (deftest current-datetime-honeysql-form-test
   (mt/test-driver :bigquery-cloud-sdk

src/metabase/driver/common/parameters.clj

@@ -73,6 +73,11 @@
   (pretty [_]
     (list (pretty/qualify-symbol-for-*ns* `->DateRange) start end)))
 
+(p.types/defrecord+ DateTimeRange [start end]
+  pretty/PrettyPrintable
+  (pretty [_]
+          (list (pretty/qualify-symbol-for-*ns* `->DateRange) start end)))
+
 (def no-value
   "Convenience for representing an *optional* parameter present in a query but whose value is unspecified in the param
   values."

src/metabase/driver/common/parameters/dates.clj

@@ -10,6 +10,8 @@
    [metabase.lib.schema.id :as lib.schema.id]
    [metabase.lib.schema.parameter :as lib.schema.parameter]
    [metabase.query-processor.error-type :as qp.error-type]
+   [metabase.query-processor.timezone :as qp.timezone]
+   [metabase.shared.util.time :as shared.ut]
    [metabase.util.date-2 :as u.date]
    [metabase.util.i18n :refer [tru]]
    [metabase.util.malli :as mu])
@@ -468,6 +470,65 @@
                          {:param date-string
                           :type  qp.error-type/invalid-parameter}))))))
 
+(defn- date-str->qp-aware-offset-dt
+  "Generate offset datetime from `date-str` with respect to qp's `results-timezone`."
+  [date-str]
+  (when date-str
+    (let [[y M d h m s] (shared.ut/yyyyMMddhhmmss->parts date-str)]
+      (try (.toOffsetDateTime (t/zoned-date-time y M d h m s 0 (t/zone-id (qp.timezone/results-timezone-id))))
+           (catch Throwable _
+             (t/offset-date-time y M d h m s 0 (t/zone-offset (qp.timezone/results-timezone-id))))))))
+
+(defn- date-str->unit-fn
+  "Return appropriate function for interval end adjustments in [[exclusive-datetime-range-end]]."
+  [date-str]
+  (when date-str
+    (if (re-matches shared.ut/local-date-regex date-str)
+      t/days
+      t/minutes)))
+
+(defn- exclusive-datetime-range-end
+  "Transform `end-dt` OffsetDateTime to appropriate range end.
+
+  Context. Datetime range is required for `FieldFilter`s on `:type/DateTime` fields (see the
+  [[metabase.driver.sql.parameters.substitution/field-filter->replacement-snippet-info]]) instead of _Date Range_
+  available from [[date-string->range]].
+
+  [[date-string->range]] returns interval of dates. [[date-str->datetime-range]] modifies the interval to consist
+  of datetimes. By adding 0 temporal padding the end interval has to be adjusted."
+  [end-dt unit-fn]
+  (when (and end-dt unit-fn)
+    (t/+ end-dt (unit-fn 1))))
+
+(defn- fallback-raw-range
+  "Try to extract date time value if [[date-string->range]] fails."
+  [date-str]
+  (let [date-str (first (re-find #"\d+-\d+-\d+T?(\d?+)?(:\d+)?(:\d+)?" date-str))]
+    {:start date-str
+     :end   date-str}))
+
+(mu/defn date-str->datetime-range :- DateStringRange
+  "Generate range from `date-range-str`.
+
+  First [[date-string->range]] generates range for dates (inclusive by default). Operating on that range,
+  this function:
+  1. converts dates to OffsetDateTime, respecting qp's timezone, adding zero temporal padding,
+  2. updates range to correct _end-exclusive datetime_*
+  3. formats the range.
+
+  This function is meant to be used for generating inclusive intervals for `:type/DateTime` field filters.
+
+  * End-exclusive gte lt filters are generated for `:type/DateTime` fields."
+  [raw-date-str]
+  (let [;; `raw-date-str` is sanitized in case it contains millis and timezone which are incompatible
+        ;; with [[date-string->range]]. `substitute-field-filter-test` expects that to happen.
+        range-raw (try (date-string->range raw-date-str)
+                       (catch Throwable _
+                         (fallback-raw-range raw-date-str)))]
+    (-> (update-vals range-raw date-str->qp-aware-offset-dt)
+        (update :end exclusive-datetime-range-end (date-str->unit-fn (:end range-raw)))
+        format-date-range)))
+
 (mu/defn date-string->filter :- mbql.s/Filter
   "Takes a string description of a *date* (not datetime) range such as 'lastmonth' or '2016-07-15~2016-08-6' and
    returns a corresponding MBQL filter clause for a given field reference."

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

@@ -21,7 +21,6 @@
    [metabase.query-processor.error-type :as qp.error-type]
    [metabase.query-processor.middleware.wrap-value-literals :as qp.wrap-value-literals]
    [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]]
@@ -30,7 +29,7 @@
    (clojure.lang IPersistentVector Keyword)
    (java.time.temporal Temporal)
    (java.util UUID)
-   (metabase.driver.common.parameters Date DateRange FieldFilter ReferencedCardQuery ReferencedQuerySnippet)))
+   (metabase.driver.common.parameters Date DateRange DateTimeRange FieldFilter ReferencedCardQuery ReferencedQuerySnippet)))
 
 ;;; ------------------------------------ ->prepared-substitution & default impls -------------------------------------
 
@@ -119,13 +118,7 @@
   (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)))))
+      :default)))
 
 ;;; ------------------------------------------- ->replacement-snippet-info -------------------------------------------
 
@@ -140,7 +133,7 @@
 
     (->replacement-snippet-info :h2 \"ABC\") -> {:replacement-snippet \"?\", :prepared-statement-args \"ABC\"}"
   {:added "0.33.4" :arglists '([driver value])}
-  (fn [driver v] [(driver/the-initialized-driver driver) (class v)])
+  (fn [driver v & _args] [(driver/the-initialized-driver driver) (class v)])
   :hierarchy #'driver/hierarchy)
 
 (defn- create-replacement-snippet
@@ -220,6 +213,15 @@
       {:replacement-snippet     (format "BETWEEN %s AND %s" (:sql-string start) (:sql-string end))
        :prepared-statement-args (concat (:param-values start) (:param-values end))})))
 
+(defmethod ->replacement-snippet-info [:sql DateTimeRange]
+  [driver {:keys [start end]} & [field-identifier]]
+  (let [[start end] (map (fn [s]
+                           (->prepared-substitution driver (maybe-parse-temporal-literal s)))
+                         [start end])]
+    {:replacement-snippet     (format "%s >= %s AND %s < %s"
+                                      field-identifier (:sql-string start) field-identifier (:sql-string end))
+     :prepared-statement-args (concat (:param-values start) (:param-values end))}))
+
 ;;; ------------------------------------- Field Filter replacement snippet info --------------------------------------
 
 (mu/defn- combine-replacement-snippet-maps :- ParamSnippetInfo
@@ -281,9 +283,20 @@
        (honeysql->replacement-snippet-info driver)
        :replacement-snippet))
 
+(defn- field-filter->replacement-snippet-for-datetime-field
+  "Generate replacement snippet for field filter on datetime field. For details on how range is generated see
+  the docstring of [[params.dates/date-str->datetime-range]]."
+  [driver {:keys [field] {:keys [value type]} :value :as _field-filter}]
+  (letfn [(->datetime-replacement-snippet-info
+            [range]
+            (->replacement-snippet-info driver range (field->identifier driver field type value)))]
+    (-> (params.dates/date-str->datetime-range value)
+        params/map->DateTimeRange
+        ->datetime-replacement-snippet-info)))
+
 (mu/defn- 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}]
+  [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
@@ -307,9 +320,15 @@
              ->honeysql
              (honeysql->replacement-snippet-info driver)))
 
+      ;; Special handling for `FieldFilter`s on `:type/DateTime` fields. DateTime range is always generated.
+      (and (params.dates/date-type? param-type)
+           (isa? ((some-fn :effective-type :base-type) field) :type/DateTime))
+      (field-filter->replacement-snippet-for-datetime-field driver field-filter)
+
       ;; convert other date to DateRange record types
       (params.dates/not-single-date-type? param-type) (prepend-field
                                                        (date-range-field-filter->replacement-snippet-info driver value))
+
       ;; convert all other dates to `= <date>`
       (params.dates/date-type? param-type)            (prepend-field
                                                        (field-filter->equals-clause-sql driver (params/map->Date {:s value})))

src/metabase/shared/util/time.cljc

@@ -3,6 +3,7 @@
   In Java these return [[OffsetDateTime]], in JavaScript they return Moments.
   Most of the implementations are in the split CLJ/CLJS files [[metabase.shared.util.internal.time]]."
   (:require
+   [clojure.string :as str]
    [metabase.shared.util.internal.time :as internal]
    [metabase.shared.util.internal.time-common :as common]
    [metabase.shared.util.namespaces :as shared.ns]
@@ -102,3 +103,12 @@
    (internal/format-relative-date-range n unit offset-n offset-unit options))
   ([t n unit offset-n offset-unit options]
    (internal/format-relative-date-range (coerce-to-timestamp t) n unit offset-n offset-unit options)))
+
+(defn yyyyMMddhhmmss->parts
+  "Generate parts vector for `yyyy-MM-ddThh:mm:ss` format `date-str`. Trailing parts, if not present in the string,
+  are added. Not compatile with strings containing milliseconds or timezone."
+  [date-str]
+  (let [parts (mapv #?(:clj #(Integer/parseInt %)
+                       :cljs js/parseInt)
+                    (str/split date-str #"-|:|T"))]
+    (into parts (repeat (- 6 (count parts)) 0))))

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

@@ -91,8 +91,9 @@
     (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")]]
+          (is (= ["select * from orders where \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?"
+                  [(t/zoned-date-time "2019-09-20T19:52:00" (t/zone-id "UTC"))
+                   (t/zoned-date-time "2019-09-20T19:53:00" (t/zone-id "UTC"))]]
                  (substitute query {"created_at" (date-field-filter-value)}))))
         (testing "param is missing"
           (is (= ["select * from orders where 1 = 1" []]
@@ -101,8 +102,9 @@
     (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"]]
+          (is (= ["select * from orders where \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?"
+                  [(t/zoned-date-time "2019-09-20T19:52:00" (t/zone-id "UTC"))
+                   (t/zoned-date-time "2019-09-20T19:53:00" (t/zone-id "UTC"))]]
                  (substitute query {"created_at" (date-field-filter-value)}))))
         (testing "param is missing — should be omitted entirely"
           (is (= ["select * from orders" nil]

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

@@ -3,6 +3,7 @@
   [[metabase.driver.sql.parameters.substitute-test]]."
   (:require
    [clojure.test :refer :all]
+   [java-time.api :as t]
    [metabase.driver :as driver]
    [metabase.driver.common.parameters :as params]
    [metabase.driver.sql.parameters.substitution
@@ -12,9 +13,7 @@
    [metabase.lib.test-metadata :as meta]
    [metabase.query-processor.store :as qp.store]
    [metabase.test :as mt]
-   [metabase.util.honey-sql-2 :as h2x])
-  (:import
-   (java.time LocalDate LocalDateTime)))
+   [metabase.util.honey-sql-2 :as h2x]))
 
 (set! *warn-on-reflection* true)
 
@@ -100,35 +99,35 @@
         (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)]]
+              expected-args [(t/zoned-date-time 2018 7 2 0 0 0 0 (t/zone-id "UTC"))
+                             (t/zoned-date-time 2018 7 5 0 0 0 0 (t/zone-id "UTC"))]]
           (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 ?"}
+                      :replacement-snippet "\"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?"}
                      (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 ?"}
+                      :replacement-snippet "\"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?"}
                      (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)]]
+              expected-args [(t/zoned-date-time 2018 7 1 12 0 0 0 (t/zone-id "UTC"))
+                             (t/zoned-date-time 2018 7 1 12 30 0 0 (t/zone-id "UTC"))]]
           (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 ?"}
+                      :replacement-snippet "\"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?"}
                      (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 ?"}
+                      :replacement-snippet "\"PUBLIC\".\"ORDERS\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"ORDERS\".\"CREATED_AT\" < ?"}
                      (sql.params.substitution/->replacement-snippet-info ::temporal-unit-alignment-override field-filter))))))))))

test/metabase/query_analysis/native_query_analyzer/parameter_substitution_test.clj

@@ -144,7 +144,7 @@
 
 (deftest field-filter-date-test
   (doseq [date-filter ["created_range" "created_my" "created_qy" "created_rel" "date_ao"]]
-    (is (= "SELECT * FROM people WHERE CAST(\"PUBLIC\".\"PEOPLE\".\"CREATED_AT\" AS date) BETWEEN ? AND ?"
+    (is (= "SELECT * FROM people WHERE \"PUBLIC\".\"PEOPLE\".\"CREATED_AT\" >= ? AND \"PUBLIC\".\"PEOPLE\".\"CREATED_AT\" < ?"
            (->sql (mt/native-query {:template-tags (tags date-filter)
                                     :query         (format "SELECT * FROM people WHERE {{%s}}" date-filter)}))))))