From 72a201819b3255c3753591297f90f05819d200cc Mon Sep 17 00:00:00 2001
From: lbrdnk <lbrdnk@users.noreply.github.com>
Date: Thu, 1 Aug 2024 11:12:14 +0200
Subject: [PATCH] Add `:relative-time-interval` mbql function (#46211)
* Add :relative-time-interval mbql function
* Add relativeDateFilterPartsRelativeTimeInterval
* Update display-name-method :relative-time-interval
* Update desugar-relative-time-interval
* Define relative-time-filter op
* Add or update tests
* Update tests
* Update display-name-method
* Update test
* Fix positive relative-time-interval shift
---
frontend/src/metabase-lib/filter.ts | 63 ++++++++++++++-----
frontend/src/metabase-lib/types.ts | 1 +
src/metabase/legacy_mbql/normalize.cljc | 6 ++
src/metabase/legacy_mbql/schema.cljc | 9 ++-
src/metabase/legacy_mbql/util.cljc | 22 +++++++
src/metabase/lib/convert.cljc | 4 ++
src/metabase/lib/core.cljc | 1 +
src/metabase/lib/filter.cljc | 13 ++++
src/metabase/lib/schema/filter.cljc | 12 ++++
test/metabase/legacy_mbql/normalize_test.cljc | 4 ++
test/metabase/legacy_mbql/util_test.cljc | 46 ++++++++++++++
test/metabase/lib/filter_test.cljc | 6 +-
test/metabase/lib/js_test.cljs | 3 +
test/metabase/lib/schema/filter_test.cljc | 1 +
.../middleware/desugar_test.clj | 7 +++
.../date_bucketing_test.clj | 48 ++++++++++++++
16 files changed, 230 insertions(+), 16 deletions(-)
diff --git a/frontend/src/metabase-lib/filter.ts b/frontend/src/metabase-lib/filter.ts
index 6d8a4720993..45d39967710 100644
--- a/frontend/src/metabase-lib/filter.ts
+++ b/frontend/src/metabase-lib/filter.ts
@@ -347,19 +347,12 @@ export function relativeDateFilterClause({
);
}
- return expressionClause("between", [
- expressionClause("+", [
- columnWithoutBucket,
- expressionClause("interval", [-offsetValue, offsetBucket]),
- ]),
- expressionClause("relative-datetime", [
- value !== "current" && value < 0 ? value : 0,
- bucket,
- ]),
- expressionClause("relative-datetime", [
- value !== "current" && value > 0 ? value : 0,
- bucket,
- ]),
+ return expressionClause("relative-time-interval", [
+ columnWithoutBucket,
+ value,
+ bucket,
+ offsetValue,
+ offsetBucket,
]);
}
@@ -371,7 +364,8 @@ export function relativeDateFilterParts(
const filterParts = expressionParts(query, stageIndex, filterClause);
return (
relativeDateFilterPartsWithoutOffset(filterParts) ??
- relativeDateFilterPartsWithOffset(filterParts)
+ relativeDateFilterPartsWithOffset(filterParts) ??
+ relativeDateFilterPartsRelativeTimeInterval(filterParts)
);
}
@@ -820,6 +814,47 @@ function relativeDateFilterPartsWithOffset({
};
}
+function relativeDateFilterPartsRelativeTimeInterval({
+ operator,
+ args,
+ options,
+}: ExpressionParts): RelativeDateFilterParts | null {
+ if (operator !== "relative-time-interval" || args.length !== 5) {
+ return null;
+ }
+
+ const [column, value, bucket, offsetValue, offsetBucket] = args;
+
+ if (!isColumnMetadata(column) || !isTemporal(column)) {
+ return null;
+ }
+
+ if (
+ !isNumberLiteral(value) ||
+ !isStringLiteral(bucket) ||
+ !isRelativeDateBucket(bucket)
+ ) {
+ return null;
+ }
+
+ if (
+ !isNumberLiteral(offsetValue) ||
+ !isStringLiteral(offsetBucket) ||
+ !isRelativeDateBucket(offsetBucket)
+ ) {
+ return null;
+ }
+
+ return {
+ column,
+ bucket,
+ value,
+ offsetBucket,
+ offsetValue,
+ options,
+ };
+}
+
function serializeExcludeDatePart(
value: number,
bucketName: ExcludeDateBucketName,
diff --git a/frontend/src/metabase-lib/types.ts b/frontend/src/metabase-lib/types.ts
index 4fb05ecd52b..473224c3fe9 100644
--- a/frontend/src/metabase-lib/types.ts
+++ b/frontend/src/metabase-lib/types.ts
@@ -275,6 +275,7 @@ export type ExpressionOperatorName =
| "concat"
| "interval"
| "time-interval"
+ | "relative-time-interval"
| "relative-datetime"
| "inside"
| "segment"
diff --git a/src/metabase/legacy_mbql/normalize.cljc b/src/metabase/legacy_mbql/normalize.cljc
index ee7191e215d..68f7db1af26 100644
--- a/src/metabase/legacy_mbql/normalize.cljc
+++ b/src/metabase/legacy_mbql/normalize.cljc
@@ -159,6 +159,12 @@
(maybe-normalize-token amount))
(maybe-normalize-token unit)]))
+(defmethod normalize-mbql-clause-tokens :relative-time-interval
+ [[_ col & [_value _bucket _offset-value _offset-bucket :as args]]]
+ (into [:relative-time-interval (normalize-tokens col :ignore-path)]
+ (map maybe-normalize-token)
+ args))
+
(defmethod normalize-mbql-clause-tokens :relative-datetime
;; Normalize a `relative-datetime` clause. `relative-datetime` comes in two flavors:
;;
diff --git a/src/metabase/legacy_mbql/schema.cljc b/src/metabase/legacy_mbql/schema.cljc
index be2d2e6233c..d12e1102118 100644
--- a/src/metabase/legacy_mbql/schema.cljc
+++ b/src/metabase/legacy_mbql/schema.cljc
@@ -858,6 +858,13 @@
unit [:ref ::RelativeDatetimeUnit]
options (optional TimeIntervalOptions))
+(defclause ^:sugar relative-time-interval
+ col Field
+ value :int
+ bucket [:ref ::RelativeDatetimeUnit]
+ offset-value :int
+ offset-bucket [:ref ::RelativeDatetimeUnit])
+
;; A segment is a special `macro` that saves some pre-definied filter clause, e.g. [:segment 1]
;; this gets replaced by a normal Filter clause in MBQL macroexpansion
;;
@@ -890,7 +897,7 @@
;; filters drivers must implement
and or not = != < > <= >= between starts-with ends-with contains
;; SUGAR filters drivers do not need to implement
- does-not-contain inside is-empty not-empty is-null not-null time-interval segment)]])
+ does-not-contain inside is-empty not-empty is-null not-null relative-time-interval time-interval segment)]])
(def ^:private CaseClause
[:tuple {:error/message ":case subclause"} Filter ExpressionArg])
diff --git a/src/metabase/legacy_mbql/util.cljc b/src/metabase/legacy_mbql/util.cljc
index 4db4f6725a1..a9d802702db 100644
--- a/src/metabase/legacy_mbql/util.cljc
+++ b/src/metabase/legacy_mbql/util.cljc
@@ -234,6 +234,27 @@
[:relative-datetime 1 unit]
[:relative-datetime n unit]]))
+(defn desugar-relative-time-interval
+ "Transform `:relative-time-interval` to `:and` expression."
+ [m]
+ (lib.util.match/replace
+ m
+ [:relative-time-interval col value bucket offset-value offset-bucket]
+ (let [col-default-bucket (cond-> col (and (vector? col) (= 3 (count col)))
+ (update 2 assoc :temporal-unit :default))
+ offset [:interval offset-value offset-bucket]
+ lower-bound (if (neg? value)
+ [:relative-datetime value bucket]
+ [:relative-datetime 1 bucket])
+ upper-bound (if (neg? value)
+ [:relative-datetime 0 bucket]
+ [:relative-datetime (inc value) bucket])
+ lower-with-offset [:+ lower-bound offset]
+ upper-with-offset [:+ upper-bound offset]]
+ [:and
+ [:>= col-default-bucket lower-with-offset]
+ [:< col-default-bucket upper-with-offset]])))
+
(defn desugar-does-not-contain
"Rewrite `:does-not-contain` filter clauses as simpler `[:not [:contains ...]]` clauses.
@@ -376,6 +397,7 @@
desugar-multi-argument-comparisons
desugar-does-not-contain
desugar-time-interval
+ desugar-relative-time-interval
desugar-is-null-and-not-null
desugar-is-empty-and-not-empty
desugar-inside
diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc
index 93409bf88ab..f1bd5d8a3d7 100644
--- a/src/metabase/lib/convert.cljc
+++ b/src/metabase/lib/convert.cljc
@@ -290,6 +290,10 @@
[[_tag field n unit options]]
(lib.options/ensure-uuid [:time-interval (or options {}) (->pMBQL field) n unit]))
+(defmethod ->pMBQL :relative-time-interval
+ [[_tag & [_column _value _bucket _offset-value _offset-bucket :as args]]]
+ (lib.options/ensure-uuid (into [:relative-time-interval {}] (map ->pMBQL) args)))
+
;; `:offset` is the same in legacy and pMBQL, but we need to update the expr it wraps.
(defmethod ->pMBQL :offset
[[tag opts expr n, :as clause]]
diff --git a/src/metabase/lib/core.cljc b/src/metabase/lib/core.cljc
index bf55a03ffef..e4458eb9e7a 100644
--- a/src/metabase/lib/core.cljc
+++ b/src/metabase/lib/core.cljc
@@ -216,6 +216,7 @@
is-empty not-empty
starts-with ends-with
contains does-not-contain
+ relative-time-interval
time-interval
segment]
[lib.filter.update
diff --git a/src/metabase/lib/filter.cljc b/src/metabase/lib/filter.cljc
index 7b24892df89..f53ee387a4d 100644
--- a/src/metabase/lib/filter.cljc
+++ b/src/metabase/lib/filter.cljc
@@ -275,6 +275,18 @@
(lib.metadata.calculation/display-name query stage-number expr style)
(u/lower-case-en (lib.temporal-bucket/describe-temporal-interval n unit)))))
+(defmethod lib.metadata.calculation/display-name-method :relative-time-interval
+ [query stage-number [_tag _opts column value bucket offset-value offset-bucket] style]
+ (if (neg? offset-value)
+ (i18n/tru "{0} is in the {1}, starting {2} ago"
+ (lib.metadata.calculation/display-name query stage-number column style)
+ (u/lower-case-en (lib.temporal-bucket/describe-temporal-interval value bucket))
+ (inflections/pluralize (abs offset-value) (name offset-bucket)))
+ (i18n/tru "{0} is in the {1}, starting {2} from now"
+ (lib.metadata.calculation/display-name query stage-number column style)
+ (u/lower-case-en (lib.temporal-bucket/describe-temporal-interval value bucket))
+ (inflections/pluralize (abs offset-value) (name offset-bucket)))))
+
(defmethod lib.metadata.calculation/display-name-method :relative-datetime
[_query _stage-number [_tag _opts n unit] _style]
(i18n/tru "{0}" (lib.temporal-bucket/describe-temporal-interval n unit)))
@@ -302,6 +314,7 @@
(lib.common/defop ends-with [whole part])
(lib.common/defop contains [whole part])
(lib.common/defop does-not-contain [whole part])
+(lib.common/defop relative-time-interval [x value bucket offset-value offset-bucket])
(lib.common/defop time-interval [x amount unit])
(lib.common/defop segment [segment-id])
diff --git a/src/metabase/lib/schema/filter.cljc b/src/metabase/lib/schema/filter.cljc
index f1530c1940a..86a614e1006 100644
--- a/src/metabase/lib/schema/filter.cljc
+++ b/src/metabase/lib/schema/filter.cljc
@@ -112,6 +112,18 @@
[false [:ref ::expression/integer]]]
#_unit [:ref ::temporal-bucketing/unit.date-time.interval]])
+(mbql-clause/define-mbql-clause :relative-time-interval :- :type/Boolean
+ [:tuple
+ [:= {:decode/normalize common/normalize-keyword} :relative-time-interval]
+ ;; `relative-time-interval` does not support options to eg. include/exclude start or end point. Only int values
+ ;; are allowed for intervals.
+ ::common/options
+ #_col [:ref ::expression/temporal]
+ #_value :int
+ #_bucket [:ref ::temporal-bucketing/unit.date-time.interval]
+ #_offset-value :int
+ #_offset-bucket [:ref ::temporal-bucketing/unit.date-time.interval]])
+
;; segments are guaranteed to return valid filter clauses and thus booleans, right?
(mbql-clause/define-mbql-clause :segment :- :type/Boolean
[:tuple
diff --git a/test/metabase/legacy_mbql/normalize_test.cljc b/test/metabase/legacy_mbql/normalize_test.cljc
index 16a33bbfb09..20e2f1c1158 100644
--- a/test/metabase/legacy_mbql/normalize_test.cljc
+++ b/test/metabase/legacy_mbql/normalize_test.cljc
@@ -196,6 +196,10 @@
{{:query {"FILTER" ["time-interval" 10 -10 "day"]}}
{:query {:filter [:time-interval 10 -10 :day]}}}
+ "relative-time-interval is correctly normalized"
+ {{:query {"FILTER" ["relative-time-interval" 10 "week" -10 "week"]}}
+ {:query {:filter [:relative-time-interval 10 :week -10 :week]}}}
+
"make sure we support time-interval options"
{["TIME_INTERVAL" 10 -30 "DAY" {"include_current" true}]
[:time-interval 10 -30 :day {:include-current true}]}
diff --git a/test/metabase/legacy_mbql/util_test.cljc b/test/metabase/legacy_mbql/util_test.cljc
index b446a086d24..8afebb7e1f5 100644
--- a/test/metabase/legacy_mbql/util_test.cljc
+++ b/test/metabase/legacy_mbql/util_test.cljc
@@ -276,6 +276,52 @@
(mbql.u/desugar-filter-clause [:time-interval [:expression "CC"] :current :week]))
"keywords like `:current` should work correctly"))
+(t/deftest ^:parallel desugar-relative-time-interval-negative-test
+ (t/testing "Desugaring relative-date-time produces expected [:and [:>=..] [:<..]] expression"
+ (let [value -10
+ bucket :day
+ offset-value -8
+ offset-bucket :week
+ exp-offset [:interval offset-value offset-bucket]]
+ (t/testing "expression reference is transformed correctly"
+ (let [expr-ref [:expression "cc"]]
+ (t/is (= [:and
+ [:>= expr-ref [:+ [:relative-datetime value bucket] exp-offset]]
+ [:< expr-ref [:+ [:relative-datetime 0 bucket] exp-offset]]]
+ (mbql.u/desugar-filter-clause
+ [:relative-time-interval expr-ref value bucket offset-value offset-bucket])))))
+ (t/testing "field reference is transformed correctly"
+ (let [field-ref [:field 100 nil]
+ exp-field-ref (update field-ref 2 assoc :temporal-unit :default)]
+ (t/is (= [:and
+ [:>= exp-field-ref [:+ [:relative-datetime value bucket] exp-offset]]
+ [:< exp-field-ref [:+ [:relative-datetime 0 bucket] exp-offset]]]
+ (mbql.u/desugar-filter-clause
+ [:relative-time-interval exp-field-ref value bucket offset-value offset-bucket]))))))))
+
+(t/deftest ^:parallel desugar-relative-time-interval-positive-test
+ (t/testing "Desugaring relative-date-time produces expected [:and [:>=..] [:<..]] expression"
+ (let [value 10
+ bucket :day
+ offset-value 8
+ offset-bucket :week
+ exp-offset [:interval offset-value offset-bucket]]
+ (t/testing "expression reference is transformed correctly"
+ (let [expr-ref [:expression "cc"]]
+ (t/is (= [:and
+ [:>= expr-ref [:+ [:relative-datetime 1 bucket] exp-offset]]
+ [:< expr-ref [:+ [:relative-datetime (inc value) bucket] exp-offset]]]
+ (mbql.u/desugar-filter-clause
+ [:relative-time-interval expr-ref value bucket offset-value offset-bucket])))))
+ (t/testing "field reference is transformed correctly"
+ (let [field-ref [:field 100 nil]
+ exp-field-ref (update field-ref 2 assoc :temporal-unit :default)]
+ (t/is (= [:and
+ [:>= exp-field-ref [:+ [:relative-datetime 1 bucket] exp-offset]]
+ [:< exp-field-ref [:+ [:relative-datetime (inc value) bucket] exp-offset]]]
+ (mbql.u/desugar-filter-clause
+ [:relative-time-interval exp-field-ref value bucket offset-value offset-bucket]))))))))
+
(t/deftest ^:parallel desugar-relative-datetime-with-current-test
(t/testing "when comparing `:relative-datetime`to `:field`, it should take the temporal unit of the `:field`"
(t/is (= [:=
diff --git a/test/metabase/lib/filter_test.cljc b/test/metabase/lib/filter_test.cljc
index a80283240dd..16c5ea786eb 100644
--- a/test/metabase/lib/filter_test.cljc
+++ b/test/metabase/lib/filter_test.cljc
@@ -701,7 +701,11 @@
(lib/expression-clause :interval [-1 :month] nil)] nil)
(lib/expression-clause :relative-datetime [0 :month] nil)
(lib/expression-clause :relative-datetime [1 :month] nil)],
- :name "Created At is in the next month, starting 1 month from now"}])))
+ :name "Created At is in the next month, starting 1 month from now"}
+ {:clause [:relative-time-interval created-at 10 :week 10 :week]
+ :name "Created At is in the next 10 weeks, starting 10 weeks from now"}
+ {:clause [:relative-time-interval created-at -10 :week -10 :week]
+ :name "Created At is in the previous 10 weeks, starting 10 weeks ago"}])))
(deftest ^:parallel specific-date-frontend-filter-display-names-test
(let [created-at (meta/field-metadata :products :created-at)]
diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs
index b4233c3d461..f034b28bea0 100644
--- a/test/metabase/lib/js_test.cljs
+++ b/test/metabase/lib/js_test.cljs
@@ -395,6 +395,9 @@
[:time-interval {} [:field {} int?] 10 :day]
(lib.js/expression-clause "time-interval" [(meta/field-metadata :products :created-at) 10 "day"] nil)
+ [:relative-time-interval {} [:field {} int?] 10 :day 10 :month]
+ (lib.js/expression-clause "time-interval" [(meta/field-metadata :products :created-at) 10 "day" 10 "month"] nil)
+
[:relative-datetime {} :current :day]
(lib.js/expression-clause "relative-datetime" ["current" "day"] nil)
diff --git a/test/metabase/lib/schema/filter_test.cljc b/test/metabase/lib/schema/filter_test.cljc
index 3c7fb225b07..fb94c789cf9 100644
--- a/test/metabase/lib/schema/filter_test.cljc
+++ b/test/metabase/lib/schema/filter_test.cljc
@@ -71,6 +71,7 @@
[:time-interval field :last :hour]
[:time-interval field 4 :hour]
[:time-interval {:include-current true} field :next :day]
+ [:relative-time-interval field 10 :day 10 :day]
[:segment 1]]]
(doseq [op (filter-ops filter-expr)]
(is (not (identical? (get-method expression/type-of-method op)
diff --git a/test/metabase/query_processor/middleware/desugar_test.clj b/test/metabase/query_processor/middleware/desugar_test.clj
index a7ffbe9802f..b956ed1b792 100644
--- a/test/metabase/query_processor/middleware/desugar_test.clj
+++ b/test/metabase/query_processor/middleware/desugar_test.clj
@@ -16,6 +16,12 @@
[:field 2 {:temporal-unit :day}]
[:relative-datetime -30 :day]
[:relative-datetime -1 :day]]
+ [:>=
+ [:field 2 {:temporal-unit :default}]
+ [:+ [:relative-datetime -30 :day] [:interval -30 :day]]]
+ [:<
+ [:field 2 {:temporal-unit :default}]
+ [:+ [:relative-datetime 0 :day] [:interval -30 :day]]]
[:!= [:field 3 nil] "(not set)"]
[:!= [:field 3 nil] "url"]
[:> [:temporal-extract [:field 4 nil] :year-of-era] [:/ [:/ 1 2] 3]]]
@@ -38,6 +44,7 @@
:filter [:and
[:= [:field 1 nil] "Run Query"]
[:time-interval [:field 2 nil] -30 :day]
+ [:relative-time-interval [:field 2 nil] -30 :day -30 :day]
[:!= [:field 3 nil] "(not set)" "url"]
[:> [:get-year [:field 4 nil]] [:/ 1 2 3]]]
:expressions {"year" [:+
diff --git a/test/metabase/query_processor_test/date_bucketing_test.clj b/test/metabase/query_processor_test/date_bucketing_test.clj
index 6e469bb1526..c2f0ebe0961 100644
--- a/test/metabase/query_processor_test/date_bucketing_test.clj
+++ b/test/metabase/query_processor_test/date_bucketing_test.clj
@@ -1052,6 +1052,29 @@
(is (= 7
(count (mt/rows (qp/process-query query)))))))))
+(deftest ^:parallel relative-time-interval-test
+ (mt/test-drivers
+ (disj (mt/normal-drivers-with-feature :date-arithmetics) :athena)
+ ;; Following verifies #45942 is solved. Changing the offset ensures that intervals do not overlap.
+ (testing "Syntactic sugar (`:relative-time-interval` clause) (#45942)"
+ (mt/dataset
+ checkins:1-per-day
+ (is (= 7
+ (ffirst
+ (mt/formatted-rows
+ [int]
+ (mt/run-mbql-query
+ checkins
+ {:aggregation [[:count]]
+ :filter [:relative-time-interval $timestamp -1 :week -1 :week]})))
+ (ffirst
+ (mt/formatted-rows
+ [int]
+ (mt/run-mbql-query
+ checkins
+ {:aggregation [[:count]]
+ :filter [:relative-time-interval $timestamp -1 :week 0 :week]})))))))))
+
;; Make sure that when referencing the same field multiple times with different units we return the one that actually
;; reflects the units the results are in. eg when we breakout by one unit and filter by another, make sure the results
;; and the col info use the unit used by breakout
@@ -1407,6 +1430,31 @@
(get-in (qp/process-query (lib.convert/->pMBQL mbql-query)) [:data :native_form])
(get-in (qp/process-query query) [:data :native_form]))))))))
+(deftest filter-by-expression-relative-time-interval-test
+ (testing "Datetime expressions can filter to a date range"
+ (mt/test-drivers
+ (disj (mt/normal-drivers-with-feature :date-arithmetics) :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/relative-time-interval
+ (lib/expression-ref $q "customdate") -1 :week -1 :week)))
+ mbql-query (mt/mbql-query
+ checkins
+ {:expressions {"customdate" $timestamp}
+ :filter [:relative-time-interval
+ [:expression "customdate" {:base-type :type/DateTime}] -1 :week -1 :week]})
+ processed (qp/process-query query)
+ mbql-processed (qp/process-query mbql-query)]
+ (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