Fix expression aggregations with number literals inside agg [ci drivers]

src/metabase/query_processor/expand.clj

@@ -51,7 +51,7 @@
   [id :- su/IntGreaterThanZero]
   (i/map->FieldPlaceholder {:field-id id}))
 
-(s/defn ^:private ^:always-validate field :- i/AnyField
+(s/defn ^:private ^:always-validate field :- i/AnyFieldOrExpression
   "Generic reference to a `Field`. F can be an integer Field ID, or various other forms like `fk->` or `aggregation`."
   [f]
   (if (integer? f)
@@ -116,7 +116,13 @@
 
 (defn- field-or-expression [f]
   (if (instance? Expression f)
-    (update f :args (partial map field-or-expression)) ; recursively call field-or-expression on all the args of the expression
+    ;; recursively call field-or-expression on all the args inside the expression unless they're numbers
+    ;; plain numbers are always assumed to be numeric literals here; you must use MBQL '98 `:field-id` syntax to refer to Fields inside an expression <3
+    (update f :args #(for [arg %]
+                       (if (number? arg)
+                         arg
+                         (field-or-expression arg))))
+    ;; otherwise if it's not an Expression it's a a
     (field f)))
 
 (s/defn ^:private ^:always-validate ag-with-field :- i/Aggregation [ag-type f]

src/metabase/query_processor/interface.clj

@@ -185,10 +185,10 @@
                          args     :- [(s/cond-pre (s/recursive #'RValue)
                                                   (s/recursive #'Aggregation))]])
 
-(def AnyField
+(def AnyFieldOrExpression
   "Schema for a `FieldPlaceholder`, `AgRef`, or `Expression`."
   (s/named (s/cond-pre ExpressionRef Expression FieldPlaceholderOrAgRef)
-           "Valid field, ag field reference, or expression reference."))
+           "Valid field, ag field reference, expression, or expression reference."))
 
 
 (def LiteralDatetimeString
@@ -302,7 +302,7 @@
 
 (def OrderBy
   "Schema for top-level `order-by` clause in an MBQL query."
-  (s/named {:field     AnyField
+  (s/named {:field     AnyFieldOrExpression
             :direction OrderByDirection}
            "Valid order-by subclause"))
 
@@ -317,7 +317,7 @@
   "Schema for an MBQL query."
   {(s/optional-key :aggregation) [Aggregation]
    (s/optional-key :breakout)    [FieldPlaceholderOrExpressionRef]
-   (s/optional-key :fields)      [AnyField]
+   (s/optional-key :fields)      [AnyFieldOrExpression]
    (s/optional-key :filter)      Filter
    (s/optional-key :limit)       su/IntGreaterThanZero
    (s/optional-key :order-by)    [OrderBy]

test/metabase/driver/druid_test.clj

@@ -192,3 +192,13 @@
   (druid-query-returning-rows
     (ql/aggregation (ql/+ 1 (ql/count)))
     (ql/breakout $venue_price)))
+
+;; aggregation with math inside the aggregation :scream_cat:
+(expect-with-engine :druid
+  [["1"  442.0]
+   ["2" 1845.0]
+   ["3"  460.0]
+   ["4"  245.0]]
+  (druid-query-returning-rows
+    (ql/aggregation (ql/sum (ql/+ $venue_price 1)))
+    (ql/breakout $venue_price)))

test/metabase/query_processor_test/expression_aggregations_test.clj

@@ -145,3 +145,14 @@
     (rows (data/run-query venues
             (ql/aggregation (ql/+ 1 (ql/count)))
             (ql/breakout $price)))))
+
+;; aggregation with math inside the aggregation :scream_cat:
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+  [[1  44]
+   [2 177]
+   [3  52]
+   [4  30]]
+  (format-rows-by [int int]
+    (rows (data/run-query venues
+            (ql/aggregation (ql/sum (ql/+ $price 1)))
+            (ql/breakout $price)))))