diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc index 87bd19ea34bed824f32dbda22e2d1de4557b8787..99ee4499af13d6675d620c20b2e457eea4f661b4 100644 --- a/src/metabase/lib/aggregation.cljc +++ b/src/metabase/lib/aggregation.cljc @@ -86,8 +86,6 @@ [_query _stage-number _case] "case") -(lib.hierarchy/derive :case ::aggregation) - (lib.hierarchy/derive ::unary-aggregation ::aggregation) (doseq [tag [:avg diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc index 93d6c971a0f77be61869ffa081c6011ac2f4f5e1..8388c45a3c7b0f7726cb64a600f777651a965890 100644 --- a/src/metabase/lib/convert.cljc +++ b/src/metabase/lib/convert.cljc @@ -162,7 +162,7 @@ (let [default (:default options)] (cond-> [:case (dissoc options :default) (mapv ->pMBQL pred-expr-pairs)] :always lib.options/ensure-uuid - default (conj (->pMBQL default))))) + (some? default) (conj (->pMBQL default))))) (defmethod ->pMBQL :expression [[tag value opts]] diff --git a/src/metabase/lib/schema/expression/conditional.cljc b/src/metabase/lib/schema/expression/conditional.cljc index 46a01c022e645b9d1ad28d210d38a0f56338a313..092d0226172378986fe7cd9f8d68d80d2ede6390 100644 --- a/src/metabase/lib/schema/expression/conditional.cljc +++ b/src/metabase/lib/schema/expression/conditional.cljc @@ -55,14 +55,25 @@ [:pred-expr-pairs [:sequential {:min 1} [:ref ::case-subclause]]] [:default [:? [:schema [:ref ::expression/expression]]]]) +(defn- allow-unkown-type + "A hack to allow case and coalesce expressions using fields only. + The real fix would pass in metadata so that the types of fields + can be determined." + [expr-type] + (if (= expr-type ::expression/type.unknown) + :type/* + expr-type)) + (defmethod expression/type-of* :case [[_tag _opts pred-expr-pairs default]] - (reduce - (fn [best-guess [_pred expr]] - (let [expr-type (expression/type-of expr)] - (best-return-type best-guess expr-type))) - default - pred-expr-pairs)) + (-> (reduce + (fn [best-guess [_pred expr]] + (let [expr-type (expression/type-of expr)] + (best-return-type best-guess expr-type))) + (when (some? default) + (expression/type-of default)) + pred-expr-pairs) + allow-unkown-type)) ;;; TODO -- add constraint that these types have to be compatible (mbql-clause/define-tuple-mbql-clause :coalesce @@ -71,4 +82,5 @@ (defmethod expression/type-of* :coalesce [[_tag _opts expr null-value]] - (best-return-type (expression/type-of expr) (expression/type-of null-value))) + (-> (best-return-type (expression/type-of expr) (expression/type-of null-value)) + allow-unkown-type)) diff --git a/test/metabase/lib/convert_test.cljc b/test/metabase/lib/convert_test.cljc index d2ef50de48366b3de5eea58994aacc9bc1445bfa..b5869dc37e3100c0139e5f78d5781bfd278ed7ff 100644 --- a/test/metabase/lib/convert_test.cljc +++ b/test/metabase/lib/convert_test.cljc @@ -186,6 +186,13 @@ [:expression "expr" {:display-name "Iambic Diameter"}] + ;; (#29950) + [:starts-with [:field 133751 nil] "CHE" {:case-sensitive true}] + + ;; (#29938) + {"First int" [:case [[[:= [:field 133751 nil] 1] 1]] {:default 0}] + "First bool" [:case [[[:= [:field 133751 nil] 1] true]] {:default false}]} + [:case [[[:< [:field 1 nil] 10] [:value nil {:base_type :type/Number}]] [[:> [:field 2 nil] 2] 10]]] {:database 67 diff --git a/test/metabase/lib/schema/expression/conditional_test.cljc b/test/metabase/lib/schema/expression/conditional_test.cljc index a1ccc86e71d09f8411bdb7c60f48742cf4cde708..d660386b37625b0379e2633e445f81881d1205b4 100644 --- a/test/metabase/lib/schema/expression/conditional_test.cljc +++ b/test/metabase/lib/schema/expression/conditional_test.cljc @@ -1,6 +1,6 @@ (ns metabase.lib.schema.expression.conditional-test (:require - [clojure.test :refer [are deftest is]] + [clojure.test :refer [are deftest is testing]] [malli.core :as mc] [metabase.lib.schema :as lib.schema] [metabase.lib.schema.expression :as expression] @@ -70,3 +70,33 @@ {:base-type :type/Text, :lib/uuid "68443c43-f9de-45e3-9f30-8dfd5fef5af6"} (meta/id :venues :name)] "bar"]}}]}))) + +(deftest ^:parallel case-type-of-with-fields-only-test + ;; Ideally expression/type-of should have enough information to determine the types of fields. + (testing "The type of a case expression can be determined even if it consists of fields only." + (is (= :type/* + (expression/type-of + [:case + {:lib/uuid "8c6e099e-b856-4aeb-a8f6-2266b5d3d1e3"} + [[[:> + {:lib/uuid "9c4cc3b0-f3c7-4d34-ab53-640ba6e911e5"} + [:field {:lib/uuid "435b08c8-9404-41a5-8c5a-00b415f14da6"} 25] + 0] + [:field {:lib/uuid "1c93ba8b-6a39-4ef2-a9e6-e3bcff042800"} 32]]] + [:field + {:source-field 29 + :lib/uuid "a5ab7f91-9826-40a7-9499-4a1a0184a450"} + 23]]))))) + +(deftest ^:parallel coalasce-type-of-with-fields-only-test + ;; Ideally expression/type-of should have enough information to determine the types of fields. + (testing "The type of a case expression can be determined even if it consists of fields only." + (is (= :type/* + (expression/type-of + [:coalesce + {:lib/uuid "8c6e099e-b856-4aeb-a8f6-2266b5d3d1e3"} + [:field {:lib/uuid "435b08c8-9404-41a5-8c5a-00b415f14da6"} 25] + [:field + {:source-field 29 + :lib/uuid "a5ab7f91-9826-40a7-9499-4a1a0184a450"} + 23]]))))) diff --git a/test/metabase/query_processor_test/test_mlv2.clj b/test/metabase/query_processor_test/test_mlv2.clj index ef9a43b1f641bad8b36efe2a0295287d1a6f4e3f..5fb36119e3bdfb0b6790df4ec843d19d69a9e759 100644 --- a/test/metabase/query_processor_test/test_mlv2.clj +++ b/test/metabase/query_processor_test/test_mlv2.clj @@ -35,12 +35,6 @@ [legacy-query] (or *skip-conversion-tests* - ;; #29938: conversion for `:case` with default value does not work correctly - (mbql.u/match-one legacy-query - :case - (mbql.u/match-one &match - {:default _default} - "#29938")) ;; #29942: missing schema for `:cum-sum` and `:cum-count` aggregations (mbql.u/match-one legacy-query #{:cum-sum :cum-count} @@ -57,10 +51,6 @@ (mbql.u/match-one legacy-query :regex-match-first "#29949") - ;; #29950: string filter clauses with options like `:case-sensitive` option not handled correctly - (mbql.u/match-one legacy-query - {:case-sensitive _case-sensitive?} - "#29950") ;; #29958: `:convert-timezone` with 2 args is broken (mbql.u/match-one legacy-query [:convert-timezone _expr _source-timezone]