Skip to content

Commit

Permalink
Fixes filters against datetime binned expressions
Browse files Browse the repository at this point in the history 
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 `=`.
  • Loading branch information
@snoe
snoe committed Feb 16, 2024
1 parent 227f0b8 commit 66232d2
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 44 deletions.
32 changes: 19 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,24 @@
(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 (normalize-ref-opts opts)]
(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 +115,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
68 changes: 37 additions & 31 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"]]}}}

"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]]}}}

{: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-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]}}}}

"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 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]]}}}}

"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 {: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-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"]]}}}

"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
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 @@ -1284,6 +1285,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)
(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 66232d2

Please sign in to comment.