Skip to content
Snippets Groups Projects
Unverified Commit 559842fc authored by Cam Saul's avatar Cam Saul Committed by GitHub
Browse files

Fix legacy auto datetime bucketing filtering against relative-datetime (#11025)

parent 10037cda
Branches
Tags
No related merge requests found
......@@ -43,6 +43,10 @@
(and (string? x)
(re-matches #"^\d{4}-\d{2}-\d{2}$" x)))
(defn- auto-bucketable-value? [v]
(or (yyyy-MM-dd-date-string? v)
(mbql.u/is-clause? :relative-datetime v)))
(defn- should-not-be-autobucketed?
"Is `x` a clause (or a clause that contains a clause) that we should definitely not autobucket?"
[x]
......@@ -51,14 +55,14 @@
(when (and (mbql.preds/Filter? x)
(not (mbql.u/is-clause? #{:and :or :not} x)))
(or
;; * is not and equality or comparison filter. e.g. wouldn't make sense to bucket a field and then check if it is
;; * is not an equality or comparison filter. e.g. wouldn't make sense to bucket a field and then check if it is
;; `NOT NULL`
(not (mbql.u/is-clause? #{:= :!= :< :> :<= :>= :between} x))
;; * has arguments that aren't `yyyy-MM-dd` date strings. The only reason we auto-bucket datetime Fields in the
;; * first place is for legacy reasons, if someone is specifying additional info like hour/minute then we
;; * shouldn't assume they want to bucket by day
(let [[_ _ & vs] x]
(not (every? yyyy-MM-dd-date-string? vs)))))
(not (every? auto-bucketable-value? vs)))))
;; do not autobucket field-ids that are already wrapped by another Field clause like `datetime-field` or
;; `binning-strategy`
(and (mbql.preds/Field? x)
......
(ns metabase.query-processor.middleware.auto-bucket-datetimes-test
(:require [expectations :refer [expect]]
(:require [clojure.test :refer :all]
[expectations :refer [expect]]
[metabase.models.field :refer [Field]]
[metabase.query-processor.middleware.auto-bucket-datetimes :as auto-bucket-datetimes]
[metabase.test.data :as data]
......@@ -184,3 +185,15 @@
(auto-bucket-mbql
{:source-table $$incidents
:breakout [$timestamp]}))))
(deftest relative-datetime-test
(is (= (->
(data/mbql-query checkins
{:filter [:= [:datetime-field $date :day] [:relative-datetime :current]]})
:query :filter)
(->
(auto-bucket
(data/mbql-query checkins
{:filter [:= $date [:relative-datetime :current]]}))
:query :filter))
"Fields being compared against `:relative-datetime`s should be subject to auto-bucketing. (#9014)"))
......@@ -22,6 +22,7 @@
[test :refer :all]]
[metabase
[driver :as driver]
[query-processor :as qp]
[query-processor-test :as qp.test]
[util :as u]]
[metabase.driver.sql.query-processor :as sql.qp]
......@@ -999,3 +1000,15 @@
(is (contains? expected-count result)
(format "count of rows where (= (%s date) %s) should be one of: %s"
(name unit) filter-value (str/join ", " (sort expected-count)))))))))))
(deftest legacy-default-datetime-bucketing-test
(is (= (str "SELECT count(*) AS \"count\" "
"FROM \"PUBLIC\".\"CHECKINS\" "
"WHERE CAST(\"PUBLIC\".\"CHECKINS\".\"DATE\" AS date) = CAST(now() AS date)")
(:query
(qp/query->native
(data/mbql-query checkins
{:aggregation [[:count]]
:filter [:= $date [:relative-datetime :current]]}))))
(str "Datetime fields that aren't wrapped in datetime-field clauses should get default :day bucketing for legacy "
"reasons. See #9014")))
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment