Fix Druid queries with temporal Fields that don't have a :temporal-unit (#16028)

* Fix Druid queries with multiple filters if some but not all are temporal

* Fix Druid queries with temporal Fields that don't have a :temporal-unit

* Test fix 

* Fix syntax errors

* Query result metadata should return the :unit from the source query when applicable

* Test fix 

* Clarify comment in test

* More test fixes 

* Test fix 

modules/drivers/druid/test/metabase/driver/druid/query_processor_test.clj

@@ -671,3 +671,13 @@
                    :filter      [:and
                                  [:= $venue_category_name "Mexican"]
                                  [:= !month.timestamp "2015-09"]]}))))))))
+
+(deftest open-ended-temporal-filter-test
+  (mt/test-driver :druid
+    (testing "Should be able to filter by an open-ended absolute temporal moment (#15902)"
+      (tqpt/with-flattened-dbdef
+        (is (= [58]
+               (mt/first-row
+                (mt/run-mbql-query checkins
+                  {:aggregation [[:count]]
+                   :filter      [:> $timestamp "2015-10-01T00:00:00Z"]}))))))))

src/metabase/query_processor.clj

@@ -13,6 +13,7 @@
             [metabase.plugins.classloader :as classloader]
             [metabase.query-processor.context :as context]
             [metabase.query-processor.error-type :as error-type]
+            [metabase.query-processor.middleware.add-default-temporal-unit :as add-default-temporal-unit]
             [metabase.query-processor.middleware.add-dimension-projections :as add-dim]
             [metabase.query-processor.middleware.add-implicit-clauses :as implicit-clauses]
             [metabase.query-processor.middleware.add-implicit-joins :as add-implicit-joins]
@@ -93,6 +94,7 @@
    #'add-implicit-joins/add-implicit-joins
    #'large-int-id/convert-id-to-string
    #'format-rows/format-rows
+   #'add-default-temporal-unit/add-default-temporal-unit
    #'desugar/desugar
    #'binning/update-binning-strategy
    #'resolve-fields/resolve-fields

src/metabase/query_processor/context/default.clj

@@ -15,7 +15,7 @@
   ;; code
   (cond
     config/is-prod? (u/minutes->ms 20)
-    config/is-test? (u/seconds->ms 30)
+    config/is-test? (u/seconds->ms 60)
     config/is-dev?  (u/minutes->ms 3)))
 
 (defn default-rff

src/metabase/query_processor/middleware/add_default_temporal_unit.clj

+(ns metabase.query-processor.middleware.add-default-temporal-unit
+  (:require [metabase.mbql.util :as mbql.u]
+            [metabase.query-processor.store :as qp.store]))
+
+(defn- add-default-temporal-unit* [query]
+  (mbql.u/replace-in query [:query]
+    [:field (_ :guard string?) (_ :guard (every-pred
+                                          :base-type
+                                          #(isa? (:base-type %) :type/Temporal)
+                                          (complement :temporal-unit)))]
+    (mbql.u/with-temporal-unit &match :default)
+
+    [:field (id :guard integer?) (_ :guard (complement :temporal-unit))]
+    (let [{base-type :base_type, effective-type :effective_type} (qp.store/field id)]
+      (cond-> &match
+        (isa? (or effective-type base-type) :type/Temporal) (mbql.u/with-temporal-unit :default)))))
+
+(defn add-default-temporal-unit
+  "Add `:temporal-unit` `:default` to any temporal `:field` clauses that don't already have a `:temporal-unit`. This
+  makes things more consistent because code downstream can rely on the key being present."
+  [qp]
+  (fn [query rff context]
+    (qp (add-default-temporal-unit* query) rff context)))

src/metabase/query_processor/middleware/annotate.clj

@@ -177,10 +177,6 @@
                (join-with-alias inner-query (:join-alias opts)))]
     ;; TODO -- I think we actually need two `:field_ref` columns -- one for referring to the Field at the SAME
     ;; level, and one for referring to the Field from the PARENT level.
