From a001218961a01b48d42889b96bd2308ea280029c Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Thu, 11 May 2023 20:01:03 +0300
Subject: [PATCH] Support expression options and fields only case and coalasce
exprs (#30395)
* Support expression options and fields only case and coalasce exprs
Fixes #29938.
Fixes #29950.
---
src/metabase/lib/aggregation.cljc | 2 --
src/metabase/lib/convert.cljc | 2 +-
.../lib/schema/expression/conditional.cljc | 26 +++++++++++----
test/metabase/lib/convert_test.cljc | 7 ++++
.../schema/expression/conditional_test.cljc | 32 ++++++++++++++++++-
.../query_processor_test/test_mlv2.clj | 10 ------
6 files changed, 58 insertions(+), 21 deletions(-)
diff --git a/src/metabase/lib/aggregation.cljc b/src/metabase/lib/aggregation.cljc
index 87bd19ea34b..99ee4499af1 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 93d6c971a0f..8388c45a3c7 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 46a01c022e6..092d0226172 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 d2ef50de483..b5869dc37e3 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 a1ccc86e71d..d660386b376 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 ef9a43b1f64..5fb36119e3b 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]
--
GitLab