Druid: allow filtering on Metric columns (#11823) [ci druid] (#12562)

modules/drivers/druid/src/metabase/driver/druid/query_processor.clj

@@ -216,8 +216,9 @@
   (str/replace s #"([%_\\])" "\\\\$1"))
 
 (defn- filter:bound
-  "Numeric `bound` filter, for finding values of `field` that are less than some value-or-field, greater than some value-or-field, or
-  both. Defaults to being `inclusive` (e.g. `<=` instead of `<`) but specify option `inclusive?` to change this."
+  "Numeric `bound` filter, for finding values of `field` that are less than some value-or-field, greater than some
+  value-or-field, or both. Defaults to being `inclusive` (e.g. `<=` instead of `<`) but specify option `inclusive?` to
+  change this."
   [field & {:keys [lower upper inclusive?]
             :or   {inclusive? true}}]
   {:type        :bound
@@ -228,29 +229,18 @@
    :lowerStrict (not inclusive?)
    :upperStrict (not inclusive?)})
 
-(defn- filter-fields-are-dimensions?
-  [fields]
-  (every? (fn [field]
-            (or
-             (not= (dimension-or-metric? field) :metric)
-             (log/warn
-              (u/format-color 'red
-                  (tru "WARNING: Filtering only works on dimensions! ''{0}'' is a metric. Ignoring filter."
-                       (->rvalue field))))))
-          fields))
-
 (defmulti ^:private parse-filter
+  "Parse an MBQL `filter-clause` and generate an appropriate Druid filter map.
+
+    (parse-filter [:= [:field-id 1] 2]) ; -> {:type :selector, :dimension \"venue_price\", :value 2}"
   {:arglists '([filter-clause])}
   ;; dispatch function first checks to make sure this is a valid filter clause, then dispatches off of the clause name
   ;; if it is.
   (fn [[clause-name & args, :as filter-clause]]
     (let [fields (filter (partial mbql.u/is-clause? #{:field-id :datetime-field}) args)]
-      (when (and
-             ;; make sure all Field args are dimensions
-             (filter-fields-are-dimensions? fields)
-             ;; and make sure none of the Fields are datetime Fields
-             ;; We'll handle :timestamp separately. It needs to go in :intervals instead
-             (not-any? (partial mbql.u/is-clause? :datetime-field) fields))
+      ;; and make sure none of the Fields are datetime Fields We'll handle :timestamp separately. It needs to go in
+      ;; :intervals instead
+      (when (not-any? (partial mbql.u/is-clause? :datetime-field) fields)
         clause-name))))
 
 (defmethod parse-filter nil
@@ -445,7 +435,7 @@
   [_ {filter-clause :filter} druid-query]
   (if-not filter-clause
     druid-query
-    (let [filter    (parse-filter    filter-clause)
+    (let [filter    (parse-filter filter-clause)
           intervals (compile-intervals (filter-clause->intervals filter-clause))]
       (cond-> druid-query
         (seq filter)    (assoc-in [:query :filter] filter)

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

@@ -583,53 +583,62 @@
                 :order-by   [[:desc [:aggregation 0]] [:asc $checkins.venue_category_name]]
                 :limit      5}))))))
 
-(deftest filter-by-numeric-column-test
+(deftest numeric-filter-test
   (mt/test-driver :druid
-    (testing "Make sure we can filter by numeric columns (#10935)"
-      (tqpt/with-flattened-dbdef
-        (letfn [(compiled [query]
-                  (-> (qp/query->native query) :query (select-keys [:filter :queryType])))]
-          (testing "scan query"
-            (let [query (mt/mbql-query checkins
-                          {:order-by [[:desc $id]]
-                           :fields   [$id $venue_price $venue_name]
-                           :filter   [:= $venue_price 1]
-                           :limit    5})]
-              (is (= {:filter    {:type :selector, :dimension "venue_price", :value 1}
-                      :queryType :scan}
-                     (compiled query)))
-              (is (= ["931" "1" "Kinaree Thai Bistro"]
-                     (mt/first-row (qp/process-query query))))))
-
-          (testing "topN query"
-            (let [query (mt/mbql-query checkins
-                          {:aggregation [[:count]]
-                           :breakout    [$venue_price]
-                           :filter      [:= $venue_price 1]})]
-              (is (= {:filter    {:type :selector, :dimension "venue_price", :value 1}
-                      :queryType :topN}
-                     (compiled query)))
-              (is (= ["1" 221]
-                     (mt/first-row (qp/process-query query))))))
-
-          (testing "groupBy query"
-            (let [query (mt/mbql-query checkins
-                          {:aggregation [[:aggregation-options [:distinct $checkins.venue_name] {:name "__count_0"}]]
-                           :breakout    [$venue_category_name $user_name]
-                           :order-by    [[:desc [:aggregation 0]] [:asc $checkins.venue_category_name]]
-                           :filter      [:= $venue_price 1]})]
-              (is (= {:filter    {:type :selector, :dimension "venue_price", :value 1}
-                      :queryType :groupBy}
-                     (compiled query)))
-              (is (= ["Mexican" "Conchúr Tihomir" 4]
-                     (mt/first-row (qp/process-query query))))))
-
-          (testing "timeseries query"
-            (let [query (mt/mbql-query checkins
-                          {:aggregation [[:count]]
-                           :filter      [:= $venue_price 1]})]
-              (is (= {:queryType :timeseries
-                      :filter    {:type :selector, :dimension "venue_price", :value 1}}
-                     (compiled query)))
-              (is (= [221]
-                     (mt/first-row (qp/process-query query)))))))))))
+    (testing
+        (tqpt/with-flattened-dbdef
+          (letfn [(compiled [query]
+                    (-> (qp/query->native query) :query (select-keys [:filter :queryType])))]
+            (doseq [[message field] {"Make sure we can filter by numeric columns (#10935)" :venue_price
+                                     "We should be able to filter by Metrics (#11823)"     :count}
+                    :let            [field-clause [:field-id (mt/id :checkins field)]
+                                     field-name   (name field)]]
+              (testing message
+                (testing "scan query"
+                  (let [query (mt/mbql-query checkins
+                                {:fields   [$id $venue_price $venue_name]
+                                 :filter   [:= field-clause 1]
+                                 :order-by [[:desc $id]]
+                                 :limit    5})]
+                    (is (= {:filter    {:type :selector, :dimension field-name, :value 1}
+                            :queryType :scan}
+                           (compiled query)))
+                    (is (= ["931" "1" "Kinaree Thai Bistro"]
+                           (mt/first-row (qp/process-query query))))))
+
+                (testing "topN query"
+                  (let [query (mt/mbql-query checkins
+                                {:aggregation [[:count]]
+                                 :breakout    [$venue_price]
+                                 :filter      [:= field-clause 1]})]
+                    (is (= {:filter    {:type :selector, :dimension field-name, :value 1}
+                            :queryType :topN}
+                           (compiled query)))
+                    (is (= ["1" 221]
+                           (mt/first-row (qp/process-query query))))))
+
+                (testing "groupBy query"
+                  (let [query (mt/mbql-query checkins
+                                {:aggregation [[:aggregation-options [:distinct $checkins.venue_name] {:name "__count_0"}]]
+                                 :breakout    [$venue_category_name $user_name]
+                                 :order-by    [[:desc [:aggregation 0]] [:asc $checkins.venue_category_name]]
+                                 :filter      [:= field-clause 1]})]
+                    (is (= {:filter    {:type :selector, :dimension field-name, :value 1}
+                            :queryType :groupBy}
+                           (compiled query)))
+                    (is (= (case field
+                             :count       ["Bar" "Felipinho Asklepios" 8]
+                             :venue_price ["Mexican" "Conchúr Tihomir" 4])
+                           (mt/first-row (qp/process-query query))))))
+
+                (testing "timeseries query"
+                  (let [query (mt/mbql-query checkins
+                                {:aggregation [[:count]]
+                                 :filter      [:= field-clause 1]})]
+                    (is (= {:queryType :timeseries
+                            :filter    {:type :selector, :dimension field-name, :value 1}}
+                           (compiled query)))
+                    (is (= (case field
+                             :count       [1000]
+                             :venue_price [221])
+                           (mt/first-row (qp/process-query query)))))))))))))