-    ;;
-    ;; If temporal bucketing is applied to a Field in a source query, you should not re-bucket it when you refer to it
-    ;; outside that source query; hence we remove the `temporal-unit` below. However you should keep the bucketing
-    ;; unit to refer to the Field *WITHIN* the source query, e.g. to add a sort on that column.
     (cond-> {:field_ref clause}
       (:base-type opts)
       (assoc :base_type (:base-type opts))
@@ -461,13 +457,24 @@
 
 (declare mbql-cols)
 
+(s/defn ^:private merge-source-metadata-col :- (s/maybe su/Map)
+  [source-metadata-col :- (s/maybe su/Map) col :- (s/maybe su/Map)]
+  (merge
+   source-metadata-col
+   col
+   ;; pass along the unit from the source query metadata if the top-level metadata has unit `:default`. This way the
+   ;; frontend will display the results correctly if bucketing was applied in the nested query, e.g. it will format
+   ;; temporal values in results using that unit
+   (when (= (:unit col) :default)
+     (select-keys source-metadata-col [:unit]))))
+
 (defn- maybe-merge-source-metadata
   "Merge information from `source-metadata` into the returned `cols` for queries that return the columns of a source
   query as-is (i.e., the parent query does not have breakouts, aggregations, or an explicit`:fields` clause --
   excluding the one added automatically by `add-source-metadata`)."
   [source-metadata cols]
   (if (= (count cols) (count source-metadata))
-    (map merge source-metadata cols)
+    (map merge-source-metadata-col source-metadata cols)
     cols))
 
 (defn- flow-field-metadata
@@ -476,7 +483,7 @@
   (let [field-id->metadata (u/key-by :id source-metadata)]
     (for [col cols]
       (if-let [source-metadata-for-field (-> col :id field-id->metadata)]
-        (merge source-metadata-for-field col)
+        (merge-source-metadata-col source-metadata-for-field col)
         col))))
 
 (defn- cols-for-source-query

src/metabase/query_processor/middleware/auto_bucket_datetimes.clj

@@ -135,10 +135,10 @@
      query)))
 
 (defn auto-bucket-datetimes
-  "Middleware that automatically adds `:temporal-unit` to breakout and filter `:field` clauses if the Field they refer
-  to has a type that derives from `:type/Temporal` (but not `:type/Time`). (This is done for historic reasons, before
-  datetime bucketing was added to MBQL; datetime Fields defaulted to breaking out by day. We might want to revisit
-  this behavior in the future.)
+  "Middleware that automatically adds `:temporal-unit` `:day` to breakout and filter `:field` clauses if the Field they
+  refer to has a type that derives from `:type/Temporal` (but not `:type/Time`). (This is done for historic reasons,
+  before datetime bucketing was added to MBQL; datetime Fields defaulted to breaking out by day. We might want to
+  revisit this behavior in the future.)
 
   Applies to any unbucketed Field in a breakout, or fields in a filter clause being compared against `yyyy-MM-dd`
   format datetime strings."

src/metabase/util/schema.clj

@@ -224,7 +224,7 @@
 
 (def Map
   "Schema for a valid map."
-  (with-api-error-message (s/pred map? (deferred-tru "Valid map"))
+  (with-api-error-message (s/named clojure.lang.IPersistentMap (deferred-tru "Valid map"))
     (deferred-tru "value must be a map.")))
 
 (def Email

test/metabase/query_processor/middleware/add_default_temporal_unit_test.clj

+(ns metabase.query-processor.middleware.add-default-temporal-unit-test
+  (:require [clojure.test :refer :all]
+            [metabase.query-processor.middleware.add-default-temporal-unit :as add-default-temporal-unit]
+            [metabase.test :as mt]))
+
+(defn- add-default-temporal-unit [query]
+  (mt/with-everything-store
+    (:pre (mt/test-qp-middleware add-default-temporal-unit/add-default-temporal-unit query))))
+
+(deftest add-default-temporal-unit-test
+  (testing "Should add temporal-unit :default to a :field clause"
+    (testing "with a Field ID"
+      (is (= (mt/mbql-query checkins
+               {:filter [:> !default.date "2021-05-13T00:00:00Z"]})
+             (add-default-temporal-unit
+              (mt/mbql-query checkins
+                {:filter [:> $date "2021-05-13T00:00:00Z"]})))))
+    (testing "with a Field name and temporal base type"
+      (is (= (mt/mbql-query checkins
+               {:filter [:>
+                         [:field "date" {:base-type :type/TimeWithLocalTZ, :temporal-unit :default}]
+                         "2021-05-13T00:00:00Z"]})
+             (add-default-temporal-unit
+              (mt/mbql-query checkins
+                {:filter [:>
+                          [:field "date" {:base-type :type/TimeWithLocalTZ}]
+                          "2021-05-13T00:00:00Z"]}))))))
+
+  (testing "Should ignore fields that already have a temporal unit"
+    (testing "with an ID"
+      (let [query (mt/mbql-query checkins
+                    {:filter [:> !month.date "2021-05-13T00:00:00Z"]})]
+        (is (= query
+               (add-default-temporal-unit query)))))
+    (testing "with a field name"
+      (let [query (mt/mbql-query checkins
+                    {:filter [:>
+                              [:field "date" {:base-type :type/TimeWithLocalTZ, :temporal-unit :month}]
+                              "2021-05-13T00:00:00Z"]})]
+        (is (= query
+               (add-default-temporal-unit query))))))
+
+  (testing "Should ignore non-temporal fields"
+    (testing "with an ID"
+      (let [query (mt/mbql-query venues
+                    {:filter [:> $price 3]})]
+        (is (= query
+               (add-default-temporal-unit query)))))
+    (testing "with a field name"
+      (let [query (mt/mbql-query checkins
+                    {:filter [:>
+                              [:field "price" {:base-type :type/Integer}]
+                              3]})]
+        (is (= query
+               (add-default-temporal-unit query)))))))
+
+(deftest ignore-parameters-test
+  (testing "Don't try to update query `:parameters`"
+    (let [query {:database   (mt/id)
+                 :type       :native
+                 :native     {:query "select 111 as my_number, 'foo' as my_string"}
+                 :parameters [{:type   "category"
+                               :value  [:param-value]
+                               :target [:dimension
+                                        [:field
+                                         (mt/id :categories :id)
+                                         {:source-field (mt/id :venues :category_id)}]]}]}]
+      (is (= query
+             (add-default-temporal-unit query))))))

test/metabase/query_processor/middleware/annotate_test.clj

@@ -120,10 +120,10 @@
                                       :strategy     :left-join}]}}
                    {:columns [:name]})))))))))
 
