Skip to content

Commit

Permalink
Fix #39769
Browse files Browse the repository at this point in the history
  • Loading branch information
@camsaul
camsaul committed Mar 21, 2024
1 parent 7216c2e commit 3a360f8
Show file tree
Hide file tree
Showing 9 changed files with 609 additions and 350 deletions.
5 changes: 4 additions & 1 deletion src/metabase/query_processor/middleware/add_timezone_info.clj
View file
Expand Up @@ -5,7 +5,10 @@
(defn- add-timezone-metadata [metadata]
(merge
metadata
{:results_timezone (qp.timezone/results-timezone-id)}
{:results_timezone (qp.timezone/results-timezone-id)
;; I added these for debugging purposes only, hence the FE-unfriendly key names. -- Cam
:qp.debug/report-timezone (qp.timezone/report-timezone-id-if-supported)
:qp.debug/database-timezone (qp.timezone/database-timezone-id)}
(when-let [requested-timezone-id (qp.timezone/requested-timezone-id)]
{:requested_timezone requested-timezone-id})))

Expand Down
22 changes: 7 additions & 15 deletions src/metabase/query_processor/middleware/format_rows.clj
View file
Expand Up @@ -9,7 +9,7 @@
[metabase.util.log :as log]
[potemkin.types :as p.types])
(:import
(java.time Instant LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime ZoneId)))
(java.time Instant OffsetDateTime OffsetTime ZonedDateTime ZoneId)))

