From 7f9afb07bca591c5c77502490a52c0bc181df83d Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Tue, 20 Feb 2024 11:49:07 -0700
Subject: [PATCH] Fixes filters against datetime binned expressions (#38874)
* Fixes filters against datetime binned expressions
Fixes #33528
Turns out that `mbql/normalize` and `lib/normalize` behaved slightly
different and `[:expression "abc" {...}]` refs would drop their opts in
the former path. In order to properly query against binned datetimes
it's important that expression ref does not lose its type or else the
optimizer will not see that `time-interval` needs to convert to a
`between` rather than an `=`.
* Fix test
* Only keep specific keys on expression opts for these expression filters
* Don't run checkin dataset on snowflake or athena
---
src/metabase/mbql/normalize.cljc | 38 ++++++++-----
test/metabase/mbql/normalize_test.cljc | 56 ++++++++++---------
.../date_bucketing_test.clj | 23 ++++++++
3 files changed, 79 insertions(+), 38 deletions(-)
diff --git a/src/metabase/mbql/normalize.cljc b/src/metabase/mbql/normalize.cljc
index 676e5bb985b..8da0f84160d 100644
--- a/src/metabase/mbql/normalize.cljc
+++ b/src/metabase/mbql/normalize.cljc
@@ -86,12 +86,30 @@
(cond-> [:aggregation aggregation-index]
(some? option) (conj option)))
+(defn- normalize-ref-opts [opts]
+ (let [opts (normalize-tokens opts :ignore-path)]
+ (cond-> opts
+ (:base-type opts) (update :base-type keyword)
+ (:temporal-unit opts) (update :temporal-unit keyword)
+ (:binning opts) (update :binning (fn [binning]
+ (cond-> binning
+ (:strategy binning) (update :strategy keyword)))))))
+
(defmethod normalize-mbql-clause-tokens :expression
;; For expression references (`[:expression \"my_expression\"]`) keep the arg as is but make sure it is a string.
- [[_ expression-name]]
- [:expression (if (keyword? expression-name)
- (mbql.u/qualified-name expression-name)
- expression-name)])
+ [[_ expression-name opts]]
+ (let [expression [:expression (if (keyword? expression-name)
+ (mbql.u/qualified-name expression-name)
+ expression-name)]
+ opts (->> opts
+ normalize-ref-opts
+ ;; Only keep fields required for handling binned&datetime expressions (#33528)
+ ;; Allowing added alias-info through here breaks
+ ;; [[metabase.query-processor.util.nest-query-test/nest-expressions-ignore-source-queries-test]]
+ (m/filter-keys #{:base-type :temporal-unit :binning})
+ not-empty)]
+ (cond-> expression
+ opts (conj opts))))
(defmethod normalize-mbql-clause-tokens :binning-strategy
;; For `:binning-strategy` clauses (which wrap other Field clauses) normalize the strategy-name and recursively
@@ -103,15 +121,9 @@
(defmethod normalize-mbql-clause-tokens :field
[[_ id-or-name opts]]
- (let [opts (normalize-tokens opts :ignore-path)]
- [:field
- id-or-name
- (cond-> opts
- (:base-type opts) (update :base-type keyword)
- (:temporal-unit opts) (update :temporal-unit keyword)
- (:binning opts) (update :binning (fn [binning]
- (cond-> binning
- (:strategy binning) (update :strategy keyword)))))]))
+ [:field
+ id-or-name
+ (normalize-ref-opts opts)])
(defmethod normalize-mbql-clause-tokens :field-literal
;; Similarly, for Field literals, keep the arg as-is, but make sure it is a string."
diff --git a/test/metabase/mbql/normalize_test.cljc b/test/metabase/mbql/normalize_test.cljc
index b5fa11eeb0e..309aad636bc 100644
--- a/test/metabase/mbql/normalize_test.cljc
+++ b/test/metabase/mbql/normalize_test.cljc
@@ -558,37 +558,43 @@
;; this is also covered
(t/deftest ^:parallel normalize-expressions-test
(normalize-tests
- "Expression names should get normalized to strings."
- {{:query {"expressions" {:abc ["+" 1 2]}
- :fields [["expression" :abc]]}}
- {:query {:expressions {"abc" [:+ 1 2]}
- :fields [[:expression "abc"]]}}}
+ "Expression names should get normalized to strings."
+ {{:query {"expressions" {:abc ["+" 1 2]}
+ :fields [["expression" :abc]]}}
+ {:query {:expressions {"abc" [:+ 1 2]}
+ :fields [[:expression "abc"]]}}}
- "are expression names exempt from lisp-casing/lower-casing?"
- {{"query" {"expressions" {:sales_tax ["-" ["field-id" 10] ["field-id" 20]]}}}
- {:query {:expressions {"sales_tax" [:- [:field-id 10] [:field-id 20]]}}}}
+ "are expression names exempt from lisp-casing/lower-casing?"
+ {{"query" {"expressions" {:sales_tax ["-" ["field-id" 10] ["field-id" 20]]}}}
+ {:query {:expressions {"sales_tax" [:- [:field-id 10] [:field-id 20]]}}}}
- "expressions should handle datetime arithemtics"
- {{:query {:expressions {:prev_month ["+" ["field-id" 13] ["interval" -1 "month"]]}}}
- {:query {:expressions {"prev_month" [:+ [:field-id 13] [:interval -1 :month]]}}}
+ "expressions should handle datetime arithemtics"
+ {{:query {:expressions {:prev_month ["+" ["field-id" 13] ["interval" -1 "month"]]}}}
+ {:query {:expressions {"prev_month" [:+ [:field-id 13] [:interval -1 :month]]}}}
- {:query {:expressions {:prev_month ["-" ["field-id" 13] ["interval" 1 "month"] ["interval" 1 "day"]]}}}
- {:query {:expressions {"prev_month" [:- [:field-id 13] [:interval 1 :month] [:interval 1 :day]]}}}
+ {:query {:expressions {:prev_month ["-" ["field-id" 13] ["interval" 1 "month"] ["interval" 1 "day"]]}}}
+ {:query {:expressions {"prev_month" [:- [:field-id 13] [:interval 1 :month] [:interval 1 :day]]}}}
- {:query {:expressions {:datetime-diff ["datetime-diff" ["field" 1 nil] ["field" 2 nil] "month"]}}}
- {:query {:expressions {"datetime-diff" [:datetime-diff [:field 1 nil] [:field 2 nil] :month]}}}
+ {:query {:expressions {:datetime-diff ["datetime-diff" ["field" 1 nil] ["field" 2 nil] "month"]}}}
+ {:query {:expressions {"datetime-diff" [:datetime-diff [:field 1 nil] [:field 2 nil] :month]}}}
- {:query {:expressions {:datetime-add ["datetime-add" ["field" 1 nil] 1 "month"]}}}
- {:query {:expressions {"datetime-add" [:datetime-add [:field 1 nil] 1 :month]}}}
+ {:query {:expressions {:datetime-add ["datetime-add" ["field" 1 nil] 1 "month"]}}}
+ {:query {:expressions {"datetime-add" [:datetime-add [:field 1 nil] 1 :month]}}}
- {:query {:expressions {:datetime-subtract ["datetime-subtract" ["field" 1 nil] 1 "month"]}}}
- {:query {:expressions {"datetime-subtract" [:datetime-subtract [:field 1 nil] 1 :month]}}}}
+ {:query {:expressions {:datetime-subtract ["datetime-subtract" ["field" 1 nil] 1 "month"]}}}
+ {:query {:expressions {"datetime-subtract" [:datetime-subtract [:field 1 nil] 1 :month]}}}}
- "expressions handle namespaced keywords correctly"
- {{:query {"expressions" {:abc/def ["+" 1 2]}
- :fields [["expression" :abc/def]]}}
- {:query {:expressions {"abc/def" [:+ 1 2]}
- :fields [[:expression "abc/def"]]}}}))
+ "expressions handle namespaced keywords correctly"
+ {{:query {"expressions" {:abc/def ["+" 1 2]}
+ :fields [["expression" :abc/def]]}}
+ {:query {:expressions {"abc/def" [:+ 1 2]}
+ :fields [[:expression "abc/def"]]}}}
+
+ "expression refs can have opts (#33528)"
+ {{:query {:expressions {"abc" [+ 1 2]}
+ :fields [[:expression "abc" {"base-type" "type/Number"}]]}}
+ {:query {:expressions {"abc" [+ 1 2]}
+ :fields [[:expression "abc" {:base-type :type/Number}]]}}}))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1378,7 +1384,7 @@
(t/deftest ^:parallel normalize-fragment-filter-test
(t/is (= [:!=
- [:expression "expr"]
+ [:expression "expr" {:base-type :type/Date}]
[:field 66302 {:base-type :type/DateTime}]]
(mbql.normalize/normalize-fragment
[:query :filter]
diff --git a/test/metabase/query_processor_test/date_bucketing_test.clj b/test/metabase/query_processor_test/date_bucketing_test.clj
index a21831b4f83..fbf4c4f4d15 100644
--- a/test/metabase/query_processor_test/date_bucketing_test.clj
+++ b/test/metabase/query_processor_test/date_bucketing_test.clj
@@ -23,6 +23,7 @@
[metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.driver.sql.query-processor-test-util :as sql.qp-test-util]
+ [metabase.lib.convert :as lib.convert]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.jvm :as lib.metadata.jvm]
@@ -1343,6 +1344,28 @@
[[0]])
(mt/formatted-rows [int] (qp/process-query query)))))))))
+(deftest filter-by-expression-time-interval-test
+ (testing "Datetime expressions can filter to a date range (#33528)"
+ (mt/test-drivers (mt/normal-drivers-except #{:snowflake :athena})
+ (mt/dataset
+ checkins:1-per-day
+ (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+ query (as-> (lib/query mp (lib.metadata/table mp (mt/id :checkins))) $q
+ (lib/expression $q "customdate" (m/find-first (comp #{(mt/id :checkins :timestamp)} :id) (lib/visible-columns $q)))
+ (lib/filter $q (lib/time-interval (lib/expression-ref $q "customdate") :current :week)))
+ mbql-query (mt/mbql-query
+ checkins
+ {:expressions {"customdate" $timestamp}
+ :filter [:time-interval [:expression "customdate" {:base-type :type/DateTime}] :current :week]})
+ processed (qp/process-query query)
+ mbql-processed (qp/process-query mbql-query)]
+ ;; Test both path ways since only mbql-queries were affected.
+ (is (= 7 (count (mt/rows processed))))
+ (is (= 7 (count (mt/rows mbql-processed))))
+ (is (= (get-in (qp/process-query mbql-query) [:data :native_form])
+ (get-in (qp/process-query (lib.convert/->pMBQL mbql-query)) [:data :native_form])
+ (get-in (qp/process-query query) [:data :native_form]))))))))
+
;; TODO -- is this really date BUCKETING? Does this BELONG HERE?!
(deftest june-31st-test
(testing "What happens when you try to add 3 months to March 31st? It should still work (#10072, #21968, #21969)"
--
GitLab