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

@@ -140,6 +140,9 @@
   [lib.filter
    filter
    filters
+   filterable-columns
+   filterable-column-operators
+   filter-clause
    and
    or
    not

src/metabase/lib/filter.cljc

 (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)})

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."

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.

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]])

test/metabase/lib/filter_test.cljc

 (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)))))

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))))))