diff --git a/.clj-kondo/src/hooks/metabase/test/data.clj b/.clj-kondo/src/hooks/metabase/test/data.clj index b865350393745ff04227db0425f71b4c90f40c21..bc93985a65bedae9c9604d73cf368ccc0e632f1d 100644 --- a/.clj-kondo/src/hooks/metabase/test/data.clj +++ b/.clj-kondo/src/hooks/metabase/test/data.clj @@ -11,6 +11,10 @@ [] (not-empty (set (keys (:clj (hooks/ns-analysis 'metabase.test.data.dataset-definitions)))))) +(defn- local-def? + [ns* sexpr] + (= ns* (:ns (hooks/resolve {:name sexpr})))) + (defn- dataset-type "`dataset` can be one of: @@ -21,7 +25,7 @@ We can only determine if an unqualified symbol refers to something in the dataset definitions namespace if there are cached results available from [[global-dataset-symbols]]." - [dataset] + [this-ns dataset] (let [sexpr (hooks/sexpr dataset) global-defs (global-dataset-symbols)] (cond @@ -31,20 +35,19 @@ (namespace sexpr) :qualified - (empty? global-defs) - :unqualified/unknown - (contains? global-defs sexpr) :unqualified/from-dataset-defs-namespace - ;; either something defined in the current namespace or let-bound in the current scope. + (local-def? this-ns sexpr) + :unqualified/local-def + :else - :unqualified/local-def))) + :unqualified/unknown))) (defn dataset - [{{[_ dataset & body] :children} :node}] + [{{[_ dataset & body] :children} :node this-ns :ns}] (let [noop (constantly nil) - body (case (dataset-type dataset) + body (case (dataset-type this-ns dataset) ;; non-symbol, qualified symbols, and unqualified symbols from the current namespace/let-bound can all ;; get converted from something like ;; diff --git a/bin/kondo.sh b/bin/kondo.sh index 2946b3594f3e0d5f54202aec3db5665f146ab321..a241744fa68b29728715fa8f20d7332fe4c12170 100755 --- a/bin/kondo.sh +++ b/bin/kondo.sh @@ -11,6 +11,9 @@ clj-kondo --copy-configs --dependencies --lint "$(clojure -A:dev -Spath)" --skip rm -rf .clj-kondo/metosin/malli-types-clj/ rm -rf .clj-kondo/.cache +# Initialize cache required for `hooks.metabase.test.data/dataset` hook. +clj-kondo --dependencies --lint "test/metabase/test/data/dataset_definitions.clj" + # Run Kondo against all of our Clojure files in the various directories they might live. find modules/drivers enterprise/backend \ -maxdepth 2 \ diff --git a/frontend/src/metabase-lib/filter.ts b/frontend/src/metabase-lib/filter.ts index 6d8a47209935f26696ca2accaf1b8a741a04de47..45d399677106f4757bf396d217eea54bcb6247c4 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 4fb05ecd52b3b7d0e536dde70cbff7a811d7b6f5..473224c3fe95544c7d373381e7f9ffc36dd5f0e1 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 ee7191e215d2afb467cb8898842efdf6b89f986f..68f7db1af265616e3cd7420384840d9356eabec8 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 be2d2e6233c342d8bfda871965606034e958add0..d12e11021183710c09bdc5af1ff636cc7da12b69 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 4db4f6725a1e24c8ef583a8956be113d99bd4c87..a9d802702db2edb4bf2c8100d93a9ac3e1ff85be 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 93409bf88ab05438c9baa3867ce106a886a40672..f1bd5d8a3d7078861aa834a0fcd4abccc09d619b 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 bf55a03ffef0275e3aa206bbeaa0f96b0fbec982..e4458eb9e7ab7e33499951b928a9c4b287b111a0 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 7b24892df8907d1f55b97fd87f7f27736296cf89..f53ee387a4de363a930cde691db153fca6da5eaf 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 f1530c1940a5673c59d444779be883ee84e38bde..86a614e10064acc51e35dc22a49b7e45c633c601 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/src/metabase/query_processor/middleware/optimize_temporal_filters.clj b/src/metabase/query_processor/middleware/optimize_temporal_filters.clj index 50ac3f3a3c9de17d900a84e4e4ba86062796e6f2..33db5ce396239a6cada9caa7b5f767e72ca50353 100644 --- a/src/metabase/query_processor/middleware/optimize_temporal_filters.clj +++ b/src/metabase/query_processor/middleware/optimize_temporal_filters.clj @@ -111,6 +111,36 @@ (temporal-value :guard optimizable-temporal-value?)] (field-and-temporal-value-have-compatible-units? field temporal-value))) +(defn- not-default-bucket-clause + [clause] + (and (vector? clause) + (not= :default (get-in clause [2 :temporal-unit])))) + +;; TODO: I believe we do not generate __filter clauses that have default temporal bucket on column arg which should be +;; optimized__. Unfortunately I'm not certain about that. If I was, the following `can-optimize-filter? :>=` and +;; `can-optimize-filter? :>=` definitions would be redundant after update of `optimizable-expr?`, ie. changing +;; the logic to something along "if `expr` has default temporal unit we should not optimize". + +(defmethod can-optimize-filter? :>= + [filter-clause] + (lib.util.match/match-one + filter-clause + [_tag + ;; Don't optimize >= with column that has default temporal bucket + (field :guard (every-pred not-default-bucket-clause optimizable-expr?)) + (temporal-value :guard optimizable-temporal-value?)] + (field-and-temporal-value-have-compatible-units? field temporal-value))) + +(defmethod can-optimize-filter? :< + [filter-clause] + (lib.util.match/match-one + filter-clause + [_tag + ;; Don't optimize < with column that has default temporal bucket + (field :guard (every-pred not-default-bucket-clause optimizable-expr?)) + (temporal-value :guard optimizable-temporal-value?)] + (field-and-temporal-value-have-compatible-units? field temporal-value))) + (defmethod can-optimize-filter? :between [filter-clause] (lib.util.match/match-one filter-clause diff --git a/test/metabase/legacy_mbql/normalize_test.cljc b/test/metabase/legacy_mbql/normalize_test.cljc index 16a33bbfb09fd6b3f950717d7a5bc50fa5efe5ab..20e2f1c1158bbe383bc1f7d65f75b6833cc12652 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 b446a086d248d5b1a4c9b0617749f5ae4940660d..8afebb7e1f5f50cb30b8e9c3f826a730bf44bd56 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 a80283240dd26cb6451467bb15cf1821a6a51644..16c5ea786eb4539026ed845b65ed92513b3f595f 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 b4233c3d461b37eb81530afca11cc30d037e87fe..f034b28bea0a0d212c160b51aed26c9864ca2998 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 3c7fb225b07ac92fcd92aae8261c0197ec794747..fb94c789cf983221b82677caa757c37f8e7e0988 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 a7ffbe9802fbf76e9d2f3015e5a2de5be222c3c8..b956ed1b79208cf6bb00654ad27c086c8cbb6741 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 6e469bb15260722d60193ad3a164a1ed0fdeadb2..9c7302f426f44169acb0b2144e0f53433a8c2107 100644 --- a/test/metabase/query_processor_test/date_bucketing_test.clj +++ b/test/metabase/query_processor_test/date_bucketing_test.clj @@ -883,10 +883,10 @@ :breakout [!year.date]})))))) ;; RELATIVE DATES -(p.types/deftype+ ^:private TimestampDatasetDef [intervalSeconds] +(p.types/deftype+ ^:private TimestampDatasetDef [intervalSeconds intervalCount] pretty/PrettyPrintable (pretty [_] - (list 'TimestampDatasetDef. intervalSeconds))) + (list 'TimestampDatasetDef. intervalSeconds intervalCount))) (defn- driver->current-datetime-base-type "Returns the :base-type of the \"current timestamp\" HoneySQL form defined by the driver `d`. Relies upon the driver @@ -899,9 +899,10 @@ (defmethod mt/get-dataset-definition TimestampDatasetDef [^TimestampDatasetDef this] - (let [interval-seconds (.intervalSeconds this)] + (let [interval-seconds (.intervalSeconds this) + intervalCount (.intervalCount this)] (mt/dataset-definition - (str "interval_" interval-seconds) + (str "interval_" interval-seconds (when-not (= 30 intervalCount) (str "_" intervalCount))) ["checkins" [{:field-name "timestamp" :base-type (or (driver->current-datetime-base-type driver/*driver*) :type/DateTime)}] @@ -923,11 +924,17 @@ (* i interval-seconds) :second)) (u.date/add :second (* i interval-seconds))) - (assert <>))]) - (range -15 15))]))) - -(defn- dataset-def-with-timestamps [interval-seconds] - (TimestampDatasetDef. interval-seconds)) + (assert <>))]) + (let [shift (quot intervalCount 2) + lower-bound (- shift) + upper-bound (- intervalCount shift)] + (range lower-bound upper-bound)))]))) + +(defn- dataset-def-with-timestamps + ([interval-seconds] + (dataset-def-with-timestamps interval-seconds 30)) + ([interval-seconds interval-count] + (TimestampDatasetDef. interval-seconds interval-count))) (def ^:private checkins:4-per-minute "Dynamically generated dataset with 30 checkins spaced 15 seconds apart, from 3 mins 45 seconds ago to 3 minutes 30 @@ -943,6 +950,10 @@ "Dynamically generated dataset with 30 checkins spaced 24 hours apart, from 15 days ago to 14 days in the future." (dataset-def-with-timestamps (* 24 (u/minutes->seconds 60)))) +(def ^:private checkins:1-per-day:60 + "Dynamically generated dataset with 60 checkins spaced 24 hours apart, from 30 days ago to 29 days in the future." + (dataset-def-with-timestamps (* 24 (u/minutes->seconds 60)) 60)) + (defn- checkins-db-is-old? "Determine whether we need to recreate one of the dynamically-generated datasets above, if the data has grown a little stale." @@ -1052,6 +1063,28 @@ (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:60 + (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 +1440,30 @@ (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:60 + (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)"