(p.types/defprotocol+ FormatValue
"Protocol for determining how QP results of various classes are serialized. Drivers can add implementations to support
Expand All @@ -19,36 +19,28 @@

(extend-protocol FormatValue
nil
(format-value [_ _]
(format-value [_v _timezone-id]
nil)

Object
(format-value [v _]
(format-value [v _timezone-id]
v)

LocalTime
(format-value [t timezone-id]
(t/format :iso-offset-time (u.date/with-time-zone-same-instant t timezone-id)))
java.time.temporal.Temporal
(format-value [t _timezone-id]
(u.date/format t))

OffsetTime
(format-value [t timezone-id]
(t/format :iso-offset-time (u.date/with-time-zone-same-instant t timezone-id)))

LocalDate
(format-value [t timezone-id]
(t/format :iso-offset-date-time (u.date/with-time-zone-same-instant t timezone-id)))

LocalDateTime
(format-value [t timezone-id]
(t/format :iso-offset-date-time (u.date/with-time-zone-same-instant t timezone-id)))

;; convert to a ZonedDateTime
Instant
(format-value [t timezone-id]
(format-value (t/zoned-date-time t (t/zone-id "UTC")) timezone-id))

OffsetDateTime
(format-value [t, ^ZoneId timezone-id]
(format-value [t timezone-id]
(t/format :iso-offset-date-time (u.date/with-time-zone-same-instant t timezone-id)))

ZonedDateTime
Expand Down
75 changes: 56 additions & 19 deletions src/metabase/query_processor/middleware/optimize_temporal_filters.clj
View file
Expand Up @@ -3,7 +3,9 @@
`optimize-temporal-filters` for more details."
(:require
[clojure.walk :as walk]
[metabase.lib.metadata :as lib.metadata]
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.store :as qp.store]
[metabase.util :as u]
[metabase.util.date-2 :as u.date]
[metabase.util.i18n :refer [trs]]
Expand All @@ -14,6 +16,7 @@
(def ^:private optimizable-units
#{:second :minute :hour :day :week :month :quarter :year})

;;; TODO -- we can use [[metabase.lib/temporal-bucket]] for this once we convert this middleware to MLv2
(defn- temporal-unit [field]
(mbql.u/match-one field [(_ :guard #{:field :expression}) _ (opts :guard :temporal-unit)] (:temporal-unit opts)))

Expand Down Expand Up @@ -45,10 +48,23 @@
[(_ :guard #{:absolute-datetime :relative-datetime}) _ (unit :guard optimizable-units)]
(= (temporal-unit field) unit)))

;;; TODO -- once we convert this middleware to MLv2 we can use [[metabase.lib.metadata.calculation/type-of]]
(defn- field-or-expression-effective-type [field-or-expression]
(mbql.u/match-one field-or-expression
[(_tag :guard #{:field :expression}) _ (opts :guard :effective-type)]
(:effective-type opts)

[:field (id :guard pos-int?) _opts]
(when-let [field (lib.metadata/field (qp.store/metadata-provider) id)]
(:effective-type field))

[(_tag :guard #{:field :expression}) _ (opts :guard :base-type)]
(:base-type opts)))

(defmethod can-optimize-filter? :default
[filter-clause]
(mbql.u/match-one filter-clause
[_
[_tag
(field :guard optimizable-field?)
(temporal-value :guard optimizable-temporal-value?)]
(field-and-temporal-value-have-compatible-units? field temporal-value)))
Expand All @@ -74,7 +90,13 @@
(defn- change-temporal-unit-to-default [field]
(mbql.u/replace field
[(_ :guard #{:field :expression}) _ (_ :guard (comp optimizable-units :temporal-unit))]
(mbql.u/update-field-options &match assoc :temporal-unit :default)))
(mbql.u/update-field-options &match assoc :temporal-unit :default)

[:absolute-datetime t _unit]
[:absolute-datetime t :default]

[:relative-datetime n _unit]
[:relative-datetime n :default]))

(defmulti ^:private temporal-value-lower-bound
"Get a clause representing the *lower* bound that should be used when converting a `temporal-value-clause` (e.g.
Expand Down Expand Up @@ -104,6 +126,10 @@
[[_ n unit] temporal-unit]
[:relative-datetime (inc (if (= n :current) 0 n)) (or unit temporal-unit)])

(defn- date-field-with-day-bucketing? [x]
(and (isa? (field-or-expression-effective-type x) :type/Date)
(= (temporal-unit x) :day)))

(defmulti ^:private optimize-filter
"Optimize a filter clause against a temporal-bucketed `:field` or `:expression` clause and `:absolute-datetime` or `:relative-datetime`
value by converting to an unbucketed range."
Expand All @@ -112,22 +138,28 @@

(defmethod optimize-filter :=
[[_tag field temporal-value]]
(let [temporal-unit (mbql.u/match-one field [(_ :guard #{:field :expression}) _ (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)]]))))
(if (date-field-with-day-bucketing? field)
[:= (change-temporal-unit-to-default field) (change-temporal-unit-to-default temporal-value)]
(let [temporal-unit (mbql.u/match-one field [(_ :guard #{:field :expression}) _ (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)]])))))

(defmethod optimize-filter :!=
[filter-clause]
(mbql.u/negate-filter-clause ((get-method optimize-filter :=) filter-clause)))
[[_tag field temporal-value :as filter-clause]]
(if (date-field-with-day-bucketing? field)
[:!= (change-temporal-unit-to-default field) (change-temporal-unit-to-default temporal-value)]
(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]
[new-filter-type
(change-temporal-unit-to-default field)
(optimize-temporal-value-fn temporal-value (temporal-unit field))])
[optimize-temporal-value-fn [tag field temporal-value] new-filter-type]
(if (date-field-with-day-bucketing? field)
[tag (change-temporal-unit-to-default field) (change-temporal-unit-to-default temporal-value)]
[new-filter-type
(change-temporal-unit-to-default field)
(optimize-temporal-value-fn temporal-value (temporal-unit field))]))

(defmethod optimize-filter :<
[filter-clause]
Expand All @@ -146,11 +178,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)]
[:and
[:>= field' (temporal-value-lower-bound lower-bound (temporal-unit field))]
[:< field' (temporal-value-upper-bound upper-bound (temporal-unit field))]]))
[[_tag field lower-bound upper-bound]]
(if (date-field-with-day-bucketing? field)
[:between
(change-temporal-unit-to-default field)
(change-temporal-unit-to-default lower-bound)
(change-temporal-unit-to-default upper-bound)]
(let [field' (change-temporal-unit-to-default field)]
[:and
[:>= field' (temporal-value-lower-bound lower-bound (temporal-unit field))]
[:< field' (temporal-value-upper-bound upper-bound (temporal-unit field))]])))

(defn- optimize-temporal-filters* [query]
(mbql.u/replace query
Expand Down
167 changes: 140 additions & 27 deletions src/metabase/query_processor/middleware/wrap_value_literals.clj
View file
Expand Up @@ -2,14 +2,17 @@
"Middleware that wraps value literals in `value`/`absolute-datetime`/etc. clauses containing relevant type
information; parses datetime string literals when appropriate."
(:require
[java-time.api :as t]
[metabase.lib.metadata :as lib.metadata]
[metabase.mbql.schema :as mbql.s]
[metabase.mbql.util :as mbql.u]
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.timezone :as qp.timezone]
[metabase.types :as types]
[metabase.util :as u]
[metabase.util.date-2 :as u.date])
[metabase.util.date-2 :as u.date]
[metabase.util.i18n :as i18n]
[metabase.query-processor.error-type :as qp.error-type])
(:import
(java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime)))

Expand Down Expand Up @@ -39,23 +42,23 @@

(defmethod type-info :field [[_ id-or-name opts]]
(merge
;; With Mlv2 queries, this could be combined with `:expression` below and use the column from the
;; query rather than metadata/field
(when (integer? id-or-name)
(type-info (lib.metadata/field (qp.store/metadata-provider) id-or-name)))
(when (:temporal-unit opts)
{:unit (:temporal-unit opts)})
(when (:base-type opts)
{:base_type (:base-type opts)})))
;; With Mlv2 queries, this could be combined with `:expression` below and use the column from the
;; query rather than metadata/field
(when (integer? id-or-name)
(type-info (lib.metadata/field (qp.store/metadata-provider) id-or-name)))
(when (:temporal-unit opts)
{:unit (:temporal-unit opts)})
(when (:base-type opts)
{:base_type (:base-type opts)})))

(defmethod type-info :expression [[_ _name opts]]
(merge
(when (isa? (:base-type opts) :type/Temporal)
{:unit :default})
(when (:temporal-unit opts)
{:unit (:temporal-unit opts)})
(when (:base-type opts)
{:base_type (:base-type opts)})))
(when (isa? (:base-type opts) :type/Temporal)
{:unit :default})
(when (:temporal-unit opts)
{:unit (:temporal-unit opts)})
(when (:base-type opts)
{:base_type (:base-type opts)})))

;;; ------------------------------------------------- add-type-info --------------------------------------------------

Expand Down Expand Up @@ -100,19 +103,129 @@
[this info & _]
[:absolute-datetime this (get info :unit :default)])

(defmulti ^:private coerce-temporal
"Coerce temporal value `t` to `target-class`, or throw an Exception if it is an invalid conversion."
{:arglists '([t target-class])}
(fn [t target-class]
[(class t) target-class]))

(defn- throw-invalid-conversion [message]
(throw (ex-info message {:type qp.error-type/invalid-query})))

(defn- throw-invalid-date []
(throw-invalid-conversion (i18n/tru "Invalid date literal: expected a date, got a time")))

(defmethod coerce-temporal [java.time.LocalDate java.time.LocalDate] [t _target-class] t)
(defmethod coerce-temporal [java.time.LocalTime java.time.LocalDate] [_t _target-class] (throw-invalid-date))
(defmethod coerce-temporal [java.time.OffsetTime java.time.LocalDate] [_t _target-class] (throw-invalid-date))
(defmethod coerce-temporal [java.time.LocalDateTime java.time.LocalDate] [t _target-class] (t/local-date t))
(defmethod coerce-temporal [java.time.OffsetDateTime java.time.LocalDate] [t _target-class] (t/local-date t))
(defmethod coerce-temporal [java.time.ZonedDateTime java.time.LocalDate] [t _target-class] (t/local-date t))

(defn- throw-invalid-time []
(throw-invalid-conversion (i18n/tru "Invalid time literal: expected a time, got a date")))

(defn- LocalTime->OffsetTime [t]
(if (= (qp.timezone/results-timezone-id) "UTC")
(t/offset-time t (t/zone-offset 0))
;; if the zone is something else, we'll just have to make do with a LocalTime, since there's no way to determine
;; what the appropriate offset to use for something like `US/Pacific` is for a give TIME with no DATE associated
;; with it.
t))

(defmethod coerce-temporal [LocalDate LocalTime] [_t _target-class] (throw-invalid-time))
(defmethod coerce-temporal [LocalTime LocalTime] [t _target-class] t)
(defmethod coerce-temporal [OffsetTime LocalTime] [t _target-class] (t/local-time t))
(defmethod coerce-temporal [LocalDateTime LocalTime] [t _target-class] (t/local-time t))
(defmethod coerce-temporal [OffsetDateTime LocalTime] [t _target-class] (t/local-time t))
(defmethod coerce-temporal [ZonedDateTime LocalTime] [t _target-class] (t/local-time t))

(defmethod coerce-temporal [LocalDate OffsetTime] [_t _target-class] (throw-invalid-time))
(defmethod coerce-temporal [LocalTime OffsetTime] [t _target-class] (LocalTime->OffsetTime t))
(defmethod coerce-temporal [OffsetTime OffsetTime] [t _target-class] t)
(defmethod coerce-temporal [LocalDateTime OffsetTime] [t target-class] (coerce-temporal (t/local-time t) target-class))
(defmethod coerce-temporal [OffsetDateTime OffsetTime] [t _target-class] (t/offset-time t))
(defmethod coerce-temporal [ZonedDateTime OffsetTime] [t _target-class] (t/offset-time t))

(defn- throw-invalid-datetime []
(throw-invalid-conversion (i18n/tru "Invalid datetime literal: expected a date or datetime, got a time")))

(defmethod coerce-temporal [LocalDate LocalDateTime] [t _target-class] (t/local-date-time t (t/local-time 0)))
(defmethod coerce-temporal [LocalTime LocalDateTime] [_t _target-class] (throw-invalid-datetime))
(defmethod coerce-temporal [OffsetTime LocalDateTime] [_t _target-class] (throw-invalid-datetime))
(defmethod coerce-temporal [LocalDateTime LocalDateTime] [t _target-class] t)
(defmethod coerce-temporal [OffsetDateTime LocalDateTime] [t _target-class] (t/local-date-time t))
(defmethod coerce-temporal [ZonedDateTime LocalDateTime] [t _target-class] (t/local-date-time t))

(defmethod coerce-temporal [LocalDate OffsetDateTime] [t target-class] (coerce-temporal (t/local-date-time t (t/local-time 0)) target-class))
(defmethod coerce-temporal [LocalTime OffsetDateTime] [_t _target-class] (throw-invalid-datetime))
(defmethod coerce-temporal [OffsetTime OffsetDateTime] [_t _target-class] (throw-invalid-datetime))
(defmethod coerce-temporal [LocalDateTime OffsetDateTime] [t _target-class] (t/offset-date-time t (qp.timezone/results-timezone-id)))
(defmethod coerce-temporal [OffsetDateTime OffsetDateTime] [t _target-class] t)
(defmethod coerce-temporal [ZonedDateTime OffsetDateTime] [t _target-class] (t/offset-date-time t))

(defmethod coerce-temporal [LocalDate ZonedDateTime] [t target-class] (coerce-temporal (t/local-date-time t (t/local-time 0)) target-class))
(defmethod coerce-temporal [LocalTime ZonedDateTime] [_t _target-class] (throw-invalid-datetime))
(defmethod coerce-temporal [OffsetTime ZonedDateTime] [_t _target-class] (throw-invalid-datetime))
(defmethod coerce-temporal [LocalDateTime ZonedDateTime] [t _target-class] (t/zoned-date-time t (t/zone-id (qp.timezone/results-timezone-id))))
(defmethod coerce-temporal [OffsetDateTime ZonedDateTime] [t _target-class] t) ; OffsetDateTime is perfectly fine.
(defmethod coerce-temporal [ZonedDateTime ZonedDateTime] [t _target-class] t)

(defn- parse-temporal-string-literal-to-class [s target-class]
(coerce-temporal (u.date/parse s) target-class))

(defmulti ^:private parse-temporal-string-literal
"Parse a temporal string literal like `2024-03-20` for use in a filter against a column with `effective-type`, e.g.
`:type/Date`. The effective-type of the target column affects what we parse the string as; for example we'd parse
the string above as a `LocalDate` for a `:type/Date` and a `OffsetDateTime` for a
`:type/DateTimeWithTZ`."
{:arglists '([effective-type s target-unit])}
(fn [effective-type _s _target-unit]
effective-type))

(defmethod parse-temporal-string-literal :default
[_effective-type s target-unit]
(let [t (u.date/parse s (qp.timezone/results-timezone-id))]
[:absolute-datetime t target-unit]))

(defmethod parse-temporal-string-literal :type/Date
[_effective-type s target-unit]
(let [t (parse-temporal-string-literal-to-class s LocalDate)]
[:absolute-datetime t target-unit]))

(defmethod parse-temporal-string-literal :type/Time
[_effective-type s target-unit]
(let [t (parse-temporal-string-literal-to-class s LocalTime)]
[:time t target-unit]))

(defmethod parse-temporal-string-literal :type/TimeWithTZ
[_effective-type s target-unit]
(let [t (parse-temporal-string-literal-to-class s OffsetTime)]
[:time t target-unit]))

(defmethod parse-temporal-string-literal :type/DateTime
[_effective-type s target-unit]
(let [t (parse-temporal-string-literal-to-class s LocalDateTime)]
[:absolute-datetime t target-unit]))

(defmethod parse-temporal-string-literal :type/DateTimeWithTZ
[_effective-type s target-unit]
(let [t (parse-temporal-string-literal-to-class s OffsetDateTime)]
[:absolute-datetime t target-unit]))

(defmethod parse-temporal-string-literal :type/DateTimeWithZoneID
[_effective-type s target-unit]
(let [t (parse-temporal-string-literal-to-class s ZonedDateTime)]
[:absolute-datetime t target-unit]))

(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
parse-datetime-strings?
(string? this))
;; 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]))
[s {:keys [unit], :as info} & {:keys [parse-datetime-strings?]
:or {parse-datetime-strings? true}}]
(if (and unit
parse-datetime-strings?)
(let [effective-type ((some-fn :effective_type :base_type) info)]
(parse-temporal-string-literal effective-type s unit))
[:value s info]))


;;; -------------------------------------------- wrap-literals-in-clause ---------------------------------------------
Expand Down

0 comments on commit 3a360f8

Please sign in to comment.