-(deftest col-info-for-datetime-field-test
+(deftest col-info-for-field-with-temporal-unit-test
   (mt/with-everything-store
     (mt/$ids venues
-      (testing "when a `:datetime-field` form is used, we should add in info about the `:unit`"
+      (testing "when a `:field` with `:temporal-unit` is used, we should add in info about the `:unit`"
         (is (= [(merge (info-for-field :venues :price)
                        {:unit      :month
                         :source    :fields
@@ -143,7 +143,38 @@
                (doall
                 (annotate/column-info
                  {:type :query, :query {:fields [[:field "price" {:base-type :type/Number, :temporal-unit :month}]]}}
-                 {:columns [:price]}))))))))
+                 {:columns [:price]}))))))
+
+    (testing "should add the correct info if the Field originally comes from a nested query"
+      (mt/$ids checkins
+        (is (= [{:name "DATE", :unit :month, :field_ref [:field %date {:temporal-unit :default}]}
+                {:name "LAST_LOGIN", :unit :month, :field_ref [:field
+                                                               %users.last_login
+                                                               {:temporal-unit :default
+                                                                :join-alias    "USERS__via__USER_ID"}]}]
+               (mapv
+                (fn [col]
+                  (select-keys col [:name :unit :field_ref]))
+                (annotate/column-info
+                 {:type  :query
+                  :query {:source-query    {:source-table $$checkins
+                                            :breakout     [[:field %date {:temporal-unit :month}]
+                                                           [:field
+                                                            %users.last_login
+                                                            {:temporal-unit :month, :source-field %user_id}]]}
+                          :source-metadata [{:name      "DATE"
+                                             :id        %date
+                                             :unit      :month
+                                             :field_ref [:field %date {:temporal-unit :month}]}
+                                            {:name      "LAST_LOGIN"
+                                             :id        %users.last_login
+                                             :unit      :month
+                                             :field_ref [:field %users.last_login {:temporal-unit :month
+                                                                                   :source-field  %user_id}]}]
+                          :fields          [[:field %date {:temporal-unit :default}]
+                                            [:field %users.last_login {:temporal-unit :default, :join-alias "USERS__via__USER_ID"}]]
+                          :limit           1}}
+                 nil))))))))
 
 (deftest col-info-for-binning-strategy-test
   (testing "when binning strategy is used, include `:binning_info`"

test/metabase/query_processor_test/nested_queries_test.clj

@@ -426,10 +426,15 @@
                                                  :data :cols)]
                                      (-> (into {} col)
                                          (assoc :source :fields)))]
