[MLv2] filter-clause and aggregation-clause should return a raw MBQL clause,...

[MLv2] filter-clause and aggregation-clause should return a raw MBQL clause, not external op (#31748)

Fixes #
* [MLv2] filter-clause and aggregation-clause should return a raw MBQL clause, not external op

* Add tests for clause fns

* Fix linter

src/metabase/lib/aggregation.cljc

@@ -309,28 +309,21 @@
                   (map #(assoc % :lib/type :mbql.aggregation/operator)))
             lib.schema.aggregation/aggregation-operators)))))
 
-;;; TODO -- this should probably return a plain aggregation clause rather than an external op form; people can convert
-;;; to external op as needed using [[metabase.lib.common/external-op]]. See
-;;; https://metaboat.slack.com/archives/C04CYTEL9N2/p1686941960566759
-(mu/defn aggregation-clause :- ::lib.schema.common/external-op
+(mu/defn aggregation-clause :- ::lib.schema.aggregation/aggregation
   "Returns a standalone aggregation clause for an `aggregation-operator` and
   a `column`.
   For aggregations requiring an argument `column` is mandatory, otherwise
   it is optional."
   ([aggregation-operator :- ::lib.schema.aggregation/operator]
    (if-not (:requires-column? aggregation-operator)
-     {:lib/type :lib/external-op
-      :operator (:short aggregation-operator)
-      :args     []}
+     (lib.options/ensure-uuid [(:short aggregation-operator) {}])
      (throw (ex-info (lib.util/format "aggregation operator %s requires an argument"
                                       (:short aggregation-operator))
                      {:aggregation-operator aggregation-operator}))))
 
   ([aggregation-operator :- ::lib.schema.aggregation/operator
     column]
-   {:lib/type :lib/external-op
-    :operator (:short aggregation-operator)
-    :args     [column]}))
+   (lib.options/ensure-uuid [(:short aggregation-operator) {} (lib.common/->op-arg column)])))
 
 (def ^:private SelectedOperatorWithColumns
   [:merge

src/metabase/lib/filter.cljc

@@ -8,8 +8,9 @@
    [metabase.lib.hierarchy :as lib.hierarchy]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.metadata.calculation :as lib.metadata.calculation]
+   [metabase.lib.options :as lib.options]
    [metabase.lib.schema :as lib.schema]
-   [metabase.lib.schema.common :as lib.schema.common]
+   [metabase.lib.schema.expression :as lib.schema.expression]
    [metabase.lib.schema.filter :as lib.schema.filter]
    [metabase.lib.temporal-bucket :as lib.temporal-bucket]
    [metabase.lib.types.isa :as lib.types.isa]
@@ -316,15 +317,11 @@
              (keep with-operators)
              columns)))))
 
-;;; TODO -- this should return a plain MBQL clause rather than an external op form; people can convert to external op
-;;; as needed using [[metabase.lib.common/external-op]]. See
-;;; https://metaboat.slack.com/archives/C04CYTEL9N2/p1686941960566759
-(mu/defn filter-clause :- ::lib.schema.common/external-op
+(mu/defn filter-clause :- ::lib.schema.expression/boolean
   "Returns a standalone filter clause for a `filter-operator`,
   a `column`, and arguments."
   [filter-operator :- ::lib.schema.filter/operator
    column :- lib.metadata/ColumnMetadata
    & args]
-  {:lib/type :lib/external-op
-   :operator (:short filter-operator)
-   :args (into [column] args)})
+  (lib.options/ensure-uuid (into [(:short filter-operator) {} (lib.common/->op-arg column)]
+                                 (map lib.common/->op-arg args))))

src/metabase/lib/join.cljc

@@ -582,5 +582,4 @@
     joinable]
    (when-let [pk-col (pk-column query stage-number joinable)]
      (when-let [fk-col (fk-column-for query stage-number pk-col)]
-       (lib.common/->op-arg
-        (lib.filter/filter-clause (equals-join-condition-operator-definition) fk-col pk-col))))))
+       (lib.filter/filter-clause (equals-join-condition-operator-definition) fk-col pk-col)))))

test/metabase/lib/aggregation_test.cljc

@@ -301,6 +301,22 @@
     :semantic-type  :type/Name
     :lib/source     :source/implicitly-joinable}])
 
+(deftest ^:parallel aggregation-clause-test
+  (let [query (lib/query meta/metadata-provider (meta/table-metadata :venues))
+        aggregation-operators (lib/available-aggregation-operators query)
+        count-op (first aggregation-operators)
+        sum-op (second aggregation-operators)]
+    (is (=? [:sum {} [:field {} (meta/id :venues :price)]]
+            (lib/aggregation-clause sum-op (meta/field-metadata :venues :price))))
+    (is (=? [:count {}]
+            (lib/aggregation-clause count-op)))
+    (is (=? [:count {} [:field {} (meta/id :venues :price)]]
+            (lib/aggregation-clause count-op (meta/field-metadata :venues :price))))
+    (is (thrown-with-msg?
+          #?(:clj Exception :cljs :default)
+          #"aggregation operator :sum requires an argument"
+          (lib/aggregation-clause sum-op)))))
+
 (deftest ^:parallel aggregation-operator-test
   (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
                   (lib/expression "double-price" (lib/* (meta/field-metadata :venues :price) 2))

test/metabase/lib/filter_test.cljc

@@ -276,16 +276,22 @@
     (is (=? [[:= {} [:field {} (meta/id :users :id)] 515]]
             (-> query
                 (lib/filter new-filter)
-                lib/filters)))))
+                lib/filters))))
+  (testing "standalone clause"
+    (let [query (lib/query meta/metadata-provider (meta/table-metadata :venues))
+          [id-col] (lib/filterable-columns query)
+          [eq-op] (lib/filterable-column-operators id-col)]
+      (is (=? [:= {} [:field {} (meta/id :venues :id)] 123]
+              (lib/filter-clause eq-op id-col 123))))))
 
 (deftest ^:parallel replace-filter-clause-test
   (testing "Make sure we are able to replace a filter clause using the lib functions for manipulating filters."
     (let [query           (lib/query meta/metadata-provider (meta/table-metadata :users))
           [first-col]     (lib/filterable-columns query)
           query           (lib/filter query (lib/filter-clause
-                                             (first (lib/filterable-column-operators first-col))
-                                             first-col
-                                             515))
+                                              (first (lib/filterable-column-operators first-col))
+                                              first-col
+                                              515))
           [filter-clause] (lib/filters query)
           external-op     (lib/external-op filter-clause)]
       (is (=? {:stages [{:filters [[:= {} [:field {} (meta/id :users :id)] 515]]}]}