Enable expression dashboard filters (#34262)

e2e/test/scenarios/dashboard-filters/reproductions/17775-filter-custom-column-date.cy.spec.js

@@ -15,7 +15,12 @@ const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
 const questionDetails = {
   query: {
     "source-table": ORDERS_ID,
-    expressions: { "CC Date": ["field", ORDERS.CREATED_AT, null] },
+    expressions: {
+      "CC Date": ["field", ORDERS.CREATED_AT, { "base-type": "type/DateTime" }],
+    },
+    "order-by": [
+      ["asc", ["field", ORDERS.ID, { "base-type": "type/BigInteger" }]],
+    ],
   },
 };
 
@@ -31,7 +36,7 @@ const parameters = [
 
 const dashboardDetails = { parameters };
 
-describe.skip("issue 17775", () => {
+describe("issue 17775", () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();
@@ -55,6 +60,7 @@ describe.skip("issue 17775", () => {
 
     // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
     cy.findByText("Column to filter on")
+      .parent()
       .parent()
       .within(() => {
         cy.findByText("Select…").click();
@@ -70,11 +76,9 @@ describe.skip("issue 17775", () => {
   it("should be able to apply dashboard filter to a custom column (metabase#17775)", () => {
     filterWidget().click();
 
-    setQuarterAndYear({ quarter: "Q1", year: "2025" });
-
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("37.65");
+    setQuarterAndYear({ quarter: "Q1", year: "2023" });
 
-    cy.findAllByText("February 11, 2025, 9:40 PM").should("have.length", 2);
+    cy.findAllByText("44.43").should("have.length", 2);
+    cy.findAllByText("March 26, 2023, 8:45 AM").should("have.length", 2);
   });
 });

e2e/test/scenarios/native-filters/helpers/e2e-date-filter-helpers.js

@@ -10,10 +10,10 @@ export function setMonthAndYear({ month, year } = {}) {
 }
 
 export function setQuarterAndYear({ quarter, year } = {}) {
-  cy.findByText(currentYearString).click();
+  popover().findByText(currentYearString).click();
 
-  cy.findByText(year).click();
-  cy.findByText(quarter).click();
+  popover().last().findByText(year).click();
+  popover().findByText(quarter).click();
 }
 
 function setDate(date, container) {

src/metabase/driver/common/parameters/dates.clj

@@ -157,6 +157,11 @@
   [relative-suffix]
   (= "~" relative-suffix))
 
+(defn- with-temporal-unit-if-field
+  [clause unit]
+  (cond-> clause
+    (mbql.u/is-clause? :field clause) (mbql.u/with-temporal-unit unit)))
+
 (def ^:private relative-date-string-decoders
   [{:parser #(= % "today")
     :range  (fn [_ dt]
@@ -164,7 +169,7 @@
                 {:start dt-res,
                  :end   dt-res}))
     :filter (fn [_ field-clause]
-              [:= (mbql.u/with-temporal-unit field-clause :day) [:relative-datetime :current]])}
+              [:= (with-temporal-unit-if-field field-clause :day) [:relative-datetime :current]])}
 
    {:parser #(= % "yesterday")
     :range  (fn [_ dt]
@@ -172,7 +177,7 @@
                 {:start (t/minus dt-res (t/days 1))
                  :end   (t/minus dt-res (t/days 1))}))
     :filter (fn [_ field-clause]
-              [:= (mbql.u/with-temporal-unit field-clause :day) [:relative-datetime -1 :day]])}
+              [:= (with-temporal-unit-if-field field-clause :day) [:relative-datetime -1 :day]])}
 
    ;; Adding a tilde (~) at the end of a past<n><unit>s filter means we should include the current day/etc.
    ;; e.g. past30days  = past 30 days, not including partial data for today ({:include-current false})
@@ -236,7 +241,7 @@
 ;; TODO - using `range->filter` so much below seems silly. Why can't we just bucket the field and use `:=` clauses?
 (defn- range->filter
   [{:keys [start end]} field-clause]
-  [:between (mbql.u/with-temporal-unit field-clause :day) (->iso-8601-date start) (->iso-8601-date end)])
+  [:between (with-temporal-unit-if-field field-clause :day) (->iso-8601-date start) (->iso-8601-date end)])
 
 (def ^:private short-day->day
   {"Mon" :monday
@@ -302,25 +307,25 @@
               {:start date, :end date})
     :filter (fn [{:keys [date]} field-clause]
               (let [iso8601date (->iso-8601-date date)]
-                [:= (mbql.u/with-temporal-unit field-clause :day) iso8601date]))}
+                [:= (with-temporal-unit-if-field field-clause :day) iso8601date]))}
    ;; day range
    {:parser (regex->parser #"([0-9-T:]+)~([0-9-T:]+)" [:date-1 :date-2])
     :range  (fn [{:keys [date-1 date-2]} _]
               {:start date-1, :end date-2})
     :filter (fn [{:keys [date-1 date-2]} field-clause]
-              [:between (mbql.u/with-temporal-unit field-clause :day) (->iso-8601-date date-1) (->iso-8601-date date-2)])}
+              [:between (with-temporal-unit-if-field field-clause :day) (->iso-8601-date date-1) (->iso-8601-date date-2)])}
    ;; before day
    {:parser (regex->parser #"~([0-9-T:]+)" [:date])
     :range  (fn [{:keys [date]} _]
               {:end date})
     :filter (fn [{:keys [date]} field-clause]
-              [:< (mbql.u/with-temporal-unit field-clause :day) (->iso-8601-date date)])}
+              [:< (with-temporal-unit-if-field field-clause :day) (->iso-8601-date date)])}
    ;; after day
    {:parser (regex->parser #"([0-9-T:]+)~" [:date])
     :range  (fn [{:keys [date]} _]
               {:start date})
     :filter (fn [{:keys [date]} field-clause]
-              [:> (mbql.u/with-temporal-unit field-clause :day) (->iso-8601-date date)])}
+              [:> (with-temporal-unit-if-field field-clause :day) (->iso-8601-date date)])}
    ;; exclusions
    {:parser (regex->parser date-exclude-regex [:unit :exclusions])
     :filter (fn [{:keys [unit exclusions]} field-clause]
@@ -328,7 +333,7 @@
                     exclusions (map (partial excluded-datetime unit (t/local-date))
                                     (str/split exclusions #"-"))]
                 (when (and (seq exclusions) (every? some? exclusions))
-                  (into [:!= (mbql.u/with-temporal-unit field-clause (excluded-temporal-unit unit))] exclusions))))}])
+                  (into [:!= (with-temporal-unit-if-field field-clause (excluded-temporal-unit unit))] exclusions))))}])
 
 (def ^:private all-date-string-decoders
   (concat relative-date-string-decoders absolute-date-string-decoders))