-          (is (= [(assoc date-col  :field_ref [:field (mt/id :checkins :date) nil])
+          ;; since the bucketing is happening in the source query rather than at this level, the field ref should
+          ;; return temporal unit `:default` rather than the upstream bucketing unit. You wouldn't want to re-apply
+          ;; the `:year` bucketing if you used this query in another subsequent query, so the field ref doesn't
+          ;; include the unit; however `:unit` is still `:year` so the frontend can use the correct formatting to
+          ;; display values of the column.
+          (is (= [(assoc date-col  :field_ref [:field (mt/id :checkins :date) {:temporal-unit :default}], :unit :year)
                   (assoc count-col :field_ref [:field "count" {:base-type (:base_type count-col)}])]
                  (mt/cols
-                   (qp/process-query (query-with-source-card card))))))))))
+                  (qp/process-query (query-with-source-card card))))))))))
 
 (defn- completed-status [{:keys [status], :as results}]
   (if (= status :completed)

test/metabase/query_processor_test/parameters_test.clj

@@ -2,6 +2,7 @@
   "Tests for support for parameterized queries in drivers that support it. (There are other tests for parameter support
   in various places; these are mainly for high-level verification that parameters are working.)"
   (:require [clojure.test :refer :all]
+            [medley.core :as m]
             [metabase.driver :as driver]
             [metabase.models :refer [Card]]
             [metabase.query-processor :as qp]
@@ -185,3 +186,19 @@
                :parameters [{:type   :category
                              :target [:dimension [:template-tag "price"]]
                              :value  [1 2]}]}))))))
+
+(deftest ignore-parameters-for-unparameterized-native-query-test
+  (testing "Parameters passed for unparameterized queries should get ignored"
+    (let [query {:database (mt/id)
+                 :type     :native
+                 :native   {:query "select 111 as my_number, 'foo' as my_string"}}]
+      (is (= (-> (qp/process-query query)
+                 (m/dissoc-in [:data :results_metadata :checksum]))
+             (-> (qp/process-query (assoc query :parameters [{:type   "category"
+                                                              :value  [:param-value]
+                                                              :target [:dimension
+                                                                       [:field
+                                                                        (mt/id :categories :id)
+                                                                        {:source-field (mt/id :venues :category_id)}]]}]))
+                 (m/dissoc-in [:data :native_form :params])
+                 (m/dissoc-in [:data :results_metadata :checksum])))))))