From d140d676bff768227bb96b33a231887052717586 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 9 Nov 2023 17:51:14 -0800
Subject: [PATCH] Generate sane SQL for a DATE[TIME] expression = DATE literal
filter (#35523)
---
shared/src/metabase/mbql/schema.cljc | 3 +-
.../lib/metadata/cached_provider.cljc | 8 +
.../lib/metadata/composed_provider.cljc | 70 +++---
src/metabase/lib/metadata/jvm.clj | 7 +
.../middleware/optimize_temporal_filters.clj | 200 +++++++++++-------
.../middleware/wrap_value_literals.clj | 41 ++--
.../lib/metadata/cached_provider_test.cljc | 4 +
.../lib/metadata/composed_provider_test.cljc | 4 +
test/metabase/lib/metadata/jvm_test.clj | 4 +
.../lib/test_metadata/graph_provider.cljc | 17 +-
.../test_util/metadata_providers/mock.cljc | 89 +++++---
.../optimize_temporal_filters_test.clj | 94 ++++++--
.../middleware/wrap_value_literals_test.clj | 32 +++
.../query_processor_test/filter_test.clj | 23 +-
14 files changed, 411 insertions(+), 185 deletions(-)
diff --git a/shared/src/metabase/mbql/schema.cljc b/shared/src/metabase/mbql/schema.cljc
index 07d746c5122..fc94832dbad 100644
--- a/shared/src/metabase/mbql/schema.cljc
+++ b/shared/src/metabase/mbql/schema.cljc
@@ -1858,7 +1858,8 @@
(mr/validator Query))
(def ^{:arglists '([query])} validate-query
- "Validator for an outer query; throw an Exception explaining why the query is invalid if it is."
+ "Validator for an outer query; throw an Exception explaining why the query is invalid if it is. Returns query if
+ valid."
(let [explainer (mr/explainer Query)]
(fn [query]
(if (valid-query? query)
diff --git a/src/metabase/lib/metadata/cached_provider.cljc b/src/metabase/lib/metadata/cached_provider.cljc
index a0eaab0ffa0..3f4aa34ab9d 100644
--- a/src/metabase/lib/metadata/cached_provider.cljc
+++ b/src/metabase/lib/metadata/cached_provider.cljc
@@ -9,6 +9,8 @@
[metabase.util.malli :as mu]
#?@(:clj ([pretty.core :as pretty]))))
+#?(:clj (set! *warn-on-reflection* true))
+
(defn- get-in-cache [cache ks]
(when-some [cached-value (get-in @cache ks)]
(when-not (= cached-value ::nil)
@@ -109,6 +111,12 @@
(bulk-metadata [_this metadata-type ids]
(bulk-metadata cache metadata-provider metadata-type ids))
+ #?(:clj Object :cljs IEquiv)
+ (#?(:clj equals :cljs -equiv) [_this another]
+ (and (instance? CachedProxyMetadataProvider another)
+ (= metadata-provider
+ (#?(:clj .metadata-provider :cljs .-metadata-provider) ^CachedProxyMetadataProvider another))))
+
#?@(:clj
[pretty/PrettyPrintable
(pretty [_this]
diff --git a/src/metabase/lib/metadata/composed_provider.cljc b/src/metabase/lib/metadata/composed_provider.cljc
index a5af45dddb0..90215f585fd 100644
--- a/src/metabase/lib/metadata/composed_provider.cljc
+++ b/src/metabase/lib/metadata/composed_provider.cljc
@@ -22,38 +22,46 @@
(m/distinct-by :id))
metadata-providers))
+(deftype ComposedMetadataProvider [metadata-providers]
+ metadata.protocols/MetadataProvider
+ (database [_this] (some metadata.protocols/database metadata-providers))
+ (table [_this table-id] (object-for-id metadata.protocols/table table-id metadata-providers))
+ (field [_this field-id] (object-for-id metadata.protocols/field field-id metadata-providers))
+ (card [_this card-id] (object-for-id metadata.protocols/card card-id metadata-providers))
+ (metric [_this metric-id] (object-for-id metadata.protocols/metric metric-id metadata-providers))
+ (segment [_this segment-id] (object-for-id metadata.protocols/segment segment-id metadata-providers))
+ (setting [_this setting-name] (object-for-id metadata.protocols/setting setting-name metadata-providers))
+ (tables [_this] (m/distinct-by :id (mapcat metadata.protocols/tables metadata-providers)))
+ (fields [_this table-id] (objects-for-table-id metadata.protocols/fields table-id metadata-providers))
+ (metrics [_this table-id] (objects-for-table-id metadata.protocols/metrics table-id metadata-providers))
+ (segments [_this table-id] (objects-for-table-id metadata.protocols/segments table-id metadata-providers))
+
+ metadata.protocols/CachedMetadataProvider
+ (cached-database [_this]
+ (some metadata.protocols/cached-database
+ (cached-providers metadata-providers)))
+ (cached-metadata [_this metadata-type id]
+ (some #(metadata.protocols/cached-metadata % metadata-type id)
+ (cached-providers metadata-providers)))
+ (store-database! [_this database-metadata]
+ (when-first [provider (cached-providers metadata-providers)]
+ (metadata.protocols/store-database! provider database-metadata)))
+ (store-metadata! [_this metadata-type id metadata]
+ (when-first [provider (cached-providers metadata-providers)]
+ (metadata.protocols/store-metadata! provider metadata-type id metadata)))
+
+ #?(:clj Object :cljs IEquiv)
+ (#?(:clj equals :cljs -equiv) [_this another]
+ (and (instance? ComposedMetadataProvider another)
+ (= metadata-providers
+ (#?(:clj .metadata-providers :cljs .-metadata-providers ) ^ComposedMetadataProvider another))))
+
+ clojure.core.protocols/Datafiable
+ (datafy [_this]
+ (cons `composed-metadata-provider (map datafy/datafy metadata-providers))))
+
(defn composed-metadata-provider
"A metadata provider composed of several different `metadata-providers`. Methods try each constituent provider in
turn from left to right until one returns a truthy result."
[& metadata-providers]
- (reify
- metadata.protocols/MetadataProvider
- (database [_this] (some metadata.protocols/database metadata-providers))
- (table [_this table-id] (object-for-id metadata.protocols/table table-id metadata-providers))
- (field [_this field-id] (object-for-id metadata.protocols/field field-id metadata-providers))
- (card [_this card-id] (object-for-id metadata.protocols/card card-id metadata-providers))
- (metric [_this metric-id] (object-for-id metadata.protocols/metric metric-id metadata-providers))
- (segment [_this segment-id] (object-for-id metadata.protocols/segment segment-id metadata-providers))
- (setting [_this setting-name] (object-for-id metadata.protocols/setting setting-name metadata-providers))
- (tables [_this] (m/distinct-by :id (mapcat metadata.protocols/tables metadata-providers)))
- (fields [_this table-id] (objects-for-table-id metadata.protocols/fields table-id metadata-providers))
- (metrics [_this table-id] (objects-for-table-id metadata.protocols/metrics table-id metadata-providers))
- (segments [_this table-id] (objects-for-table-id metadata.protocols/segments table-id metadata-providers))
-
- metadata.protocols/CachedMetadataProvider
- (cached-database [_this]
- (some metadata.protocols/cached-database
- (cached-providers metadata-providers)))
- (cached-metadata [_this metadata-type id]
- (some #(metadata.protocols/cached-metadata % metadata-type id)
- (cached-providers metadata-providers)))
- (store-database! [_this database-metadata]
- (when-first [provider (cached-providers metadata-providers)]
- (metadata.protocols/store-database! provider database-metadata)))
- (store-metadata! [_this metadata-type id metadata]
- (when-first [provider (cached-providers metadata-providers)]
- (metadata.protocols/store-metadata! provider metadata-type id metadata)))
-
- clojure.core.protocols/Datafiable
- (datafy [_this]
- (cons `composed-metadata-provider (map datafy/datafy metadata-providers)))))
+ (->ComposedMetadataProvider metadata-providers))
diff --git a/src/metabase/lib/metadata/jvm.clj b/src/metabase/lib/metadata/jvm.clj
index 2fcd1f994a6..8ddb18480dd 100644
--- a/src/metabase/lib/metadata/jvm.clj
+++ b/src/metabase/lib/metadata/jvm.clj
@@ -19,6 +19,8 @@
[toucan2.core :as t2]
[toucan2.model :as t2.model]))
+(set! *warn-on-reflection* true)
+
(def ^:private MetadataType
[:enum
:metadata/database
@@ -336,6 +338,11 @@
(bulk-metadata [_this metadata-type ids]
(bulk-instances metadata-type database-id ids))
+ Object
+ (equals [_this another]
+ (and (instance? UncachedApplicationDatabaseMetadataProvider another)
+ (= database-id (.database-id ^UncachedApplicationDatabaseMetadataProvider another))))
+
pretty/PrettyPrintable
(pretty [_this]
(list `->UncachedApplicationDatabaseMetadataProvider database-id)))
diff --git a/src/metabase/query_processor/middleware/optimize_temporal_filters.clj b/src/metabase/query_processor/middleware/optimize_temporal_filters.clj
index 880630beefe..08dd23d8bed 100644
--- a/src/metabase/query_processor/middleware/optimize_temporal_filters.clj
+++ b/src/metabase/query_processor/middleware/optimize_temporal_filters.clj
@@ -3,10 +3,10 @@
`optimize-temporal-filters` for more details."
(:require
[clojure.walk :as walk]
+ [metabase.lib.hierarchy :as lib.hierarchy]
[metabase.mbql.util :as mbql.u]
[metabase.util :as u]
[metabase.util.date-2 :as u.date]
- [metabase.util.i18n :refer [trs]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]))
@@ -14,54 +14,93 @@
(def ^:private optimizable-units
#{:second :minute :hour :day :week :month :quarter :year})
-(defn- temporal-unit [field]
- (mbql.u/match-one field [:field _ (opts :guard :temporal-unit)] (:temporal-unit opts)))
-
-(defn- optimizable-field? [field]
- (mbql.u/match-one field
- [:field _ (_ :guard (comp optimizable-units :temporal-unit))]))
-
-(defmulti ^:private can-optimize-filter?
- mbql.u/dispatch-by-clause-name-or-class)
-
-(defn- optimizable-temporal-value?
- "Can `temporal-value` clause can be optimized?"
- [temporal-value]
- (mbql.u/match-one temporal-value
- [:relative-datetime (_ :guard #{0 :current})]
- true
-
- [(_ :guard #{:absolute-datetime :relative-datetime}) _ (unit :guard optimizable-units)]
- true))
-
-(defn- field-and-temporal-value-have-compatible-units?
- "Do datetime `field` clause and `temporal-value` clause have 'compatible' units that mean we'll be able to optimize
- the filter clause they're in?"
- [field temporal-value]
- (mbql.u/match-one temporal-value
- [:relative-datetime (_ :guard #{0 :current})]
- true
-
- [(_ :guard #{:absolute-datetime :relative-datetime}) _ (unit :guard optimizable-units)]
- (= (temporal-unit field) unit)))
-
-(defmethod can-optimize-filter? :default
- [filter-clause]
- (mbql.u/match-one filter-clause
- [_
- (field :guard optimizable-field?)
- (temporal-value :guard optimizable-temporal-value?)]
- (field-and-temporal-value-have-compatible-units? field temporal-value)))
-
-(defmethod can-optimize-filter? :between
- [filter-clause]
- (mbql.u/match-one filter-clause
- [_
- (field :guard optimizable-field?)
- (temporal-value-1 :guard optimizable-temporal-value?)
- (temporal-value-2 :guard optimizable-temporal-value?)]
- (and (field-and-temporal-value-have-compatible-units? field temporal-value-1)
- (field-and-temporal-value-have-compatible-units? field temporal-value-2))))
+(defmulti ^:private temporal-unit
+ {:arglists '([expression])}
+ mbql.u/dispatch-by-clause-name-or-class
+ :hierarchy lib.hierarchy/hierarchy)
+
+(defmethod temporal-unit :default
+ [_expr]
+ nil)
+
+(defmethod temporal-unit :field
+ [[_field _id-or-name opts]]
+ (:temporal-unit opts))
+
+(defmethod temporal-unit :relative-datetime
+ [[_relative-datetime _n unit]]
+ unit)
+
+(defmethod temporal-unit :absolute-datetime
+ [[_absolute-datetime _t unit]]
+ unit)
+
+(doseq [tag [:+ :-]]
+ (lib.hierarchy/derive tag ::arithmetic-expr))
+
+(defmethod temporal-unit ::arithmetic-expr
+ [[_tag & args]]
+ (some temporal-unit args))
+
+(defmulti ^:private optimizable?
+ {:arglists '([expression])}
+ mbql.u/dispatch-by-clause-name-or-class
+ :hierarchy lib.hierarchy/hierarchy)
+
+(defmethod optimizable? :default
+ [_expr]
+ false)
+
+(defmethod optimizable? :field
+ [[_field _id-or-name opts]]
+ (contains? optimizable-units (:temporal-unit opts)))
+
+(defmethod optimizable? :expression
+ [[_expression _name opts]]
+ (let [expression-type ((some-fn :effective-type :base-type) opts)]
+ (isa? expression-type :type/Temporal)))
+
+(defmethod optimizable? :relative-datetime
+ [[_relative-datetime n unit]]
+ (if unit
+ (contains? optimizable-units unit)
+ (#{0 :current} n)))
+
+(defmethod optimizable? :absolute-datetime
+ [[_absolute-datetime _t unit]]
+ (contains? optimizable-units unit))
+
+(defmethod optimizable? ::arithmetic-expr
+ [[_tag & args]]
+ (some optimizable? args))
+
+(doseq [tag [:= :!= :< :<= :> :>=]]
+ (lib.hierarchy/derive tag ::binary-filter))
+
+(defn- compatible-units?
+ "The temporal units for two expressions *x* and *y* are compatible for optimization purposes if
+
+ a. They both have units, and those units are the same
+
+ b. One of them has a unit, but the other does not."
+ [x-unit y-unit]
+ (if (and x-unit y-unit)
+ (= x-unit y-unit)
+ (or x-unit y-unit)))
+
+(defmethod optimizable? ::binary-filter
+ [[_tag lhs rhs]]
+ (and (optimizable? lhs)
+ (optimizable? rhs)
+ (compatible-units? (temporal-unit lhs) (temporal-unit rhs))))
+
+(defmethod optimizable? :between
+ [[_between expr lower-bound upper-bound]]
+ (and (optimizable? expr)
+ (optimizable? lower-bound)
+ (optimizable? upper-bound)
+ (compatible-units? (temporal-unit expr) (temporal-unit lower-bound))
+ (compatible-units? (temporal-unit expr) (temporal-unit upper-bound))))
(mu/defn ^:private temporal-literal-lower-bound
[unit t :- (ms/InstanceOfClass java.time.temporal.Temporal)]
@@ -71,9 +110,9 @@
[unit t :- (ms/InstanceOfClass java.time.temporal.Temporal)]
(:end (u.date/range t unit)))
-(defn- change-temporal-unit-to-default [field]
- (mbql.u/replace field
- [:field _ (_ :guard (comp optimizable-units :temporal-unit))]
+(defn- change-temporal-unit-to-default [expression]
+ (mbql.u/replace expression
+ [:field _id-or-name (_opts :guard (comp optimizable-units :temporal-unit))]
(mbql.u/update-field-options &match assoc :temporal-unit :default)))
(defmulti ^:private temporal-value-lower-bound
@@ -108,26 +147,25 @@
"Optimize a filter clause against a temporal-bucketed `:field` clause and `:absolute-datetime` or `:relative-datetime`
value by converting to an unbucketed range."
{:arglists '([clause])}
- mbql.u/dispatch-by-clause-name-or-class)
+ mbql.u/dispatch-by-clause-name-or-class
+ :hierarchy lib.hierarchy/hierarchy)
(defmethod optimize-filter :=
- [[_tag field temporal-value]]
- (let [temporal-unit (mbql.u/match-one field [:field _ (opts :guard :temporal-unit)] (:temporal-unit opts))]
- (when (field-and-temporal-value-have-compatible-units? field temporal-value)
- (let [field' (change-temporal-unit-to-default field)]
- [:and
- [:>= field' (temporal-value-lower-bound temporal-value temporal-unit)]
- [:< field' (temporal-value-upper-bound temporal-value temporal-unit)]]))))
+ [[_tag lhs rhs]]
+ (let [lhs' (change-temporal-unit-to-default lhs)]
+ [:and
+ [:>= lhs' (temporal-value-lower-bound rhs (temporal-unit lhs))]
+ [:< lhs' (temporal-value-upper-bound rhs (temporal-unit lhs))]]))
(defmethod optimize-filter :!=
[filter-clause]
(mbql.u/negate-filter-clause ((get-method optimize-filter :=) filter-clause)))
(defn- optimize-comparison-filter
- [optimize-temporal-value-fn [_filter-type field temporal-value] new-filter-type]
+ [optimize-temporal-value-fn [_tag lhs rhs] new-filter-type]
[new-filter-type
- (change-temporal-unit-to-default field)
- (optimize-temporal-value-fn temporal-value (temporal-unit field))])
+ (change-temporal-unit-to-default lhs)
+ (optimize-temporal-value-fn rhs (temporal-unit lhs))])
(defmethod optimize-filter :<
[filter-clause]
@@ -146,16 +184,16 @@
(optimize-comparison-filter temporal-value-lower-bound filter-clause :>=))
(defmethod optimize-filter :between
- [[_ field lower-bound upper-bound]]
- (let [field' (change-temporal-unit-to-default field)]
+ [[_between expr lower-bound upper-bound]]
+ (let [expr' (change-temporal-unit-to-default expr)]
[:and
- [:>= field' (temporal-value-lower-bound lower-bound (temporal-unit field))]
- [:< field' (temporal-value-upper-bound upper-bound (temporal-unit field))]]))
+ [:>= expr' (temporal-value-lower-bound lower-bound (temporal-unit expr))]
+ [:< expr' (temporal-value-upper-bound upper-bound (temporal-unit expr))]]))
(defn- optimize-temporal-filters* [query]
(mbql.u/replace query
(_ :guard (partial mbql.u/is-clause? (set (keys (methods optimize-filter)))))
- (or (when (can-optimize-filter? &match)
+ (or (when (optimizable? &match)
(u/prog1 (optimize-filter &match)
(if <>
(when-not (= &match <>)
@@ -163,7 +201,8 @@
;; if for some reason `optimize-filter` doesn't return an optimized filter clause, log and error and use
;; the original. `can-optimize-filter?` shouldn't have said we could optimize this filter in the first
;; place
- (log/error (trs "Error optimizing temporal filter clause") (pr-str &match)))))
+ (log/error "Error optimizing temporal filter clause: optimize-filter returned nil"
+ (pr-str &match)))))
&match)))
(defn optimize-temporal-filters
@@ -194,13 +233,16 @@
(if (not= query-type :query)
query
;; walk query, looking for inner-query forms that have a `:filter` key
- (walk/postwalk
- (fn [form]
- (if-not (and (map? form) (seq (:filter form)))
- form
- ;; optimize the filters in this inner-query form.
- (let [optimized (optimize-temporal-filters* form)]
- ;; if we did some optimizations, we should flatten/deduplicate the filter clauses afterwards.
- (cond-> optimized
- (not= optimized form) (update :filter mbql.u/combine-filter-clauses)))))
- query)))
+ (let [query' (walk/postwalk
+ (fn [form]
+ (if-not (and (map? form) (seq (:filter form)))
+ form
+ ;; optimize the filters in this inner-query form.
+ (let [optimized (optimize-temporal-filters* form)]
+ ;; if we did some optimizations, we should flatten/deduplicate the filter clauses afterwards.
+ (cond-> optimized
+ (not= optimized form) (update :filter mbql.u/combine-filter-clauses)))))
+ query)]
+ (when-not (= query query')
+ (log/debugf "Optimized temporal filters:\n%s" (u/pprint-to-str query)))
+ query')))
diff --git a/src/metabase/query_processor/middleware/wrap_value_literals.clj b/src/metabase/query_processor/middleware/wrap_value_literals.clj
index 6cc88177a8e..171e07832ad 100644
--- a/src/metabase/query_processor/middleware/wrap_value_literals.clj
+++ b/src/metabase/query_processor/middleware/wrap_value_literals.clj
@@ -7,9 +7,11 @@
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.timezone :as qp.timezone]
+ [metabase.shared.util.internal.time-common :as shared.ut.common]
[metabase.types :as types]
[metabase.util :as u]
- [metabase.util.date-2 :as u.date])
+ [metabase.util.date-2 :as u.date]
+ [metabase.util.log :as log])
(:import
(java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime)))
@@ -45,6 +47,10 @@
(when (:base-type opts)
{:base_type (:base-type opts)})))
+(defmethod type-info :expression
+ [[_expression _expression-name opts]]
+ (u/snake-keys (select-keys opts [:base-type :effective-type :temporal-unit])))
+
;;; ------------------------------------------------- add-type-info --------------------------------------------------
@@ -89,19 +95,27 @@
[this info & _]
[:absolute-datetime this (get info :unit :default)])
+(defn- temporal-value? [info]
+ (or (:unit info)
+ (some-> ((some-fn :effective_type :base_type) info) (isa? :type/Temporal))))
+
(defmethod add-type-info String
- [this {:keys [unit], :as info} & {:keys [parse-datetime-strings?]
- :or {parse-datetime-strings? true}}]
- (if-let [temporal-value (when (and unit
+ [s {:keys [unit], :as info} & {:keys [parse-datetime-strings?]
+ :or {parse-datetime-strings? true}}]
+ (if-let [temporal-value (when (and (temporal-value? info)
parse-datetime-strings?
- (string? this))
+ (string? s))
;; TIMEZONE FIXME - I think this should actually use
;; (qp.timezone/report-timezone-id-if-supported) instead ?
- (u.date/parse this (qp.timezone/results-timezone-id)))]
- (if (some #(instance? % temporal-value) [LocalTime OffsetTime])
- [:time temporal-value unit]
- [:absolute-datetime temporal-value unit])
- [:value this info]))
+ (u.date/parse s (qp.timezone/results-timezone-id)))]
+ (let [unit (or unit
+ (if (shared.ut.common/matches-date? s)
+ :day
+ :default))]
+ (if (some #(instance? % temporal-value) [LocalTime OffsetTime])
+ [:time temporal-value unit]
+ [:absolute-datetime temporal-value unit]))
+ [:value s info]))
;;; -------------------------------------------- wrap-literals-in-clause ---------------------------------------------
@@ -163,5 +177,8 @@
[{query-type :type, :as query}]
(if-not (= query-type :query)
query
- (mbql.s/validate-query
- (update query :query wrap-value-literals-in-mbql-query nil))))
+ (let [query' (update query :query wrap-value-literals-in-mbql-query nil)]
+ (when-not (= query query')
+ (log/debugf "Wrapped literals in :value clauses:\n%s" (u/pprint-to-str query'))
+ (mbql.s/validate-query query'))
+ query')))
diff --git a/test/metabase/lib/metadata/cached_provider_test.cljc b/test/metabase/lib/metadata/cached_provider_test.cljc
index c0b0df2d906..b42d1b35a26 100644
--- a/test/metabase/lib/metadata/cached_provider_test.cljc
+++ b/test/metabase/lib/metadata/cached_provider_test.cljc
@@ -52,3 +52,7 @@
(lib.metadata.protocols/bulk-metadata provider :metadata/table #{1 2})))
(is (= 2
@fetch-count))))))
+
+(deftest ^:parallel equality-test
+ (is (= (lib.metadata.cached-provider/cached-metadata-provider nil)
+ (lib.metadata.cached-provider/cached-metadata-provider nil))))
diff --git a/test/metabase/lib/metadata/composed_provider_test.cljc b/test/metabase/lib/metadata/composed_provider_test.cljc
index e9a6a6ef56a..5a5bae2d11c 100644
--- a/test/metabase/lib/metadata/composed_provider_test.cljc
+++ b/test/metabase/lib/metadata/composed_provider_test.cljc
@@ -25,3 +25,7 @@
(lib.metadata/field
metadata-provider
(meta/id :people :birth-date)))))))
+
+(deftest ^:parallel equality-test
+ (is (= (lib/composed-metadata-provider meta/metadata-provider)
+ (lib/composed-metadata-provider meta/metadata-provider))))
diff --git a/test/metabase/lib/metadata/jvm_test.clj b/test/metabase/lib/metadata/jvm_test.clj
index 4cf981fe197..bc434fcaf06 100644
--- a/test/metabase/lib/metadata/jvm_test.clj
+++ b/test/metabase/lib/metadata/jvm_test.clj
@@ -193,3 +193,7 @@
(lib.metadata/card
(lib.metadata.jvm/application-database-metadata-provider (mt/id))
card-id)))))
+
+(deftest ^:parallel equality-test
+ (is (= (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+ (lib.metadata.jvm/application-database-metadata-provider (mt/id)))))
diff --git a/test/metabase/lib/test_metadata/graph_provider.cljc b/test/metabase/lib/test_metadata/graph_provider.cljc
index 4b691e54afd..eea2c2a6224 100644
--- a/test/metabase/lib/test_metadata/graph_provider.cljc
+++ b/test/metabase/lib/test_metadata/graph_provider.cljc
@@ -1,6 +1,7 @@
(ns metabase.lib.test-metadata.graph-provider
(:require
[clojure.core.protocols]
+ [clojure.test :refer [deftest is]]
[medley.core :as m]
[metabase.lib.metadata.protocols :as lib.metadata.protocols]
#?@(:clj
@@ -73,9 +74,19 @@
(datafy [_this]
(list `->SimpleGraphMetadataProvider metadata-graph))
+ #?(:clj Object :cljs IEquiv)
+ (#?(:clj equals :cljs -equiv) [_this another]
+ (and (instance? SimpleGraphMetadataProvider another)
+ (= metadata-graph
+ (#?(:clj .metadata-graph :cljs .-metadata-graph) ^SimpleGraphMetadataProvider another))))
+
#?@(:clj
[pretty/PrettyPrintable
(pretty [_this]
- (if (identical? metadata-graph @(requiring-resolve 'metabase.lib.test-metadata/metadata))
- 'metabase.lib.test-metadata/metadata-provider
- (list `->SimpleGraphMetadataProvider metadata-graph)))]))
+ (if (identical? metadata-graph @(requiring-resolve 'metabase.lib.test-metadata/metadata))
+ 'metabase.lib.test-metadata/metadata-provider
+ (list `->SimpleGraphMetadataProvider metadata-graph)))]))
+
+(deftest ^:parallel equality-test
+ (is (= (->SimpleGraphMetadataProvider {})
+ (->SimpleGraphMetadataProvider {}))))
diff --git a/test/metabase/lib/test_util/metadata_providers/mock.cljc b/test/metabase/lib/test_util/metadata_providers/mock.cljc
index 8dc447c101b..0d6dc7c2f27 100644
--- a/test/metabase/lib/test_util/metadata_providers/mock.cljc
+++ b/test/metabase/lib/test_util/metadata_providers/mock.cljc
@@ -1,10 +1,12 @@
(ns metabase.lib.test-util.metadata-providers.mock
(:require
[clojure.core.protocols]
+ [clojure.test :refer [deftest is]]
[medley.core :as m]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.protocols :as metadata.protocols]
+ [metabase.lib.test-metadata :as meta]
[metabase.util.malli :as mu]))
(defn- with-optional-lib-type
@@ -27,6 +29,47 @@
[:segments {:optional true} [:maybe [:sequential (with-optional-lib-type lib.metadata/SegmentMetadata :metadata/segment)]]]
[:settings {:optional true} [:maybe [:map-of :keyword any?]]]])
+(deftype MockMetadataProvider [metadata]
+ metadata.protocols/MetadataProvider
+ (database [_this] (some-> (:database metadata)
+ (assoc :lib/type :metadata/database)
+ (dissoc :tables)))
+ (table [_this table-id] (some-> (m/find-first #(= (:id %) table-id) (:tables metadata))
+ (assoc :lib/type :metadata/table)
+ (dissoc :fields)))
+ (field [_this field-id] (some-> (m/find-first #(= (:id %) field-id) (:fields metadata))
+ (assoc :lib/type :metadata/column)))
+ (card [_this card-id] (some-> (m/find-first #(= (:id %) card-id) (:cards metadata))
+ (assoc :lib/type :metadata/card)))
+ (metric [_this metric-id] (some-> (m/find-first #(= (:id %) metric-id) (:metrics metadata))
+ (assoc :lib/type :metadata/metric)))
+ (segment [_this segment-id] (some-> (m/find-first #(= (:id %) segment-id) (:segments metadata))
+ (assoc :lib/type :metadata/segment)))
+ (tables [_this] (for [table (:tables metadata)]
+ (-> (assoc table :lib/type :metadata/table)
+ (dissoc :fields))))
+ (fields [_this table-id] (for [field (:fields metadata)
+ :when (= (:table-id field) table-id)]
+ (assoc field :lib/type :metadata/column)))
+ (metrics [_this table-id] (for [metric (:metrics metadata)
+ :when (= (:table-id metric) table-id)]
+ (assoc metric :lib/type :metadata/metric)))
+ (segments [_this table-id] (for [segment (:segments metadata)
+ :when (= (:table-id segment) table-id)]
+ (assoc segment :lib/type :metadata/segment)))
+
+ (setting [_this setting] (get-in metadata [:settings (keyword setting)]))
+
+ #?(:clj Object :cljs IEquiv)
+ (#?(:clj equals :cljs -equiv) [_this another]
+ (and (instance? MockMetadataProvider another)
+ (= metadata
+ (#?(:clj .metadata :cljs .-metadata) ^MockMetadataProvider another))))
+
+ clojure.core.protocols/Datafiable
+ (datafy [_this]
+ (list `mock-metadata-provider metadata)))
+
(mu/defn mock-metadata-provider :- lib.metadata/MetadataProvider
"Create a mock metadata provider to facilitate writing tests. All keys except `:database` should be a sequence of maps
e.g.
@@ -42,43 +85,19 @@
(lib.tu/mock-metadata-provider parent-metadata-provider {...})
=>
(lib/composed-metadata-provider (lib.tu/mock-metadata-provider {...}) parent-metadata-provider)"
- ([{:keys [database tables fields cards metrics segments settings] :as m} :- MockMetadata]
- (reify
- metadata.protocols/MetadataProvider
- (database [_this] (some-> database
- (assoc :lib/type :metadata/database)
- (dissoc :tables)))
- (table [_this table-id] (some-> (m/find-first #(= (:id %) table-id) tables)
- (assoc :lib/type :metadata/table)
- (dissoc :fields)))
- (field [_this field-id] (some-> (m/find-first #(= (:id %) field-id) fields)
- (assoc :lib/type :metadata/column)))
- (card [_this card-id] (some-> (m/find-first #(= (:id %) card-id) cards)
- (assoc :lib/type :metadata/card)))
- (metric [_this metric-id] (some-> (m/find-first #(= (:id %) metric-id) metrics)
- (assoc :lib/type :metadata/metric)))
- (segment [_this segment-id] (some-> (m/find-first #(= (:id %) segment-id) segments)
- (assoc :lib/type :metadata/segment)))
- (tables [_this] (for [table tables]
- (-> (assoc table :lib/type :metadata/table)
- (dissoc :fields))))
- (fields [_this table-id] (for [field fields
- :when (= (:table-id field) table-id)]
- (assoc field :lib/type :metadata/column)))
- (metrics [_this table-id] (for [metric metrics
- :when (= (:table-id metric) table-id)]
- (assoc metric :lib/type :metadata/metric)))
- (segments [_this table-id] (for [segment segments
- :when (= (:table-id segment) table-id)]
- (assoc segment :lib/type :metadata/segment)))
-
- (setting [_this setting] (get settings (keyword setting)))
-
- clojure.core.protocols/Datafiable
- (datafy [_this]
- (list `mock-metadata-provider m))))
+ ([m :- MockMetadata]
+ (->MockMetadataProvider m))
([parent-metadata-provider mock-metadata]
(lib/composed-metadata-provider
(mock-metadata-provider mock-metadata)
parent-metadata-provider)))
+
+(deftest ^:parallel equality-test
+ (let [time-field (assoc (meta/field-metadata :people :birth-date)
+ :base-type :type/Time
+ :effective-type :type/Time)]
+ (is (= (mock-metadata-provider
+ {:fields [time-field]})
+ (mock-metadata-provider
+ {:fields [time-field]})))))
diff --git a/test/metabase/query_processor/middleware/optimize_temporal_filters_test.clj b/test/metabase/query_processor/middleware/optimize_temporal_filters_test.clj
index 22cf7fd9a8c..21cd48ab91e 100644
--- a/test/metabase/query_processor/middleware/optimize_temporal_filters_test.clj
+++ b/test/metabase/query_processor/middleware/optimize_temporal_filters_test.clj
@@ -21,7 +21,7 @@
(-> (optimize-temporal-filters/optimize-temporal-filters query)
(get-in [:query :filter]))))
-(deftest optimize-day-bucketed-filter-test
+(deftest ^:parallel optimize-day-bucketed-filter-test
(testing "Make sure we aren't doing anything wacky when optimzing filters against fields bucketed by day"
(letfn [(optimize [filter-type]
(#'optimize-temporal-filters/optimize-filter
@@ -87,7 +87,7 @@
:lower (u.date/parse "2019-01-01" "UTC")
:upper (u.date/parse "2020-01-01" "UTC")}])
-(deftest optimize-temporal-filters-test
+(deftest ^:parallel optimize-temporal-filters-test
(driver/with-driver ::timezone-driver
(doseq [{:keys [unit filter-value lower upper]} test-units-and-values]
(let [lower [:absolute-datetime lower :default]
@@ -152,7 +152,7 @@
(-> (optimize-temporal-filters/optimize-temporal-filters query)
(get-in [:query :filter]))))
-(deftest timezones-test
+(deftest ^:parallel timezones-test
(driver/with-driver ::timezone-driver
(doseq [timezone-id ["UTC" "US/Pacific"]]
(testing (format "%s timezone" timezone-id)
@@ -176,7 +176,7 @@
(format "= %s in the %s timezone should be optimized to range %s -> %s"
t timezone-id lower upper))))))))))
-(deftest skip-optimization-test
+(deftest ^:parallel skip-optimization-test
(let [clause [:= [:field 1 {:temporal-unit :day}] [:absolute-datetime #t "2019-01-01" :month]]]
(is (= clause
(optimize-temporal-filters clause))
@@ -192,36 +192,46 @@
(str/replace #"\"" "")
(str/replace #"PUBLIC\." "")))))
-(deftest e2e-test
+(deftest ^:parallel e2e-test
(testing :=
(is (= {:query "WHERE (CHECKINS.DATE >= ?) AND (CHECKINS.DATE < ?)"
:params [(t/zoned-date-time (t/local-date 2019 9 24) (t/local-time 0) "UTC")
(t/zoned-date-time (t/local-date 2019 9 25) (t/local-time 0) "UTC")]}
(mt/$ids checkins
- (filter->sql [:= !day.date "2019-09-24T12:00:00.000Z"])))))
+ (filter->sql [:= !day.date "2019-09-24T12:00:00.000Z"]))))))
+
+(deftest ^:parallel e2e-test-2
(testing :<
(is (= {:query "WHERE CHECKINS.DATE < ?"
:params [(t/zoned-date-time (t/local-date 2019 9 24) (t/local-time 0) "UTC")]}
(mt/$ids checkins
- (filter->sql [:< !day.date "2019-09-24T12:00:00.000Z"])))))
+ (filter->sql [:< !day.date "2019-09-24T12:00:00.000Z"]))))))
+
+(deftest ^:parallel e2e-test-3
(testing :between
(is (= {:query "WHERE (CHECKINS.DATE >= ?) AND (CHECKINS.DATE < ?)"
:params [(t/zoned-date-time (t/local-date 2019 9 1) (t/local-time 0) "UTC")
(t/zoned-date-time (t/local-date 2019 11 1) (t/local-time 0) "UTC")]}
(mt/$ids checkins
- (filter->sql [:between !month.date "2019-09-02T12:00:00.000Z" "2019-10-05T12:00:00.000Z"]))))
- (is (= {:query "WHERE (CHECKINS.DATE >= ?) AND (CHECKINS.DATE < ?)"
- :params [(t/zoned-date-time "2019-09-01T00:00Z[UTC]")
- (t/zoned-date-time "2019-10-02T00:00Z[UTC]")]}
+ (filter->sql [:between !month.date "2019-09-02T12:00:00.000Z" "2019-10-05T12:00:00.000Z"]))))))
+
+(deftest ^:parallel e2e-test-4
+ (testing :between
+ (is (= {:query "WHERE (CHECKINS.DATE >= ?) AND (CHECKINS.DATE < ?)"
+ :params [(t/zoned-date-time "2019-09-01T00:00Z[UTC]")
+ (t/zoned-date-time "2019-10-02T00:00Z[UTC]")]}
(mt/$ids checkins
(filter->sql [:between !day.date "2019-09-01" "2019-10-01"]))))))
-(deftest optimize-relative-datetimes-test
+(def ^:private relative-datetime-units
+ "second/millisecond are not currently allowed in `:relative-datetime`, but if we add them we can re-enable their
+ tests."
+ [#_:millisecond #_:second :minute :hour :day :week :month :quarter :year])
+
+(deftest ^:parallel optimize-relative-datetimes-last-test
(testing "Should optimize relative-datetime clauses (#11837)"
(mt/dataset attempted-murders
- ;; second/millisecond are not currently allowed in `:relative-datetime`, but if we add them we can re-enable
- ;; their tests
- (doseq [unit [#_:millisecond #_:second :minute :hour :day :week :month :quarter :year]]
+ (doseq [unit relative-datetime-units]
(testing (format "last %s" unit)
(is (= (mt/mbql-query attempts
{:aggregation [[:count]]
@@ -238,7 +248,12 @@
{:aggregation [[:count]]
:filter [:=
[:field %datetime {:temporal-unit unit}]
- [:relative-datetime -1 unit]]})))))
+ [:relative-datetime -1 unit]]})))))))))
+
+(deftest ^:parallel optimize-relative-datetimes-this-test
+ (testing "Should optimize relative-datetime clauses (#11837)"
+ (mt/dataset attempted-murders
+ (doseq [unit relative-datetime-units]
(testing (format "this %s" unit)
;; test the various different ways we might refer to 'now'
(doseq [clause [[:relative-datetime 0]
@@ -259,7 +274,12 @@
{:aggregation [[:count]]
:filter [:=
[:field %datetime {:temporal-unit unit}]
- clause]})))))))
+ clause]})))))))))))
+
+(deftest ^:parallel optimize-relative-datetimes-next-test
+ (testing "Should optimize relative-datetime clauses (#11837)"
+ (mt/dataset attempted-murders
+ (doseq [unit relative-datetime-units]
(testing (format "next %s" unit)
(is (= (mt/mbql-query attempts
{:aggregation [[:count]]
@@ -277,7 +297,7 @@
[:field %datetime {:temporal-unit unit}]
[:relative-datetime 1 unit]]})))))))))
-(deftest optimize-mixed-temporal-values-test
+(deftest ^:parallel optimize-mixed-temporal-values-test
(testing "We should be able to optimize mixed usages of `:absolute-datetime` and `:relative-datetime`"
(mt/dataset attempted-murders
(testing "between month(2021-01-15) and month(now) [inclusive]"
@@ -299,7 +319,7 @@
[:absolute-datetime #t "2021-01-15T00:00:00Z" :month]
[:relative-datetime 0]]}))))))))
-(deftest optimize-relative-datetimes-e2e-test
+(deftest ^:parallel optimize-relative-datetimes-e2e-test
(testing "Should optimize relative-datetime clauses (#11837)"
(mt/dataset attempted-murders
(is (= (str "SELECT COUNT(*) AS \"count\" "
@@ -316,7 +336,7 @@
{:aggregation [[:count]]
:filter [:time-interval $datetime :last :month]}))))))))
-(deftest flatten-filters-test
+(deftest ^:parallel flatten-filters-test
(testing "Should flatten the `:filter` clause after optimizing"
(mt/$ids checkins
(is (= [:and
@@ -328,7 +348,7 @@
[:= $venue_id 1]
[:= [:field %date {:temporal-unit :month}] [:relative-datetime -1 :month]]]))))))
-(deftest deduplicate-filters-tets
+(deftest ^:parallel deduplicate-filters-tets
(testing "Should deduplicate the optimized filters with any existing ones"
(mt/$ids checkins
(is (= [:and
@@ -339,7 +359,7 @@
[:< [:field %date {:temporal-unit :default}] [:relative-datetime 0 :month]]
[:= [:field %date {:temporal-unit :month}] [:relative-datetime -1 :month]]]))))))
-(deftest optimize-filters-all-levels-test
+(deftest ^:parallel optimize-filters-all-levels-test
(testing "Should optimize filters at all levels of the query"
(mt/$ids checkins
(is (= (mt/mbql-query checkins
@@ -354,7 +374,7 @@
{:source-table $$checkins
:filter [:= [:field %date {:temporal-unit :month}] [:relative-datetime -1 :month]]}})))))))
-(deftest optimize-filter-with-nested-compatible-field
+(deftest ^:parallel optimize-filter-with-nested-compatible-field
(testing "Should optimize fields when starting n :temporal-unit ago (#25378)"
(mt/$ids users
(is (= (mt/mbql-query users
@@ -377,3 +397,31 @@
:source-query {:source-table $$users
:aggregation [[:count]]
:breakout [[:field %last_login {:temporal-unit :month}]]}})))))))
+
+(deftest ^:parallel optimize-filter-with-expression-ref-test
+ (is (= [:and
+ [:>=
+ [:expression "CC Created At" {:base-type :type/DateTimeWithLocalTZ}]
+ [:absolute-datetime #t "2017-10-07" :default]]
+ [:<
+ [:expression "CC Created At" {:base-type :type/DateTimeWithLocalTZ}]
+ [:absolute-datetime #t "2017-10-08" :default]]]
+ (#'optimize-temporal-filters/optimize-filter
+ [:=
+ [:expression "CC Created At" {:base-type :type/DateTimeWithLocalTZ}]
+ [:absolute-datetime #t "2017-10-07" :day]]))))
+
+(deftest ^:parallel optimize-filter-with-expression-ref-test-2
+ (testing ":between is inclusive"
+ (is (= [:and
+ [:>=
+ [:expression "CC Created At" {:base-type :type/DateTimeWithLocalTZ}]
+ [:absolute-datetime #t "2017-10-07" :default]]
+ [:<
+ [:expression "CC Created At" {:base-type :type/DateTimeWithLocalTZ}]
+ [:absolute-datetime #t "2017-10-09" :default]]]
+ (#'optimize-temporal-filters/optimize-filter
+ [:between
+ [:expression "CC Created At" {:base-type :type/DateTimeWithLocalTZ}]
+ [:absolute-datetime #t "2017-10-07" :day]
+ [:absolute-datetime #t "2017-10-08" :day]])))))
diff --git a/test/metabase/query_processor/middleware/wrap_value_literals_test.clj b/test/metabase/query_processor/middleware/wrap_value_literals_test.clj
index ce7ec108a49..bac3097b17e 100644
--- a/test/metabase/query_processor/middleware/wrap_value_literals_test.clj
+++ b/test/metabase/query_processor/middleware/wrap_value_literals_test.clj
@@ -3,6 +3,9 @@
[clojure.test :refer :all]
[java-time.api :as t]
[metabase.driver :as driver]
+ [metabase.lib.convert :as lib.convert]
+ [metabase.lib.core :as lib]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
[metabase.lib.test-util.macros :as lib.tu.macros]
@@ -232,3 +235,32 @@
:filter [:not [:starts-with [:field "A" {:base-type :type/Text}] "f"]],
:source-query {:native "select 'foo' as a union select null as a union select 'bar' as a"}}
nil)))))
+
+(deftest ^:parallel expression-test
+ (testing "Value literals compared to :expression refs should get wrapped"
+ (mt/dataset sample-dataset
+ (qp.store/with-metadata-provider (mt/id)
+ (let [people (lib.metadata/table (qp.store/metadata-provider) (mt/id :people))
+ created-at (lib.metadata/field (qp.store/metadata-provider) (mt/id :people :created_at))
+ query (as-> (lib/query (qp.store/metadata-provider) people) query
+ (lib/expression query "CC Created At" created-at)
+ (lib/filter query (lib/=
+ (lib/expression-ref query "CC Created At")
+ "2017-10-07"))
+ (lib/aggregate query (lib/count)))]
+
+ (is (=? {:stages [{:filters
+ [[:=
+ {}
+ [:expression {:base-type :type/DateTimeWithLocalTZ} "CC Created At"]
+ "2017-10-07"]]}]}
+ query))
+ (is (=? {:stages [{:lib/type :mbql.stage/mbql,
+ :filters [[:=
+ {}
+ [:expression {:base-type :type/DateTimeWithLocalTZ} "CC Created At"]
+ [:absolute-datetime {} #t "2017-10-07T00:00Z[UTC]" :day]]]}]}
+ (->> query
+ lib.convert/->legacy-MBQL
+ wrap-value-literals
+ (lib/query query)))))))))
diff --git a/test/metabase/query_processor_test/filter_test.clj b/test/metabase/query_processor_test/filter_test.clj
index 96e05d579c5..83ba26543ad 100644
--- a/test/metabase/query_processor_test/filter_test.clj
+++ b/test/metabase/query_processor_test/filter_test.clj
@@ -4,10 +4,14 @@
[clojure.set :as set]
[clojure.test :refer :all]
[metabase.driver :as driver]
+ [metabase.lib.core :as lib]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.query-processor :as qp]
[metabase.query-processor-test.timezones-test :as timezones-test]
+ [metabase.query-processor.store :as qp.store]
[metabase.query-processor.test-util :as qp.test-util]
- [metabase.test :as mt]))
+ [metabase.test :as mt]
+ [metabase.util :as u]))
(deftest ^:parallel and-test
(mt/test-drivers (mt/normal-drivers)
@@ -656,3 +660,20 @@
(mt/run-mbql-query bird-count
{:order-by [[:asc $count] [:asc $id]]
:limit 3}))))))))
+
+(deftest ^:parallel date-filter-on-datetime-column-test
+ (testing "Filtering a DATETIME expression by a DATE literal string should do something sane (#17807)"
+ (mt/dataset sample-dataset
+ (qp.store/with-metadata-provider (mt/id)
+ (let [people (lib.metadata/table (qp.store/metadata-provider) (mt/id :people))
+ created-at (lib.metadata/field (qp.store/metadata-provider) (mt/id :people :created_at))
+ query (as-> (lib/query (qp.store/metadata-provider) people) query
+ (lib/expression query "CC Created At" created-at)
+ (lib/filter query (lib/=
+ (lib/expression-ref query "CC Created At")
+ "2017-10-07"))
+ (lib/aggregate query (lib/count)))]
+ (testing (str "\nquery =\n" (u/pprint-to-str query))
+ (mt/with-native-query-testing-context query
+ (is (= [[2]]
+ (mt/rows (qp/process-query query)))))))))))
--
GitLab