src/metabase/models/params.clj

@@ -46,16 +46,23 @@
     (throw (ex-info (tru ":parameter_mappings must be a sequence of maps with :parameter_id and :type keys")
                     {:parameter_mappings parameter_mappings}))))
 
-(mu/defn unwrap-field-clause :- mbql.s/field
-  "Unwrap something that contains a `:field` clause, such as a template tag, Also handles unwrapped integers for
-  legacy compatibility.
+(mu/defn unwrap-field-clause :- [:maybe mbql.s/field]
+  "Unwrap something that contains a `:field` clause, such as a template tag.
+  Also handles unwrapped integers for legacy compatibility.
 
-    (unwrap-field-clause [:field-id 100]) ; -> [:field-id 100]"
+    (unwrap-field-clause [:field 100 nil]) ; -> [:field 100 nil]"
   [field-form]
   (if (integer? field-form)
     [:field field-form nil]
     (mbql.u/match-one field-form :field)))
 
+(mu/defn unwrap-field-or-expression-clause :- mbql.s/Field
+  "Unwrap a `:field` clause or expression clause, such as a template tag. Also handles unwrapped integers for
+  legacy compatibility."
+  [field-or-ref-form]
+  (or (unwrap-field-clause field-or-ref-form)
+      (mbql.u/match-one field-or-ref-form :expression)))
+
 (defn wrap-field-id-if-needed
   "Wrap a raw Field ID in a `:field` clause if needed."
   [field-id-or-form]
@@ -101,7 +108,7 @@
   [[_ tag] card]
   (get-in card [:dataset_query :native :template-tags (u/qualified-name tag) :dimension]))
 
-(mu/defn param-target->field-clause :- [:maybe mbql.s/field]
+(mu/defn param-target->field-clause :- [:maybe mbql.s/Field]
   "Parse a Card parameter `target` form, which looks something like `[:dimension [:field-id 100]]`, and return the Field
   ID it references (if any)."
   [target card]
@@ -109,7 +116,7 @@
     (when (mbql.u/is-clause? :dimension target)
       (let [[_ dimension] target]
         (try
-          (unwrap-field-clause
+          (unwrap-field-or-expression-clause
            (if (mbql.u/is-clause? :template-tag dimension)
              (template-tag->field-form dimension card)
              dimension))
@@ -215,7 +222,7 @@
 ;;; |                                               DASHBOARD-SPECIFIC                                               |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(mu/defn ^:private dashcards->parameter-mapping-field-clauses :- [:maybe [:set mbql.s/field]]
+(mu/defn ^:private dashcards->parameter-mapping-field-clauses :- [:maybe [:set mbql.s/Field]]
   "Return set of any Fields referenced directly by the Dashboard's `:parameters` (i.e., 'explicit' parameters) by
   looking at the appropriate `:parameter_mappings` entries for its Dashcards."
   [dashcards]

src/metabase/query_processor/middleware/parameters/mbql.clj

@@ -3,6 +3,8 @@
   (:require
    [metabase.driver.common.parameters.dates :as params.dates]
    [metabase.driver.common.parameters.operators :as params.ops]
+   [metabase.lib.convert :as lib.convert]
+   [metabase.lib.core :as lib]
    [metabase.lib.metadata.protocols :as lib.metadata.protocols]
    [metabase.mbql.schema :as mbql.s]
    [metabase.mbql.util :as mbql.u]
@@ -21,21 +23,33 @@
     (Double/parseDouble s)
     (Long/parseLong s)))
 
+(defn- field-type
+  [field-clause]
+  (mbql.u/match-one field-clause
+    [:field (id :guard integer?) _]  ((some-fn :effective-type :base-type)
+                                      (lib.metadata.protocols/field (qp.store/metadata-provider) id))
+    [:field (_ :guard string?) opts] (:base-type opts)))
+
+(defn- expression-type
+  [query expression-clause]
+  (mbql.u/match-one expression-clause
+    [:expression (expression-name :guard string?)]
+    (lib/type-of (lib/query (qp.store/metadata-provider) (lib.convert/->pMBQL query))
+                 (lib.convert/->pMBQL &match))))
+
 (mu/defn ^:private parse-param-value-for-type
   "Convert `param-value` to a type appropriate for `param-type`.
   The frontend always passes parameters in as strings, which is what we want in most cases; for numbers, instead
   convert the parameters to integers or floating-point numbers."
-  [param-type param-value field-clause :- mbql.s/field]
+  [query param-type param-value field-clause :- mbql.s/Field]
   (cond
     ;; for `id` or `category` type params look up the base-type of the Field and see if it's a number or not.
     ;; If it *is* a number then recursively call this function and parse the param value as a number as appropriate.
     (and (#{:id :category} param-type)
-         (let [base-type (mbql.u/match-one field-clause
-                           [:field (id :guard integer?) _]  ((some-fn :effective-type :base-type)
-                                                             (lib.metadata.protocols/field (qp.store/metadata-provider) id))
-                           [:field (_ :guard string?) opts] (:base-type opts))]
+         (let [base-type (or (field-type field-clause)
+                             (expression-type query field-clause))]
            (isa? base-type :type/Number)))
-    (recur :number param-value field-clause)
+    (recur query :number param-value field-clause)
 
     ;; no conversion needed if PARAM-TYPE isn't :number or PARAM-VALUE isn't a string
     (or (not= param-type :number)
@@ -46,7 +60,7 @@
     (to-numeric param-value)))
 
 (mu/defn ^:private build-filter-clause :- [:maybe mbql.s/Filter]
-  [{param-type :type, param-value :value, [_ field :as target] :target, :as param}]
+  [query {param-type :type, param-value :value, [_ field :as target] :target, :as param}]
   (cond
     (params.ops/operator? param-type)
     (params.ops/to-clause param)
@@ -54,12 +68,13 @@
     (sequential? param-value)
     (mbql.u/simplify-compound-filter
      (vec (cons :or (for [value param-value]
-                      (build-filter-clause {:type param-type, :value value, :target target})))))
+                      (build-filter-clause query {:type param-type, :value value, :target target})))))
 
     ;; single value, date range. Generate appropriate MBQL clause based on date string
     (params.dates/date-type? param-type)
-    (params.dates/date-string->filter (parse-param-value-for-type param-type param-value (params/unwrap-field-clause field))
-                                      field)
+    (params.dates/date-string->filter
+     (parse-param-value-for-type query param-type param-value (params/unwrap-field-or-expression-clause field))
+     field)
 
     ;; TODO - We can't tell the difference between a dashboard parameter (convert to an MBQL filter) and a native
     ;; query template tag parameter without this. There's should be a better, less fragile way to do this. (Not 100%
@@ -71,7 +86,7 @@
     :else
     [:=
      (params/wrap-field-id-if-needed field)
-     (parse-param-value-for-type param-type param-value (params/unwrap-field-clause field))]))
+     (parse-param-value-for-type query param-type param-value (params/unwrap-field-or-expression-clause field))]))
 
 (defn expand
   "Expand parameters for MBQL queries in `query` (replacing Dashboard or Card-supplied params with the appropriate
@@ -87,6 +102,6 @@
       (recur query rest)
 
       :else
-      (let [filter-clause (build-filter-clause (assoc param :value param-value))
+      (let [filter-clause (build-filter-clause query (assoc param :value param-value))
             query         (mbql.u/add-filter-clause query filter-clause)]
         (recur query rest)))))

test/metabase/query_processor/middleware/parameters/mbql_test.clj

@@ -275,14 +275,15 @@
                             :target $date
                             :value  ["2014-06" "2015-06"]}]})))))
 
