Handle comparing ChronoLocalDateTime and Instants (#12834)

* Handle comparing ChronoLocalDateTime and Instants

During fingerprinting, we can end up trying to compare two datatypes
that are not comparable - ChronoLocalDateTime and Instant. Coerce the
ChronoLocalDateTime to a proper Instant for the comparison to work.

When earliest / latest are called, the acc parameter must be coerced to
a Temporal object also.

Resolves #12311

[ci all]

* making ->temporal recursive

* Get the timezone in an precedence order

When converting a LocalTime for fingerprinting, convert to an Instant
using the following precedence order:

* report timezone
* database timezone
* system timezone

[ci all]

* use UTC for timezones

src/metabase/query_processor/timezone.clj

@@ -61,7 +61,7 @@
   (.. (t/system-clock) getZone getId))
 
 (defn requested-timezone-id
-  "The timezone that we would *like* to run a query in, regardless of whether we are actaully able to do so. This is
+  "The timezone that we would *like* to run a query in, regardless of whether we are actually able to do so. This is
   always equal to the value of the `report-timezone` Setting (if it is set), otherwise the database timezone (if known),
   otherwise the system timezone."
   ^String []

src/metabase/sync/analyze/fingerprint/fingerprinters.clj

@@ -8,6 +8,7 @@
              [math :as math]]
             [medley.core :as m]
             [metabase.models.field :as field]
+            [metabase.query-processor.timezone :as qp.tz]
             [metabase.sync.analyze.classifiers.name :as classify.name]
             [metabase.sync.util :as sync-util]
             [metabase.util :as u]
@@ -17,6 +18,7 @@
             [redux.core :as redux])
   (:import com.bigml.histogram.Histogram
            com.clearspring.analytics.stream.cardinality.HyperLogLogPlus
+           [java.time.chrono ChronoLocalDateTime ChronoZonedDateTime]
            java.time.temporal.Temporal))
 
 (defn col-wise
@@ -157,6 +159,8 @@
               {:type {~(first field-type) fingerprint#}})))
          (trs "Error generating fingerprint for {0}" (sync-util/name-for-logging field#))))))
 
+(declare ->temporal)
+
 (defn- earliest
   ([] nil)
   ([acc]
@@ -182,9 +186,21 @@
 
 (extend-protocol ITemporalCoerceable
   nil      (->temporal [_]    nil)
-  String   (->temporal [this] (u.date/parse this))
-  Long     (->temporal [this] (t/instant this))
-  Integer  (->temporal [this] (t/instant this))
+  String   (->temporal [this] (->temporal (u.date/parse this)))
+  Long     (->temporal [this] (->temporal (t/instant this)))
+  Integer  (->temporal [this] (->temporal (t/instant this)))
+  ;; this is challenging, because ChronoLocalDate requires a ZoneOffset
+  ;; to work with. Ideally, we would use the database's ZoneOffset, but
+  ;; we don't have access to that here. Use the JVM's systemDefault to
+  ;; convert this.
+  ChronoLocalDateTime (->temporal [this] (.toInstant this
+                                                     (.. (if-let [tz (or (qp.tz/report-timezone-id-if-supported)
+                                                                         (qp.tz/database-timezone-id))]
+                                                           (java.time.ZoneId/of tz)
+                                                           (java.time.ZoneId/systemDefault))
+                                                         (getRules)
+                                                         (getOffset (java.time.Instant/now)))))
+  ChronoZonedDateTime (->temporal [this] (.toInstant this))
   Temporal (->temporal [this] this))
 
 (deffingerprinter :type/DateTime

test/metabase/sync/analyze/fingerprint/fingerprinters_test.clj

 (ns metabase.sync.analyze.fingerprint.fingerprinters-test
   (:require [clojure.test :refer :all]
             [metabase.models.field :as field]
-            [metabase.sync.analyze.fingerprint.fingerprinters :refer :all]))
+            [metabase.sync.analyze.fingerprint.fingerprinters :refer :all]
+            [metabase.test :as mt]))
 
 (deftest fingerprint-temporal-values-test
-  (is (= {:global {:distinct-count 4
-                   :nil%           0.5}
-          :type   {:type/DateTime {:earliest "2013-01-01"
-                                   :latest   "2018-01-01"}}}
-         (transduce identity
-                    (fingerprinter (field/map->FieldInstance {:base_type :type/DateTime}))
-                    [#t "2013" nil #t "2018" nil nil #t "2015"])))
-  (testing "nil temporal values"
-    {:global {:distinct-count 1
-              :nil%           1.0}
-     :type {:type/DateTime {:earliest nil
-                            :latest   nil}}}
-    (transduce identity
-               (fingerprinter (field/map->FieldInstance {:base_type :type/DateTime}))
-               (repeat 10 nil))))
+  ;; we want to test h2 and postgres, because h2 doesn't
+  ;; support overriding the timezone for a session / report
+  (mt/test-drivers #{:h2 :postgres}
+    (doseq [tz ["UTC" nil]]
+      (mt/with-temporary-setting-values [report-timezone tz]
+        (mt/with-database-timezone-id "UTC"
+          (mt/with-everything-store
+           (is (= {:global {:distinct-count 4
+                            :nil%           0.5}
+                   :type   {:type/DateTime {:earliest "2013-01-01"
+                                            :latest   "2018-01-01"}}}
+                  (transduce identity
+                             (fingerprinter (field/map->FieldInstance {:base_type :type/DateTime}))
+                             [#t "2013" nil #t "2018" nil nil #t "2015"])))
+           (testing "handle ChronoLocalDateTime"
+             (is (= {:global {:distinct-count 2
+                              :nil%           0.0}
+                     :type   {:type/DateTime {:earliest "2013-01-01T20:04:00Z"
+                                              :latest   "2018-01-01T04:04:00Z"}}}
+                    (transduce identity
+                               (fingerprinter (field/map->FieldInstance {:base_type :type/Temporal}))
+                               [(java.time.LocalDateTime/of 2013 01 01 20 04 0 0)
+                                (java.time.LocalDateTime/of 2018 01 01 04 04 0 0)]))))
+           (testing "handle comparing explicit Instant with ChronoLocalDateTime"
+             (is (= {:global {:distinct-count 2
+                              :nil%           0.0}
+                     :type   {:type/DateTime {:earliest "2007-12-03T10:15:30Z"
+                                              :latest   "2018-01-01T04:04:00Z"}}}
+                    (transduce identity
+                               (fingerprinter (field/map->FieldInstance {:base_type :type/Temporal}))
+                               [(java.time.Instant/parse "2007-12-03T10:15:30.00Z")
+                                (java.time.LocalDateTime/of 2018 01 01 04 04 0 0)]))))
+           (testing "mixing numbers and strings"
+             (is (= {:global {:distinct-count 2
+                              :nil%           0.0}
+                     :type   {:type/DateTime {:earliest "1970-01-01T00:00:01.234Z"
+                                              :latest   "2007-12-03T10:15:30Z"}}}
+                    (transduce identity
+                               (fingerprinter (field/map->FieldInstance {:base_type :type/Temporal}))
+                               ["2007-12-03T10:15:30.00Z" 1234]))))
+           (testing "nil temporal values"
+             (is (= {:global {:distinct-count 1
+                              :nil%           1.0}
+                     :type   {:type/DateTime {:earliest nil
+                                              :latest   nil}}}
+                    (transduce identity
+                               (fingerprinter (field/map->FieldInstance {:base_type :type/DateTime}))
+                               (repeat 10 nil)))))))))))
 
 (deftest disambiguate-test
   (testing "We should correctly disambiguate multiple competing multimethods (DateTime and FK in this case)"