Make normalization aware of share

src/metabase/mbql/normalize.clj

@@ -296,6 +296,47 @@
     [:field-id field]
     field))
 
+(defn- canonicalize-filter [filter-clause]
+  (mbql.u/replace filter-clause
+    seq? (recur (vec &match))
+
+    ;; for `and`/`or`/`not` compound filters, recurse on the arg(s), then simplify the whole thing
+    [(filter-name :guard #{:and :or :not}) & args]
+    (mbql.u/simplify-compound-filter (vec (cons
+                                           filter-name
+                                           ;; we need to canonicalize any other mbql clauses that might show up in
+                                           ;; args like datetime-field here because simplify-compund-filter validates
+                                           ;; its output :(
+                                           (map (comp canonicalize-mbql-clauses canonicalize-filter)
+                                                args))))
+
+    [(filter-name :guard #{:starts-with :ends-with :contains :does-not-contain}) field arg options]
+    [filter-name (wrap-implicit-field-id field) arg options]
+
+    [(filter-name :guard #{:starts-with :ends-with :contains :does-not-contain}) field arg]
+    [filter-name (wrap-implicit-field-id field) arg]
+
+    [:inside field-1 field-2 & coordinates]
+    (vec
+     (concat
+      [:inside (wrap-implicit-field-id field-1) (wrap-implicit-field-id field-2)]
+      coordinates))
+
+    ;; if you put a `:datetime-field` inside a `:time-interval` we should fix it for you
+    [:time-interval [:datetime-field field _] & args]
+    (recur (apply vector :time-interval field args))
+
+    ;; all the other filter types have an implict field ID for the first arg
+    ;; (e.g. [:= 10 20] gets canonicalized to [:= [:field-id 10] 20]
+    [(filter-name :guard #{:= :!= :< :<= :> :>= :is-null :not-null :between :inside :time-interval}) field & more]
+    (apply vector filter-name (wrap-implicit-field-id field) more)
+
+    ;; don't wrap segment IDs in `:field-id`
+    [:segment _]
+    &match
+    _
+    (throw (IllegalArgumentException. (str (tru "Illegal filter clause: {0}" filter-clause))))))
+
 (defn- canonicalize-aggregation-subclause
   "Remove `:rows` type aggregation (long-since deprecated; simpliy means no aggregation) if present, and wrap
   `:field-ids` where appropriate."
@@ -323,6 +364,9 @@
     [:metric _]
     &match
 
+    [:share pred]
+    [:share (canonicalize-filter pred)]
+
     ;; something with an arg like [:sum [:field-id 41]]
     [ag-type field]
     [ag-type (wrap-implicit-field-id field)]))
@@ -360,47 +404,6 @@
        (map canonicalize-aggregation-subclause)
        (filterv identity)))
 
-(defn- canonicalize-filter [filter-clause]
-  (mbql.u/replace filter-clause
-    seq? (recur (vec &match))
-
-    ;; for `and`/`or`/`not` compound filters, recurse on the arg(s), then simplify the whole thing
-    [(filter-name :guard #{:and :or :not}) & args]
-    (mbql.u/simplify-compound-filter (vec (cons
-                                           filter-name
-                                           ;; we need to canonicalize any other mbql clauses that might show up in
-                                           ;; args like datetime-field here because simplify-compund-filter validates
-                                           ;; its output :(
-                                           (map (comp canonicalize-mbql-clauses canonicalize-filter)
-                                                args))))
-
-    [(filter-name :guard #{:starts-with :ends-with :contains :does-not-contain}) field arg options]
-    [filter-name (wrap-implicit-field-id field) arg options]
-
-    [(filter-name :guard #{:starts-with :ends-with :contains :does-not-contain}) field arg]
-    [filter-name (wrap-implicit-field-id field) arg]
-
-    [:inside field-1 field-2 & coordinates]
-    (vec
-     (concat
-      [:inside (wrap-implicit-field-id field-1) (wrap-implicit-field-id field-2)]
-      coordinates))
-
-    ;; if you put a `:datetime-field` inside a `:time-interval` we should fix it for you
-    [:time-interval [:datetime-field field _] & args]
-    (recur (apply vector :time-interval field args))
-
-    ;; all the other filter types have an implict field ID for the first arg
-    ;; (e.g. [:= 10 20] gets canonicalized to [:= [:field-id 10] 20]
-    [(filter-name :guard #{:= :!= :< :<= :> :>= :is-null :not-null :between :inside :time-interval}) field & more]
-    (apply vector filter-name (wrap-implicit-field-id field) more)
-
-    ;; don't wrap segment IDs in `:field-id`
-    [:segment _]
-    &match
-    _
-    (throw (IllegalArgumentException. (str (tru "Illegal filter clause: {0}" filter-clause))))))
-
 (defn- canonicalize-order-by
   "Make sure order by clauses like `[:asc 10]` get `:field-id` added where appropriate, e.g. `[:asc [:field-id 10]]`"
   [clauses]

test/metabase/query_processor_test/share_test.clj

@@ -17,6 +17,15 @@
        ffirst
        double))
 
+;; Test normalization
+(datasets/expect-with-drivers (non-timeseries-drivers-with-feature :basic-aggregations)
+  0.94
+  (->> {:aggregation [["share" ["<" ["field-id" (data/id :venues :price)] 4]]]}
+       (data/run-mbql-query venues)
+       rows
+       ffirst
+       double))
+
 (datasets/expect-with-drivers (non-timeseries-drivers-with-feature :basic-aggregations)
   0.17
   (->> {:aggregation [[:share [:and [:< [:field-id (data/id :venues :price)] 4]