Merge pull request #9535 from metabase/insights-more-robust-datetime-unit-inference

Insights: make datatime unit inference more robust

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

@@ -134,9 +134,12 @@
        (= (count datetimes) 1)
        (empty? others)))
 
-(def ^:private ms->day
-  "We downsize UNIX timestamps to lessen the chance of overflows and numerical instabilities."
-  #(/ % (* 1000 60 60 24)))
+;; We downsize UNIX timestamps to lessen the chance of overflows and numerical instabilities.
+(def ^Long ^:const ^:private ms-in-a-day (* 1000 60 60 24))
+
+(defn- ms->day
+  [dt]
+  (/ dt ms-in-a-day))
 
 (defn- about=
   [a b]
@@ -151,13 +154,18 @@
    :quarter (* 30.4 3)
    :year    365.1})
 
+(defn- infer-unit
+  [from to]
+  (when (and from to)
+    (some (fn [[unit duration]]
+            (when (about= (- to from) duration)
+              unit))
+          unit->duration)))
+
 (defn- valid-period?
   [from to unit]
-  (when (and from to)
-    (let [delta (- to from)]
-      (if unit
-        (about= delta (unit->duration unit))
-        (some (partial about= delta) (vals unit->duration))))))
+  (when (and from to unit)
+    (about= (- to from) (unit->duration unit))))
 
 (defn- timeseries-insight
   [{:keys [numbers datetimes]}]
@@ -185,7 +193,10 @@
                             (stats/simple-linear-regression xfn yfn)
                             (best-fit xfn yfn)))
               (fn [[[y-previous y-current] [x-previous x-current] [offset slope] best-fit]]
-                (let [show-change? (valid-period? x-previous x-current (:unit datetime))]
+                (let [unit         (if (contains? #{:default nil} (:unit datetime))
+                                     (infer-unit x-previous x-current)
+                                     (:unit datetime))
+                      show-change? (valid-period? x-previous x-current unit)]
                   {:last-value     y-current
                    :previous-value (when show-change?
                                      y-previous)

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

@@ -35,12 +35,14 @@
       :last-value))
 
 
+(defn- inst->day
+  [inst]
+  (some-> inst (.getTime) (#'i/ms->day)))
+
 (defn- valid-period?
-  ([from to] (valid-period? from to nil))
+  ([from to] (valid-period? from to (#'i/infer-unit (inst->day from) (inst->day to))))
   ([from to period]
-   (boolean (#'i/valid-period? (some-> from (.getTime) (#'i/ms->day))
-                               (some-> to (.getTime) (#'i/ms->day))
-                               period))))
+   (boolean (#'i/valid-period? (inst->day from) (inst->day to) period))))
 
 (expect
   true
@@ -75,3 +77,6 @@
 (expect
   false
   (valid-period? #inst "2015-01-01" #inst "2015-04-03" :month))
+(expect
+  false
+  (valid-period? #inst "2015-01" #inst "2015-02" nil))