From 1e286322e1ee516f328f5a6a2f2eb64a3a93ec20 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Thu, 1 Jun 2023 21:10:46 -0600
Subject: [PATCH] [MLv2] Change expressions map into a vector of expressions
(#31272)
* [MLv2] Change expressions map into a vector of expressions
* Fix tests
---
src/metabase/lib/convert.cljc | 25 ++++-
src/metabase/lib/dev.cljc | 3 +-
src/metabase/lib/expression.cljc | 19 ++--
src/metabase/lib/remove_replace.cljc | 32 ++----
src/metabase/lib/schema.cljc | 2 +-
src/metabase/lib/schema/expression.cljc | 6 +-
src/metabase/lib/util.cljc | 35 ++++--
test/metabase/lib/aggregation_test.cljc | 15 ++-
test/metabase/lib/convert_test.cljc | 7 +-
test/metabase/lib/expression_test.cljc | 102 ++++++++++--------
test/metabase/lib/remove_replace_test.cljc | 22 ++--
.../schema/expression/conditional_test.cljc | 8 +-
test/metabase/lib/schema_test.cljc | 13 +--
test/metabase/lib/stage_test.cljc | 12 +--
14 files changed, 176 insertions(+), 125 deletions(-)
diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc
index 8db742f516a..93e31780ee6 100644
--- a/src/metabase/lib/convert.cljc
+++ b/src/metabase/lib/convert.cljc
@@ -102,7 +102,14 @@
(defmethod ->pMBQL :mbql.stage/mbql
[stage]
- (let [aggregations (->pMBQL (:aggregation stage))]
+ (let [aggregations (->pMBQL (:aggregation stage))
+ expressions (->> stage
+ :expressions
+ (mapv (fn [[k v]]
+ (-> v
+ ->pMBQL
+ (lib.util/named-expression-clause k))))
+ not-empty)]
(binding [*legacy-index->pMBQL-uuid* (into {}
(map-indexed (fn [idx [_tag {ag-uuid :lib/uuid}]]
[idx ag-uuid]))
@@ -112,8 +119,8 @@
(if-not (get stage k)
stage
(update stage k ->pMBQL)))
- (m/assoc-some stage :aggregation aggregations)
- (disj stage-keys :aggregation)))))
+ (m/assoc-some stage :aggregation aggregations :expressions expressions)
+ (disj stage-keys :aggregation :expressions)))))
(defmethod ->pMBQL :mbql/join
[join]
@@ -370,8 +377,18 @@
(-> stage
disqualify
(m/update-existing :aggregation #(mapv aggregation->legacy-MBQL %))
+ (m/update-existing :expressions (fn [expressions]
+ (into {}
+ (for [expression expressions
+ :let [legacy-clause (->legacy-MBQL expression)]]
+ [(lib.util/expression-name expression)
+ ;; We wrap literals in :value ->pMBQL
+ ;; so unwrap this direction
+ (if (= :value (first legacy-clause))
+ (second legacy-clause)
+ legacy-clause)]))))
(update-list->legacy-boolean-expression :filters :filter))
- (disj stage-keys :aggregation :filters))))
+ (disj stage-keys :aggregation :filters :expressions))))
(defmethod ->legacy-MBQL :mbql.stage/native [stage]
(-> stage
diff --git a/src/metabase/lib/dev.cljc b/src/metabase/lib/dev.cljc
index e4c0f726f5c..ccad959fef4 100644
--- a/src/metabase/lib/dev.cljc
+++ b/src/metabase/lib/dev.cljc
@@ -69,7 +69,8 @@
(fn [query stage-number]
(case expression-or-aggregation
:expression
- (if (contains? (:expressions (lib.util/query-stage query stage-number)) index-or-name)
+ (if (some (comp #{index-or-name} lib.util/expression-name)
+ (:expressions (lib.util/query-stage query stage-number)))
(lib.options/ensure-uuid [:expression {} index-or-name])
(throw (ex-info (str "Undefined expression " index-or-name)
{:expression-name index-or-name
diff --git a/src/metabase/lib/expression.cljc b/src/metabase/lib/expression.cljc
index 7db77dde040..13e369dc947 100644
--- a/src/metabase/lib/expression.cljc
+++ b/src/metabase/lib/expression.cljc
@@ -4,6 +4,7 @@
[+ - * / case coalesce abs time concat replace])
(:require
[clojure.string :as str]
+ [medley.core :as m]
[metabase.lib.common :as lib.common]
[metabase.lib.hierarchy :as lib.hierarchy]
[metabase.lib.metadata :as lib.metadata]
@@ -36,7 +37,8 @@
stage-number :- :int
expression-name :- ::lib.schema.common/non-blank-string]
(let [stage (lib.util/query-stage query stage-number)]
- (or (get-in stage [:expressions expression-name])
+ (or (m/find-first (comp #{expression-name} lib.util/expression-name)
+ (:expressions stage))
(throw (ex-info (i18n/tru "No expression named {0}" (pr-str expression-name))
{:expression-name expression-name
:query query
@@ -204,7 +206,9 @@
(lib.util/update-query-stage
query stage-number
update :expressions
- assoc expression-name (lib.common/->op-arg query stage-number an-expression-clause)))))
+ (fnil conj [])
+ (-> (lib.common/->op-arg query stage-number an-expression-clause)
+ (lib.util/named-expression-clause expression-name))))))
(lib.common/defop + [x y & more])
(lib.common/defop - [x y & more])
@@ -257,11 +261,12 @@
([query :- ::lib.schema/query
stage-number :- :int]
(some->> (not-empty (:expressions (lib.util/query-stage query stage-number)))
- (mapv (fn [[expression-name expression-definition]]
- (-> (lib.metadata.calculation/metadata query stage-number expression-definition)
- (assoc :lib/source :source/expressions
- :name expression-name
- :display-name expression-name)))))))
+ (mapv (fn [expression-definition]
+ (let [expression-name (lib.util/expression-name expression-definition)]
+ (-> (lib.metadata.calculation/metadata query stage-number expression-definition)
+ (assoc :lib/source :source/expressions
+ :name expression-name
+ :display-name expression-name))))))))
(mu/defn expressions :- [:maybe ::lib.schema.expression/expressions]
"Get the expressions map from a given stage of a `query`."
diff --git a/src/metabase/lib/remove_replace.cljc b/src/metabase/lib/remove_replace.cljc
index c4023a800c6..cd93619272f 100644
--- a/src/metabase/lib/remove_replace.cljc
+++ b/src/metabase/lib/remove_replace.cljc
@@ -2,7 +2,6 @@
(:require
[medley.core :as m]
[metabase.lib.common :as lib.common]
- [metabase.lib.expression :as lib.expression]
[metabase.lib.join :as lib.join]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.util :as lib.util]
@@ -15,11 +14,8 @@
join-condition-paths (for [idx join-indices]
[:joins idx :conditions])
join-field-paths (for [idx join-indices]
- [:joins idx :fields])
- expr-paths (for [[expr-name] (lib.expression/expressions query stage-number)]
- [:expressions expr-name])]
- (concat [[:order-by] [:breakout] [:filters] [:fields] [:aggregation]]
- expr-paths
+ [:joins idx :fields])]
+ (concat [[:order-by] [:breakout] [:filters] [:fields] [:aggregation] [:expressions]]
join-field-paths
join-condition-paths)))
@@ -62,17 +58,17 @@
(defn- remove-replace-location
[query stage-number unmodified-query-for-stage location target-clause remove-replace-fn]
(let [result (lib.util/update-query-stage query stage-number
- remove-replace-fn location target-clause)
+ remove-replace-fn location target-clause)
target-uuid (lib.util/clause-uuid target-clause)]
(if (not= query result)
(mbql.match/match-one location
- [:expressions expr-name]
+ [:expressions]
(-> result
(remove-local-references
stage-number
unmodified-query-for-stage
:expression
- expr-name)
+ (lib.util/expression-name target-clause))
(remove-stage-references stage-number unmodified-query-for-stage target-uuid))
[:aggregation]
@@ -99,13 +95,10 @@
(let [stage (lib.util/query-stage query stage-number)
to-remove (mapcat
(fn [location]
- (when-let [sub-loc (get-in stage location)]
- (let [clauses (if (lib.util/clause-uuid sub-loc)
- [sub-loc]
- sub-loc)]
- (->> clauses
- (keep #(mbql.match/match-one sub-loc
- [target-op _ target-ref-id] [location %]))))))
+ (when-let [clauses (get-in stage location)]
+ (->> clauses
+ (keep #(mbql.match/match-one %
+ [target-op _ target-ref-id] [location %])))))
(stage-paths query stage-number))]
(reduce
(fn [query [location target-clause]]
@@ -133,11 +126,8 @@
stage (lib.util/query-stage query stage-number)
location (m/find-first
(fn [possible-location]
- (when-let [sub-loc (get-in stage possible-location)]
- (let [clauses (if (lib.util/clause? sub-loc)
- [sub-loc]
- sub-loc)
- target-uuid (lib.util/clause-uuid target-clause)]
+ (when-let [clauses (get-in stage possible-location)]
+ (let [target-uuid (lib.util/clause-uuid target-clause)]
(when (some (comp #{target-uuid} :lib/uuid second) clauses)
possible-location))))
(stage-paths query stage-number))
diff --git a/src/metabase/lib/schema.cljc b/src/metabase/lib/schema.cljc
index 2eaeb5816cb..5facbc83dab 100644
--- a/src/metabase/lib/schema.cljc
+++ b/src/metabase/lib/schema.cljc
@@ -52,7 +52,7 @@
[:ref ::id/table-card-id-string]])
(defn- expression-ref-error-for-stage [stage]
- (let [expression-names (set (keys (:expressions stage)))]
+ (let [expression-names (into #{} (map (comp :lib/expression-name second)) (:expressions stage))]
(mbql.match/match-one (dissoc stage :joins :lib/stage-metadata)
[:expression _opts (expression-name :guard (complement expression-names))]
(str "Invalid :expression reference: no expression named " (pr-str expression-name)))))
diff --git a/src/metabase/lib/schema/expression.cljc b/src/metabase/lib/schema/expression.cljc
index ef150881173..ae1075f4862 100644
--- a/src/metabase/lib/schema/expression.cljc
+++ b/src/metabase/lib/schema/expression.cljc
@@ -153,7 +153,5 @@
;;; the `:expressions` definition map as found as a top-level key in an MBQL stage
(mr/def ::expressions
- [:map-of
- {:min 1, :error/message ":expressions definition map of expression name -> expression"}
- ::common/non-blank-string
- ::expression])
+ [:sequential {:min 1} [:and [:ref ::expression]
+ [:cat :any [:map [:lib/expression-name :string]] [:* :any]]]])
diff --git a/src/metabase/lib/util.cljc b/src/metabase/lib/util.cljc
index 88d1c7d3b65..1a3ae79721d 100644
--- a/src/metabase/lib/util.cljc
+++ b/src/metabase/lib/util.cljc
@@ -14,6 +14,7 @@
[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.id :as lib.schema.id]
[metabase.mbql.util :as mbql.u]
[metabase.shared.util.i18n :as i18n]
@@ -49,18 +50,36 @@
(when (clause? clause)
(get-in clause [1 :lib/uuid])))
+(defn expression-name
+ "Returns the :lib/expression-name of `clause`. Returns nil if `clause` is not a clause."
+ [clause]
+ (when (clause? clause)
+ (get-in clause [1 :lib/expression-name])))
+
+(defn named-expression-clause
+ "Top level expressions must be clauses with :lib/expression-name, so if we get a literal, wrap it in :value."
+ [clause a-name]
+ (assoc-in
+ (if (clause? clause)
+ clause
+ [:value {:lib/uuid (str (random-uuid))
+ :effective-type (lib.schema.expression/type-of clause)}
+ clause])
+ [1 :lib/expression-name] a-name))
+
(defn replace-clause
"Replace the `target-clause` in `stage` `location` with `new-clause`.
If a clause has :lib/uuid equal to the `target-clause` it is swapped with `new-clause`.
If `location` contains no clause with `target-clause` no replacement happens."
[stage location target-clause new-clause]
{:pre [(clause? target-clause)]}
- (m/update-existing-in
- stage
- location
- (fn [clause-or-clauses]
- (if (= :expressions (first location))
- new-clause
+ (let [new-clause (if (= :expressions (first location))
+ (named-expression-clause new-clause (expression-name target-clause))
+ new-clause)]
+ (m/update-existing-in
+ stage
+ location
+ (fn [clause-or-clauses]
(->> (for [clause clause-or-clauses]
(if (= (clause-uuid clause) (clause-uuid target-clause))
new-clause
@@ -77,9 +96,7 @@
(if-let [target (get-in stage location)]
(let [target-uuid (clause-uuid target-clause)
[first-loc last-loc] [(first location) (last location)]
- result (if (= :expressions first-loc)
- nil
- (into [] (remove (comp #{target-uuid} clause-uuid)) target))]
+ result (into [] (remove (comp #{target-uuid} clause-uuid)) target)]
(cond
(seq result)
(assoc-in stage location result)
diff --git a/test/metabase/lib/aggregation_test.cljc b/test/metabase/lib/aggregation_test.cljc
index 68a1ff4426d..37b443f6a01 100644
--- a/test/metabase/lib/aggregation_test.cljc
+++ b/test/metabase/lib/aggregation_test.cljc
@@ -168,10 +168,11 @@
:display-name "Sum of double-price"}
(col-info-for-aggregation-clause
(lib.tu/venues-query-with-last-stage
- {:expressions {"double-price" [:*
- {:lib/uuid (str (random-uuid))}
- (lib.tu/field-clause :venues :price {:base-type :type/Integer})
- 2]}})
+ {:expressions [[:*
+ {:lib/uuid (str (random-uuid))
+ :lib/expression-name "double-price"}
+ (lib.tu/field-clause :venues :price {:base-type :type/Integer})
+ 2]]})
[:sum
{:lib/uuid (str (random-uuid))}
[:expression {:base-type :type/Integer, :lib/uuid (str (random-uuid))} "double-price"]])))))
@@ -400,10 +401,8 @@
[{:lib/type :mbql.stage/mbql
:source-table int?
:expressions
- {"double-price"
- [:* {} [:field {:base-type :type/Integer, :effective-type :type/Integer} int?] 2]
- "budget?"
- [:< {} [:field {:base-type :type/Integer, :effective-type :type/Integer} int?] 2]}
+ [[:* {:lib/expression-name "double-price"} [:field {:base-type :type/Integer, :effective-type :type/Integer} int?] 2]
+ [:< {:lib/expression-name "budget?"} [:field {:base-type :type/Integer, :effective-type :type/Integer} int?] 2]]
:aggregation
[[:sum {} [:expression {} "double-price"]]
[:count {}]
diff --git a/test/metabase/lib/convert_test.cljc b/test/metabase/lib/convert_test.cljc
index b5869dc37e3..e5cabac8ef9 100644
--- a/test/metabase/lib/convert_test.cljc
+++ b/test/metabase/lib/convert_test.cljc
@@ -268,7 +268,12 @@
:parameters [{:target [:dimension [:field 16 {:source-field 5}]],
:type :category,
:value [:param-value]}],
- :type :native}))
+ :type :native}
+
+ {:database 1
+ :type :query
+ :query {:source-table 224
+ :expressions {"a" 1}}}))
(deftest ^:parallel round-trip-options-test
(testing "Round-tripping (p)MBQL caluses with options (#30280)"
diff --git a/test/metabase/lib/expression_test.cljc b/test/metabase/lib/expression_test.cljc
index 5001464cf2d..850bf851703 100644
--- a/test/metabase/lib/expression_test.cljc
+++ b/test/metabase/lib/expression_test.cljc
@@ -1,5 +1,6 @@
(ns metabase.lib.expression-test
(:require
+ #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
[clojure.test :refer [deftest is testing]]
[malli.core :as mc]
[metabase.lib.core :as lib]
@@ -8,8 +9,7 @@
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.expression :as lib.schema.expression]
[metabase.lib.test-metadata :as meta]
- [metabase.lib.test-util :as lib.tu]
- #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))))
+ [metabase.lib.test-util :as lib.tu]))
(comment lib/keep-me)
@@ -20,9 +20,9 @@
:database (meta/id)
:stages [{:lib/type :mbql.stage/mbql
:source-table (meta/id :venues)
- :expressions {"myadd" [:+ {:lib/uuid string?}
- 1
- [:field {:base-type :type/Integer, :lib/uuid string?} (meta/id :venues :category-id)]]}}]}
+ :expressions [[:+ {:lib/uuid string? :lib/expression-name "myadd"}
+ 1
+ [:field {:base-type :type/Integer, :lib/uuid string?} (meta/id :venues :category-id)]]]}]}
(-> (lib/query-for-table-name meta/metadata-provider "VENUES")
(lib/expression "myadd" (lib/+ 1 (lib/field "VENUES" "CATEGORY_ID")))
(dissoc :lib/metadata)))))
@@ -89,20 +89,19 @@
:display-name "double-price"
:lib/source :source/expressions}
(lib.metadata.calculation/metadata
- (lib.tu/venues-query-with-last-stage
- {:expressions {"double-price" [:*
- {:lib/uuid (str (random-uuid))}
- (lib.tu/field-clause :venues :price {:base-type :type/Integer})
- 2]}})
- -1
- [:expression {:lib/uuid (str (random-uuid))} "double-price"]))))
+ (-> lib.tu/venues-query
+ (lib/expression "double-price"
+ (lib/* (lib.tu/field-clause :venues :price {:base-type :type/Integer}) 2)))
+ -1
+ [:expression {:lib/uuid (str (random-uuid))} "double-price"]))))
(deftest ^:parallel expression-references-in-fields-clause-test
(let [query (lib.tu/venues-query-with-last-stage
- {:expressions {"prev_month" [:+
- {:lib/uuid (str (random-uuid))}
- (lib.tu/field-clause :users :last-login)
- [:interval {:lib/uuid (str (random-uuid))} -1 :month]]}
+ {:expressions [[:+
+ {:lib/uuid (str (random-uuid))
+ :lib/expression-name "prev_month"}
+ (lib.tu/field-clause :users :last-login)
+ [:interval {:lib/uuid (str (random-uuid))} -1 :month]]]
:fields [[:expression {:base-type :type/DateTime, :lib/uuid (str (random-uuid))} "prev_month"]]})]
(is (=? [{:name "prev_month"
:display-name "prev_month"
@@ -122,12 +121,11 @@
(lib.metadata.calculation/display-name lib.tu/venues-query -1 clause)))))
(deftest ^:parallel expression-reference-names-test
- (let [query (assoc-in lib.tu/venues-query
- [:stages 0 :expressions "double-price"]
- [:*
- {:lib/uuid (str (random-uuid))}
- (lib.tu/field-clause :venues :price {:base-type :type/Integer})
- 2])
+ (let [query (-> lib.tu/venues-query
+ (lib/expression "double-price"
+ (lib/*
+ (lib.tu/field-clause :venues :price {:base-type :type/Integer})
+ 2)))
expr [:sum
{:lib/uuid (str (random-uuid))}
[:expression {:lib/uuid (str (random-uuid))} "double-price"]]]
@@ -144,17 +142,12 @@
(lib.metadata.calculation/display-name lib.tu/venues-query -1 clause)))))
(defn- infer-first
- ([expr]
- (infer-first expr nil))
-
- ([expr last-stage]
- (lib.metadata.calculation/metadata
- (lib.tu/venues-query-with-last-stage
- (merge
- {:expressions {"expr" expr}}
- last-stage))
+ [expr]
+ (lib.metadata.calculation/metadata
+ (-> lib.tu/venues-query
+ (lib/expression "expr" expr))
-1
- [:expression {:lib/uuid (str (random-uuid))} "expr"])))
+ [:expression {:lib/uuid (str (random-uuid))} "expr"]))
(deftest ^:parallel infer-coalesce-test
(testing "Coalesce"
@@ -198,25 +191,26 @@
:display-name "last-login-plus-2"
:lib/source :source/expressions}
(lib.metadata.calculation/metadata
- (lib.tu/venues-query-with-last-stage
- {:expressions {"last-login-plus-2" [:datetime-add
- {:lib/uuid (str (random-uuid))}
- (lib.tu/field-clause :users :last-login {:base-type :type/DateTime})
- 2
- :hour]}})
+ (-> lib.tu/venues-query
+ (lib/expression "last-login-plus-2"
+ [:datetime-add
+ {:lib/uuid (str (random-uuid))}
+ (lib.tu/field-clause :users :last-login {:base-type :type/DateTime})
+ 2
+ :hour]))
-1
[:expression {:lib/uuid (str (random-uuid))} "last-login-plus-2"]))))
(deftest ^:parallel col-info-for-expression-error-message-test
(testing "if there is no matching expression it should give a meaningful error message"
(is (thrown-with-msg?
- #?(:clj Throwable :cljs js/Error)
- #"No expression named \"double-price\""
- (lib.metadata.calculation/metadata
- (lib.tu/venues-query-with-last-stage
- {:expressions {"one-hundred" 100}})
- -1
- [:expression {:lib/uuid (str (random-uuid))} "double-price"])))))
+ #?(:clj Throwable :cljs js/Error)
+ #"No expression named \"double-price\""
+ (lib.metadata.calculation/metadata
+ (-> lib.tu/venues-query
+ (lib/expression "one-hundred" (lib/+ 100 0)))
+ -1
+ [:expression {:lib/uuid (str (random-uuid))} "double-price"])))))
(deftest ^:parallel arithmetic-expression-type-of-test
(testing "Make sure we can calculate correct type information for arithmetic expression"
@@ -262,3 +256,21 @@
(ex-message ex)))
(is (= {:expression-name "ID"}
(ex-data ex))))))
+
+(deftest ^:parallel literal-expression-test
+ (is (=? [{:lib/type :metadata/field,
+ :base-type :type/Integer,
+ :name "expr",
+ :display-name "expr",
+ :lib/source :source/expressions}]
+ (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/expression "expr" 100)
+ (lib/expressions-metadata))))
+ (is (=? [[:value {:lib/expression-name "expr" :effective-type :type/Integer} 100]]
+ (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/expression "expr" 100)
+ (lib/expressions))))
+ (is (=? [[:value {:lib/expression-name "expr" :effective-type :type/Text} "value"]]
+ (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
+ (lib/expression "expr" "value")
+ (lib/expressions)))))
diff --git a/test/metabase/lib/remove_replace_test.cljc b/test/metabase/lib/remove_replace_test.cljc
index e0163fb36fe..ae426057aeb 100644
--- a/test/metabase/lib/remove_replace_test.cljc
+++ b/test/metabase/lib/remove_replace_test.cljc
@@ -197,7 +197,7 @@
(let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
(lib/expression "a" (lib/field "VENUES" "ID"))
(lib/expression "b" (lib/field "VENUES" "PRICE")))
- {expr-a "a" expr-b "b" :as expressions} (lib/expressions query)]
+ [expr-a expr-b :as expressions] (lib/expressions query)]
(is (= 2 (count expressions)))
(is (= 1 (-> query
(lib/remove-clause expr-a)
@@ -208,7 +208,7 @@
(lib/remove-clause expr-b)
(lib/expressions))))
(testing "removing with dependent should cascade"
- (is (=? {:stages [{:expressions {"b" expr-b} :order-by (symbol "nil #_\"key is not present.\"")}
+ (is (=? {:stages [{:expressions [expr-b] :order-by (symbol "nil #_\"key is not present.\"")}
(complement :filters)]}
(-> query
(lib/order-by (lib.dev/ref-lookup :expression "a"))
@@ -367,20 +367,21 @@
(let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
(lib/expression "a" (lib/field (meta/id :venues :id)))
(lib/expression "b" (lib/field (meta/id :venues :name))))
- {expr-a "a" expr-b "b" :as expressions} (lib/expressions query)
+ [expr-a expr-b :as expressions] (lib/expressions query)
replaced (-> query
(lib/replace-clause expr-a (lib/field (meta/id :venues :price))))
- {_repl-expr-a "a" repl-expr-b "b" :as replaced-expressions} (lib/expressions replaced)]
+ [_repl-expr-a repl-expr-b :as replaced-expressions] (lib/expressions replaced)]
(is (= 2 (count expressions)))
- (is (=? {"a" [:field {} (meta/id :venues :price)]}
+ (is (=? [[:field {:lib/expression-name "a"} (meta/id :venues :price)]
+ expr-b]
replaced-expressions))
(is (not= expressions replaced-expressions))
(is (= 2 (count replaced-expressions)))
(is (= expr-b repl-expr-b))
(testing "replacing with dependent should cascade"
(is (=? {:stages [{:aggregation (symbol "nil #_\"key is not present.\"")
- :expressions {"a" [:field {} (meta/id :venues :price)]
- "b" expr-b}}
+ :expressions [[:field {:lib/expression-name "a"} (meta/id :venues :price)]
+ expr-b]}
(complement :filters)]}
(-> query
(lib/aggregate (lib/sum (lib.dev/ref-lookup :expression "a")))
@@ -388,7 +389,12 @@
(lib/append-stage)
;; TODO Should be able to create a ref with lib/field [#29763]
(lib/filter (lib/= [:field {:lib/uuid (str (random-uuid)) :base-type :type/Integer} "a"] 1))
- (lib/replace-clause 0 expr-a (lib/field "VENUES" "PRICE"))))))))
+ (lib/replace-clause 0 expr-a (lib/field "VENUES" "PRICE"))))))
+ (testing "replace with literal expression"
+ (is (=? {:stages [{:expressions [[:value {:lib/expression-name "a" :effective-type :type/Integer} 999]
+ expr-b]}]}
+ (-> query
+ (lib/replace-clause 0 expr-a 999)))))))
(deftest ^:parallel replace-order-by-breakout-col-test
(testing "issue #30980"
diff --git a/test/metabase/lib/schema/expression/conditional_test.cljc b/test/metabase/lib/schema/expression/conditional_test.cljc
index d660386b376..5f83adf1559 100644
--- a/test/metabase/lib/schema/expression/conditional_test.cljc
+++ b/test/metabase/lib/schema/expression/conditional_test.cljc
@@ -63,13 +63,13 @@
:database (meta/id)
:stages [{:lib/type :mbql.stage/mbql,
:source-table (meta/id :venues)
- :expressions {"expr"
- [:coalesce
- {:lib/uuid "455a9f5e-4996-4df9-82aa-01bc083b2efe"}
+ :expressions [[:coalesce
+ {:lib/uuid "455a9f5e-4996-4df9-82aa-01bc083b2efe"
+ :lib/expression-name "expr"}
[:field
{:base-type :type/Text, :lib/uuid "68443c43-f9de-45e3-9f30-8dfd5fef5af6"}
(meta/id :venues :name)]
- "bar"]}}]})))
+ "bar"]]}]})))
(deftest ^:parallel case-type-of-with-fields-only-test
;; Ideally expression/type-of should have enough information to determine the types of fields.
diff --git a/test/metabase/lib/schema_test.cljc b/test/metabase/lib/schema_test.cljc
index 7a3a189fe53..2cc34511967 100644
--- a/test/metabase/lib/schema_test.cljc
+++ b/test/metabase/lib/schema_test.cljc
@@ -68,7 +68,8 @@
(def ^:private valid-expression
[:+
- {:lib/uuid (str (random-uuid))}
+ {:lib/uuid (str (random-uuid))
+ :lib/expression-name "price + 2"}
[:field
{:lib/uuid (str (random-uuid))}
2]
@@ -79,15 +80,15 @@
(me/humanize (mc/explain ::lib.schema/stage stage)))
{:lib/type :mbql.stage/mbql
:source-table 1
- :expressions {"price + 2" valid-expression}
+ :expressions [valid-expression]
:fields [[:expression {:lib/uuid (str (random-uuid))} "price + 2"]]}
nil
{:lib/type :mbql.stage/mbql
:source-table 1
- :expressions {"price + 1" valid-expression}
- :fields [[:expression {:lib/uuid (str (random-uuid))} "price + 2"]]}
- ["Invalid :expression reference: no expression named \"price + 2\""]
+ :expressions [valid-expression]
+ :fields [[:expression {:lib/uuid (str (random-uuid))} "price + 1"]]}
+ ["Invalid :expression reference: no expression named \"price + 1\""]
{:lib/type :mbql.stage/mbql
:source-table 1
@@ -107,7 +108,7 @@
[:field {:join-alias "Q1", :lib/uuid (str (random-uuid))} 2]]]
:stages [{:lib/type :mbql.stage/mbql
:source-table 3
- :expressions {"price + 2" valid-expression}
+ :expressions [valid-expression]
:order-by [[:asc
{:lib/uuid (str (random-uuid))}
[:expression {:lib/uuid (str (random-uuid))} "price + 2"]]]}
diff --git a/test/metabase/lib/stage_test.cljc b/test/metabase/lib/stage_test.cljc
index e2cd8cf1828..bb19c5846be 100644
--- a/test/metabase/lib/stage_test.cljc
+++ b/test/metabase/lib/stage_test.cljc
@@ -75,8 +75,8 @@
(let [query (-> (lib/query-for-table-name meta/metadata-provider "VENUES")
(lib/expression "ID + 1" (lib/+ (lib/field "VENUES" "ID") 1))
(lib/expression "ID + 2" (lib/+ (lib/field "VENUES" "ID") 2)))]
- (is (=? {:stages [{:expressions {"ID + 1" [:+ {} [:field {} (meta/id :venues :id)] 1]
- "ID + 2" [:+ {} [:field {} (meta/id :venues :id)] 2]}}]}
+ (is (=? {:stages [{:expressions [[:+ {:lib/expression-name "ID + 1"} [:field {} (meta/id :venues :id)] 1]
+ [:+ {:lib/expression-name "ID + 2"} [:field {} (meta/id :venues :id)] 2]]}]}
query))
query))
@@ -108,8 +108,8 @@
[:field {} (meta/id :venues :category-id)]
[:field {:join-alias "Cat"} (meta/id :categories :id)]]]
:fields :all}]
- :expressions {"ID + 1" [:+ {} [:field {} (meta/id :venues :id)] 1]
- "ID + 2" [:+ {} [:field {} (meta/id :venues :id)] 2]}}]}
+ :expressions [[:+ {:lib/expression-name "ID + 1"} [:field {} (meta/id :venues :id)] 1]
+ [:+ {:lib/expression-name "ID + 2"} [:field {} (meta/id :venues :id)] 2]]}]}
query))
(let [metadata (lib.metadata.calculation/metadata query)]
(is (=? [{:id (meta/id :venues :id), :name "ID", :lib/source :source/table-defaults}
@@ -160,8 +160,8 @@
id-plus-1))
(let [query' (-> query
(lib/with-fields [id-plus-1]))]
- (is (=? {:stages [{:expressions {"ID + 1" [:+ {} [:field {} (meta/id :venues :id)] 1]
- "ID + 2" [:+ {} [:field {} (meta/id :venues :id)] 2]}
+ (is (=? {:stages [{:expressions [[:+ {:lib/expression-name "ID + 1"} [:field {} (meta/id :venues :id)] 1]
+ [:+ {:lib/expression-name "ID + 2"} [:field {} (meta/id :venues :id)] 2]]
:fields [[:expression {} "ID + 1"]]}]}
query'))
(testing "If `:fields` is specified, expressions should only come back if they are in `:fields`"
--
GitLab