From 5d315c9ee3f5ee1d9867b4bf70e628a413d7667e Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Mon, 29 May 2023 15:31:25 -0600
Subject: [PATCH] Metabase lib 2 filterable columns (#31105)
* Add initial filterable columns implemenation
* Implement filterable-columns
Change filters to return internal clauses rather than external operators
* Add comments
---
src/metabase/lib/core.cljc | 3 +
src/metabase/lib/filter.cljc | 171 ++++++++++++++++++++-
src/metabase/lib/js.cljs | 20 +++
src/metabase/lib/schema/expression.cljc | 6 +-
src/metabase/lib/schema/filter.cljc | 9 +-
test/metabase/lib/filter_test.cljc | 90 +++++++++--
test/metabase/lib/remove_replace_test.cljc | 2 +-
7 files changed, 275 insertions(+), 26 deletions(-)
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index 61c5ecec054..f7316153694 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -140,6 +140,9 @@
[lib.filter
filter
filters
+ filterable-columns
+ filterable-column-operators
+ filter-clause
and
or
not
diff --git a/src/metabase/lib/filter.cljc b/src/metabase/lib/filter.cljc
index d9a60f08995..d5fbd0a4950 100644
--- a/src/metabase/lib/filter.cljc
+++ b/src/metabase/lib/filter.cljc
@@ -1,22 +1,23 @@
(ns metabase.lib.filter
(:refer-clojure
:exclude
- [filter and or not = < <= > ->> >= not-empty case])
+ [filter and or not = < <= > >= not-empty case])
(:require
[clojure.string :as str]
[metabase.lib.common :as lib.common]
[metabase.lib.hierarchy :as lib.hierarchy]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
- [metabase.lib.schema]
- [metabase.lib.schema.common :as schema.common]
+ [metabase.lib.schema :as lib.schema]
+ [metabase.lib.schema.filter :as lib.schema.filter]
[metabase.lib.temporal-bucket :as lib.temporal-bucket]
+ [metabase.lib.types.isa :as lib.types.isa]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
+ [metabase.util :as u]
[metabase.util.malli :as mu])
#?(:cljs (:require-macros [metabase.lib.filter])))
-(comment metabase.lib.schema/keep-me)
-
(doseq [tag [:and :or]]
(lib.hierarchy/derive tag ::compound))
@@ -157,7 +158,7 @@
new-filter (lib.common/->op-arg query stage-number boolean-expression)]
(lib.util/update-query-stage query stage-number update :filters (fnil conj []) new-filter))))
-(mu/defn filters :- [:maybe [:sequential ::schema.common/external-op]]
+(mu/defn filters :- [:maybe [:ref ::lib.schema/filters]]
"Returns the current filters in stage with `stage-number` of `query`.
If `stage-number` is omitted, the last stage is used. Logicaly, the
filter attached to the query is the conjunction of the expressions
@@ -167,5 +168,159 @@
([query :- :metabase.lib.schema/query] (filters query nil))
([query :- :metabase.lib.schema/query
stage-number :- [:maybe :int]]
- (some->> (clojure.core/not-empty (:filters (lib.util/query-stage query (clojure.core/or stage-number -1))))
- (mapv lib.common/external-op))))
+ (clojure.core/not-empty (:filters (lib.util/query-stage query (clojure.core/or stage-number -1))))))
+
+(defmethod lib.metadata.calculation/display-name-method :mbql.filter/operator
+ [_query _stage-number {:keys [display-name]} _display-name-style]
+ display-name)
+
+(defmethod lib.metadata.calculation/display-info-method :mbql.filter/operator
+ [_query _stage-number {:keys [display-name] short-name :short}]
+ {:short-name (u/qualified-name short-name)
+ :display-name display-name})
+
+(defn- filter-operators
+ "The list of available filter operators.
+ The order of operators is relevant for the front end.
+ There are slight differences between names and ordering for the different base types."
+ [column]
+ (let [key-operators [{:short := :display-name (i18n/tru "Is")}
+ {:short :!= :display-name (i18n/tru "Is not")}
+ {:short :> :display-name (i18n/tru "Greater than")}
+ {:short :< :display-name (i18n/tru "Less than")}
+ {:short :between :display-name (i18n/tru "Between")}
+ {:short :>= :display-name (i18n/tru "Greater than or equal to")}
+ {:short :<= :display-name (i18n/tru "Less than or equal to")}
+ {:short :is-null :display-name (i18n/tru "Is empty")}
+ {:short :not-null :display-name (i18n/tru "Not empty")}]]
+ ;; The order of these clauses is important since we want to match the most relevant type
+ (condp #(lib.types.isa/isa? %2 %1) column
+ :type/PK
+ key-operators
+
+ :type/FK
+ key-operators
+
+ :type/Location
+ [{:short := :display-name (i18n/tru "Is")}
+ {:short :!= :display-name (i18n/tru "Is not")}
+ {:short :is-empty :display-name (i18n/tru "Is empty")}
+ {:short :not-empty :display-name (i18n/tru "Not empty")}
+ {:short :contains :display-name (i18n/tru "Contains")}
+ {:short :does-not-contain :display-name (i18n/tru "Does not contain")}
+ {:short :starts-with :display-name (i18n/tru "Starts with")}
+ {:short :ends-with :display-name (i18n/tru "Ends with")}]
+
+ :type/Temporal
+ [{:short :!= :display-name (i18n/tru "Excludes")}
+ {:short := :display-name (i18n/tru "Is")}
+ {:short :< :display-name (i18n/tru "Before")}
+ {:short :> :display-name (i18n/tru "After")}
+ {:short :between :display-name (i18n/tru "Between")}
+ {:short :is-null :display-name (i18n/tru "Is empty")}
+ {:short :not-null :display-name (i18n/tru "Not empty")}]
+
+ :type/Coordinate
+ [{:short := :display-name (i18n/tru "Is")}
+ {:short :!= :display-name (i18n/tru "Is not")}
+ {:short :inside :display-name (i18n/tru "Inside")}
+ {:short :> :display-name (i18n/tru "Greater than")}
+ {:short :< :display-name (i18n/tru "Less than")}
+ {:short :between :display-name (i18n/tru "Between")}
+ {:short :>= :display-name (i18n/tru "Greater than or equal to")}
+ {:short :<= :display-name (i18n/tru "Less than or equal to")}]
+
+ :type/Number
+ [{:short := :display-name (i18n/tru "Equal to")}
+ {:short :!= :display-name (i18n/tru "Not equal to")}
+ {:short :> :display-name (i18n/tru "Greater than")}
+ {:short :< :display-name (i18n/tru "Less than")}
+ {:short :between :display-name (i18n/tru "Between")}
+ {:short :>= :display-name (i18n/tru "Greater than or equal to")}
+ {:short :<= :display-name (i18n/tru "Less than or equal to")}
+ {:short :is-null :display-name (i18n/tru "Is empty")}
+ {:short :not-null :display-name (i18n/tru "Not empty")}]
+
+ :type/Text
+ [{:short := :display-name (i18n/tru "Is")}
+ {:short :!= :display-name (i18n/tru "Is not")}
+ {:short :contains :display-name (i18n/tru "Contains")}
+ {:short :does-not-contain :display-name (i18n/tru "Does not contain")}
+ {:short :is-null :display-name (i18n/tru "Is null")}
+ {:short :not-null :display-name (i18n/tru "Not null")}
+ {:short :is-empty :display-name (i18n/tru "Is empty")}
+ {:short :not-empty :display-name (i18n/tru "Not empty")}
+ {:short :starts-with :display-name (i18n/tru "Starts with")}
+ {:short :ends-with :display-name (i18n/tru "Ends with")}]
+
+ :type/TextLike
+ [{:short := :display-name (i18n/tru "Is")}
+ {:short :!= :display-name (i18n/tru "Is not")}
+ {:short :is-null :display-name (i18n/tru "Is null")}
+ {:short :not-null :display-name (i18n/tru "Not null")}
+ {:short :is-empty :display-name (i18n/tru "Is empty")}
+ {:short :not-empty :display-name (i18n/tru "Not empty")}]
+
+ :type/Boolean
+ [{:short := :display-name (i18n/tru "Is")}
+ {:short :is-null :display-name (i18n/tru "Is empty")}
+ {:short :not-null :display-name (i18n/tru "Not empty")}]
+
+ ;; default
+ [{:short := :display-name (i18n/tru "Is")}
+ {:short :!= :display-name (i18n/tru "Is not")}
+ {:short :is-null :display-name (i18n/tru "Is null")}
+ {:short :not-null :display-name (i18n/tru "Not null")}])))
+
+(def ^:private ColumnWithOperators
+ [:merge
+ lib.metadata/ColumnMetadata
+ [:map
+ [:operators {:optional true} [:sequential ::lib.schema.filter/operator]]]])
+
+(mu/defn filterable-column-operators :- [:maybe [:sequential ::lib.schema.filter/operator]]
+ "Returns the operators for which `filterable-column` is applicable."
+ [filterable-column :- ColumnWithOperators]
+ (:operators filterable-column))
+
+(mu/defn filterable-columns :- [:sequential ColumnWithOperators]
+ "Get column metadata for all the columns that can be filtered in
+ the stage number `stage-number` of the query `query`
+ If `stage-number` is omitted, the last stage is used.
+ The rules for determining which columns can be broken out by are as follows:
+
+ 1. custom `:expressions` in this stage of the query
+
+ 2. Fields 'exported' by the previous stage of the query, if there is one;
+ otherwise Fields from the current `:source-table`
+
+ 3. Fields exported by explicit joins
+
+ 4. Fields in Tables that are implicitly joinable."
+
+ ([query :- ::lib.schema/query]
+ (filterable-columns query -1))
+
+ ([query :- ::lib.schema/query
+ stage-number :- :int]
+ (let [stage (lib.util/query-stage query stage-number)
+ columns (lib.metadata.calculation/visible-columns query stage-number stage)
+ with-operators (fn [column]
+ (when-let [operators (->> (filter-operators column)
+ (mapv #(assoc % :lib/type :mbql.filter/operator))
+ clojure.core/not-empty)]
+ (assoc column :operators operators)))]
+ (clojure.core/not-empty
+ (into []
+ (keep with-operators)
+ columns)))))
+
+(mu/defn filter-clause
+ "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)})
diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs
index a5759eade3e..c8444c30710 100644
--- a/src/metabase/lib/js.cljs
+++ b/src/metabase/lib/js.cljs
@@ -339,6 +339,26 @@
[aggregation-operator]
(to-array (lib.core/aggregation-operator-columns aggregation-operator)))
+(defn ^:export filterable-columns
+ "Get the available filterable columns for the stage with `stage-number` of
+ the query `a-query`.
+ If `stage-number` is omitted, the last stage is used."
+ ([a-query]
+ (filterable-columns a-query -1))
+ ([a-query stage-number]
+ (to-array (lib.core/filterable-columns a-query stage-number))))
+
+(defn ^:export filterable-column-operators
+ "Returns the operators for which `filterable-column` is applicable."
+ [filterable-column]
+ (to-array (lib.core/filterable-column-operators filterable-column)))
+
+(defn ^:export filter-clause
+ "Returns a standalone filter clause for a `filter-operator`,
+ a `column`, and arguments."
+ [filter-operator column & args]
+ (apply lib.core/filter-clause filter-operator column args))
+
(defn ^:export fields
"Get the current `:fields` in a query. Unlike the lib core version, this will return an empty sequence if `:fields` is
not specified rather than `nil` for JS-friendliness."
diff --git a/src/metabase/lib/schema/expression.cljc b/src/metabase/lib/schema/expression.cljc
index 59995074439..ef150881173 100644
--- a/src/metabase/lib/schema/expression.cljc
+++ b/src/metabase/lib/schema/expression.cljc
@@ -138,9 +138,13 @@
(expression-schema orderable-types
"an expression that can be compared with :> or :<"))
+(def equality-comparable-types
+ "Set of base types that can be campared with equality."
+ #{:type/Boolean :type/Text :type/Number :type/Temporal})
+
(mr/def ::equality-comparable
[:maybe
- (expression-schema #{:type/Boolean :type/Text :type/Number :type/Temporal}
+ (expression-schema equality-comparable-types
"an expression that can appear in := or :!=")])
;;; any type of expression.
diff --git a/src/metabase/lib/schema/filter.cljc b/src/metabase/lib/schema/filter.cljc
index 52e026ee4ad..3af76c8eb9e 100644
--- a/src/metabase/lib/schema/filter.cljc
+++ b/src/metabase/lib/schema/filter.cljc
@@ -5,7 +5,8 @@
[metabase.lib.schema.common :as common]
[metabase.lib.schema.expression :as expression]
[metabase.lib.schema.mbql-clause :as mbql-clause]
- [metabase.lib.schema.temporal-bucketing :as temporal-bucketing]))
+ [metabase.lib.schema.temporal-bucketing :as temporal-bucketing]
+ [metabase.util.malli.registry :as mr]))
(doseq [op [:and :or]]
(mbql-clause/define-catn-mbql-clause op :- :type/Boolean
@@ -97,3 +98,9 @@
[:= :segment]
::common/options
[:or ::common/int-greater-than-zero ::common/non-blank-string]])
+
+(mr/def ::operator
+ [:map
+ [:lib/type [:= :mbql.filter/operator]]
+ [:short [:enum := :!= :inside :between :< :> :<= :>= :is-null :not-null :is-empty :not-empty :contains :does-not-contain :starts-with :ends-with]]
+ [:display-name :string]])
diff --git a/test/metabase/lib/filter_test.cljc b/test/metabase/lib/filter_test.cljc
index c546fbe9584..ea76197d42d 100644
--- a/test/metabase/lib/filter_test.cljc
+++ b/test/metabase/lib/filter_test.cljc
@@ -1,11 +1,11 @@
(ns metabase.lib.filter-test
(:require
+ #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
[clojure.test :refer [deftest is testing]]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[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]))
(defn- test-clause [result-filter f & args]
(testing "return a function for later resolution"
@@ -14,7 +14,7 @@
(is (=? result-filter
(f' {:lib/metadata meta/metadata} -1))))))
-(deftest ^:parallel filter-clause-test
+(deftest ^:parallel general-filter-clause-test
(let [q1 (lib/query-for-table-name meta/metadata-provider "CATEGORIES")
q2 (lib/saved-question-query meta/metadata-provider meta/saved-question)
q3 (lib/query-for-table-name meta/metadata-provider "CHECKINS")
@@ -126,9 +126,7 @@
(testing "setting a simple filter via the helper function"
(let [result-query
(lib/filter q1 (lib/between venues-category-id-metadata 42 100))
- result-filter {:operator (-> original-filter first name)
- :options (second original-filter)
- :args (subvec original-filter 2)}]
+ result-filter original-filter]
(is (=? simple-filtered-query
(dissoc result-query :lib/metadata)))
(testing "and getting the current filter"
@@ -152,23 +150,17 @@
(meta/id :venues :category-id)]
42
100]
- first-result-filter {:operator (-> first-filter first name)
- :options (second first-filter)
- :args (subvec first-filter 2)}
+ first-result-filter first-filter
second-filter [:starts-with
{:lib/uuid string?}
[:field {:base-type :type/Text, :lib/uuid string?} (meta/id :venues :name)]
"prefix"]
- second-result-filter {:operator (-> second-filter first name)
- :options (second second-filter)
- :args (subvec second-filter 2)}
+ second-result-filter second-filter
third-filter [:contains
{:lib/uuid string?}
[:field {:base-type :type/Text, :lib/uuid string?} (meta/id :venues :name)]
"part"]
- third-result-filter {:operator (-> third-filter first name)
- :options (second third-filter)
- :args (subvec third-filter 2)}
+ third-result-filter third-filter
first-add (lib/filter simple-query
(lib/between
(lib/field "VENUES" "CATEGORY_ID")
@@ -212,3 +204,71 @@
{:lib/uuid "953597df-a96d-4453-a57b-665e845abc69"}
[:field {:lib/uuid "be28f393-538a-406b-90da-bac5f8ef565e"} (meta/id :venues :name)]
"t"]))) ))
+
+(deftest ^:parallel filterable-columns
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "USERS")
+ (lib/join (-> (lib/join-clause (lib/table (meta/id :checkins))
+ [(lib/=
+ (lib/field "CHECKINS" "USER_ID")
+ (lib/field "USERS" "ID"))])
+ (lib/with-join-fields :all)))
+ (lib/join (-> (lib/join-clause (lib/table (meta/id :venues))
+ [(lib/=
+ (lib/field "CHECKINS" "VENUE_ID")
+ (lib/field "VENUES" "ID"))])
+ (lib/with-join-fields :all))))
+ columns (lib/filterable-columns query)
+ pk-operators [:= :!= :> :< :between :>= :<= :is-null :not-null]
+ temporal-operators [:!= := :< :> :between :is-null :not-null]
+ coordinate-operators [:= :!= :is-empty :not-empty :contains :does-not-contain :starts-with :ends-with]
+ text-operators [:= :!= :contains :does-not-contain :is-null :not-null :is-empty :not-empty :starts-with :ends-with]]
+ (is (= ["ID"
+ "NAME"
+ "LAST_LOGIN"
+ "Checkins__ID"
+ "Checkins__DATE"
+ "Checkins__USER_ID"
+ "Checkins__VENUE_ID"
+ "Venues__ID"
+ "Venues__NAME"
+ "Venues__CATEGORY_ID"
+ "Venues__LATITUDE"
+ "Venues__LONGITUDE"
+ "Venues__PRICE"
+ "CATEGORIES__via__CATEGORY_ID__ID"
+ "CATEGORIES__via__CATEGORY_ID__NAME"]
+ (map :lib/desired-column-alias columns)))
+ (testing "Operators are attached to proper columns"
+ (is (=? {"ID" pk-operators,
+ "NAME" text-operators,
+ "Venues__PRICE" pk-operators
+ "Venues__LATITUDE" coordinate-operators
+ "LAST_LOGIN" temporal-operators}
+ (into {} (for [col columns]
+ [(:lib/desired-column-alias col) (mapv :short (lib/filterable-column-operators col))])))))
+ (testing "Type specific display names"
+ (let [display-info-by-type-and-op (->> columns
+ (map (juxt :lib/desired-column-alias
+ (fn [col]
+ (->> col
+ lib/filterable-column-operators
+ (map (comp (juxt :short-name :display-name) #(lib/display-info query %)))
+ (into {})))))
+ (into {}))]
+ (is (=? {"ID" {"=" "Is" "is-null" "Is empty" ">" "Greater than"}
+ "NAME" {"=" "Is" "is-null" "Is null" "is-empty" "Is empty"}
+ "LAST_LOGIN" {"!=" "Excludes" ">" "After"}
+ "Venues__PRICE" {"=" "Equal to" "is-null" "Is empty"}}
+ display-info-by-type-and-op))))))
+
+(deftest ^:parallel filter-clause-test
+ (let [query (-> (lib/query-for-table-name meta/metadata-provider "USERS"))
+ [first-col] (lib/filterable-columns query)
+ new-filter (lib/filter-clause
+ (first (lib/filterable-column-operators first-col))
+ first-col
+ 515)]
+ (is (=? [[:= {} [:field {} (meta/id :users :id)] 515]]
+ (-> query
+ (lib/filter new-filter)
+ lib/filters)))))
diff --git a/test/metabase/lib/remove_replace_test.cljc b/test/metabase/lib/remove_replace_test.cljc
index a2979cba35a..c8100260a95 100644
--- a/test/metabase/lib/remove_replace_test.cljc
+++ b/test/metabase/lib/remove_replace_test.cljc
@@ -241,7 +241,7 @@
(lib/replace-clause (first filters) (lib/= (lib/field (meta/id :venues :id)) 1)))
replaced-filters (lib/filters replaced)]
(is (not= filters replaced-filters))
- (is (=? {:operator "=" :args [[:field {} (meta/id :venues :id)] 1]}
+ (is (=? [:= {} [:field {} (meta/id :venues :id)] 1]
(first replaced-filters)))
(is (= 2 (count replaced-filters)))
(is (= (second filters) (second replaced-filters))))))
--
GitLab