diff --git a/e2e/test/scenarios/dashboard-filters/reproductions/17775-filter-custom-column-date.cy.spec.js b/e2e/test/scenarios/dashboard-filters/reproductions/17775-filter-custom-column-date.cy.spec.js
index 6078dd9736d77d6b1018d3f075ba31a75401e9d0..59908f41535b6906d6f29830de66d47138289623 100644
--- a/e2e/test/scenarios/dashboard-filters/reproductions/17775-filter-custom-column-date.cy.spec.js
+++ b/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);
});
});
diff --git a/e2e/test/scenarios/native-filters/helpers/e2e-date-filter-helpers.js b/e2e/test/scenarios/native-filters/helpers/e2e-date-filter-helpers.js
index 5af610f88483d9896b036f89f5f5659fb9369d51..fcd1d834114d3fbb130bbe3822482b13c1fa3a2a 100644
--- a/e2e/test/scenarios/native-filters/helpers/e2e-date-filter-helpers.js
+++ b/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) {
diff --git a/src/metabase/driver/common/parameters/dates.clj b/src/metabase/driver/common/parameters/dates.clj
index b5c177c25782fc484fad7e7b74869892583ff2da..6b9a94cbc2a1ac3ded90e90830c5f1d92f4826ec 100644
--- a/src/metabase/driver/common/parameters/dates.clj
+++ b/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))
diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj
index 0613e2be8baaa4d31e75030b1d9d313f10192067..191431cca65b6c8d285bd52a53c6a92fdb53859d 100644
--- a/src/metabase/models/params.clj
+++ b/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]
diff --git a/src/metabase/query_processor/middleware/parameters/mbql.clj b/src/metabase/query_processor/middleware/parameters/mbql.clj
index 1258c7d8f68a27fdbb6ef2ec755c594dcca250b7..b4abae1af787bc3ef9924fd66d7fb4df4af5c38b 100644
--- a/src/metabase/query_processor/middleware/parameters/mbql.clj
+++ b/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)))))
diff --git a/test/metabase/query_processor/middleware/parameters/mbql_test.clj b/test/metabase/query_processor/middleware/parameters/mbql_test.clj
index ac6fc3b8f6fe900bd5abd98cb05790054163d040..d05e6ef4191b4002267216ebd1b1047317f35ea4 100644
--- a/test/metabase/query_processor/middleware/parameters/mbql_test.clj
+++ b/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]
diff --git a/test/metabase/query_processor/middleware/parameters_test.clj b/test/metabase/query_processor/middleware/parameters_test.clj
index 30d34fde8162574cbfdf769b801132a955b09fb6..56dece06e079df641abdeb3acc3c738ec14da492 100644
--- a/test/metabase/query_processor/middleware/parameters_test.clj
+++ b/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