Skip to content

Commit

Permalink
Fixes filters against datetime binned expressions (#38874) (#38975)
Browse files Browse the repository at this point in the history 
* 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

Co-authored-by: Case Nelson <case@metabase.com>
  • Loading branch information
@metabase-bot @snoe
metabase-bot[bot] and snoe committed Feb 27, 2024
1 parent 2fdce0c commit 07262ba
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 38 deletions.
38 changes: 25 additions & 13 deletions src/metabase/mbql/normalize.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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."
Expand Down
56 changes: 31 additions & 25 deletions test/metabase/mbql/normalize_test.cljc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,37 +555,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}]]}}}))


;;; +----------------------------------------------------------------------------------------------------------------+
Expand Down Expand Up @@ -1375,7 +1381,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]
Expand Down
23 changes: 23 additions & 0 deletions test/metabase/query_processor_test/date_bucketing_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -1342,6 +1343,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)"
Expand Down

0 comments on commit 07262ba

Please sign in to comment.