-(defn- build-filter-clause [param]
+(defn- build-filter-clause [query param]
   (qp.store/with-metadata-provider (mt/id)
-    (#'qp.mbql/build-filter-clause param)))
+    (#'qp.mbql/build-filter-clause query param)))
 
 (deftest ^:parallel convert-ids-to-numbers-test
   (is (= (mt/$ids venues
            [:= $id 1])
          (build-filter-clause
+          nil
           (mt/$ids venues
             {:type   :id
              :target [:dimension $id]

test/metabase/query_processor/middleware/parameters_test.clj

@@ -78,6 +78,27 @@
                               :parameters   [{:name "price", :type :category, :target $price, :value 1}]}
                :aggregation  [[:count]]}))))))
 
+(deftest ^:parallel expand-mbql-source-query-date-expression-param-test
+  (testing "can we expand MBQL number and date expression params in a source query?"
+    (is (= (mt/mbql-query users
+             {:source-query {:source-table (meta/id :users)
+                             :expressions {"date-column" [:field (meta/id :users :last-login) nil]
+                                           "number-column" [:field (meta/id :users :id) nil]}
+                             :filter [:and
+                                      [:between [:expression "date-column"] "2019-09-29" "2023-09-29"]
+                                      [:= [:expression "number-column"] 1]]}})
+           (substitute-params
+            (mt/mbql-query users
+              {:source-query {:source-table (meta/id :users)
+                              :expressions {"date-column" [:field (meta/id :users :last-login) nil]
+                                            "number-column" [:field (meta/id :users :id) nil]}
+                              :parameters   [{:type :date/range
+                                              :value "2019-09-29~2023-09-29"
+                                              :target [:dimension [:expression "date-column"]]}
+                                             {:type :category
+                                              :value 1
+                                              :target [:dimension [:expression "number-column"]]}]}}))))))
+
 (deftest ^:parallel expand-native-source-query-params-test
   (testing "can we expand native params if in a source query?"
     (is (= (mt/mbql-query nil