Sb fixes from master (#12315)

* Insights: fix `change` looping infinitely on input (0, -1) (#12282)

* Harden insights (#12289)

* Fix expressions test [ci drivers] (#12295)

* MBQL: add nil comparison tests (#12290)

* More test fixes (#12298)

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

@@ -108,7 +108,8 @@
        (reduced result#)
        result#)))
 
-(defn- with-error-handling
+(defn with-error-handling
+  "Wrap `rf` in an error-catching transducer."
   [rf msg]
   (fn
     ([] (with-reduced-error msg (rf)))

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

@@ -8,7 +8,10 @@
             [metabase.mbql.util :as mbql.u]
             [metabase.models.field :as field]
             [metabase.sync.analyze.fingerprint.fingerprinters :as f]
-            [metabase.util.date-2 :as u.date]
+            [metabase.sync.util :as sync-util]
+            [metabase.util
+             [date-2 :as u.date]
+             [i18n :refer [trs]]]
             [redux.core :as redux])
   (:import [java.time Instant LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime]))
 
@@ -32,7 +35,7 @@
       (cond
         (every? neg? [x1 x2])     (change (- x1) (- x2))
         (and (neg? x1) (pos? x2)) (- (change x1 x2))
-        (neg? x1)                 (- (change x2 x1))
+        (neg? x1)                 (- (change x2 (- x1)))
         :else                     (/ (- x2 x1) x1)))))
 
 (defn reservoir-sample
@@ -178,8 +181,6 @@
       Instant        (t/to-millis-from-epoch t)
       OffsetDateTime (t/to-millis-from-epoch t)
       ZonedDateTime  (t/to-millis-from-epoch t)
-      ;; TODO - really not convinced this behavior makes sense. Not sure what `xfn` below is actually supposed to be
-      ;; doing? This roughly matches the old behavior when we were using `java.util.Date`.
       LocalDate      (->millis-from-epoch (t/offset-date-time t (t/local-time 0) (t/zone-offset 0)))
       LocalDateTime  (->millis-from-epoch (t/offset-date-time t (t/zone-offset 0)))
       LocalTime      (->millis-from-epoch (t/offset-date-time (t/local-date "1970-01-01") t (t/zone-offset 0)))
@@ -195,31 +196,34 @@
                             f/->temporal
                             ->millis-from-epoch
                             ms->day)]
-    (apply redux/juxt
-           (for [number-col numbers]
-             (redux/post-complete
-              (let [y-position (:position number-col)
-                    yfn        #(nth % y-position)]
-                (redux/juxt ((map yfn) (last-n 2))
-                            ((map xfn) (last-n 2))
-                            (stats/simple-linear-regression xfn yfn)
-                            (best-fit xfn yfn)))
-              (fn [[[y-previous y-current] [x-previous x-current] [offset slope] best-fit]]
-                (let [unit         (if (or (nil? (:unit datetime))
-                                           (->> datetime :unit mbql.u/normalize-token (= :default)))
-                                     (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)
-                   :last-change    (when show-change?
-                                     (change y-current y-previous))
-                   :slope          slope
-                   :offset         offset
-                   :best-fit       best-fit
-                   :col            (:name number-col)
-                   :unit           unit})))))))
+    (f/with-error-handling
+      (apply redux/juxt
+             (for [number-col numbers]
+               (redux/post-complete
+                (let [y-position (:position number-col)
+                      yfn        #(nth % y-position)]
+                  (redux/juxt ((map yfn) (last-n 2))
+                              ((map xfn) (last-n 2))
+                              (stats/simple-linear-regression xfn yfn)
+                              (best-fit xfn yfn)))
+                (fn [[[y-previous y-current] [x-previous x-current] [offset slope] best-fit]]
+                  (let [unit         (if (or (nil? (:unit datetime))
+                                             (->> datetime :unit mbql.u/normalize-token (= :default)))
+                                       (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)
+                     :last-change    (when show-change?
+                                       (change y-current y-previous))
+                     :slope          slope
+                     :offset         offset
+                     :best-fit       best-fit
+                     :col            (:name number-col)
+                     :unit           unit})))))
+      (trs "Error generating timeseries insight keyed by: {0}"
+           (sync-util/name-for-logging (field/map->FieldInstance datetime))))))
 
 (defn insights
   "Based on the shape of returned data construct a transducer to statistically analyize data."

src/metabase/sync/analyze/query_results.clj

@@ -90,4 +90,5 @@
                            (assoc metadata :fingerprint fingerprint)))
                        fingerprints
                        cols)
-        :insights insights}))))
+        :insights (when-not (instance? Throwable insights)
+                    insights)}))))

test/metabase/query_processor_test/expressions_test.clj

@@ -249,14 +249,15 @@
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 (deftest expressions+joins-test
-  (datasets/test-drivers (mt/normal-drivers-with-feature :expressions)
+  (datasets/test-drivers (mt/normal-drivers-with-feature :expressions :left-join)
     (testing "Do calculated columns play well with joins"
-      (is (= "Quentin Sören"
+      (is (= "Simcha Yan"
              (-> (mt/run-mbql-query checkins
                    {:expressions {:prev_month [:+ $date [:interval -31 :day]]}
                     :fields      [[:joined-field "users__via__user_id" [:field-id (data/id :users :name)]]
                                   [:expression :prev_month]]
                     :limit       1
+                    :order-by    [[:asc $date]]
                     :joins       [{:strategy :left-join
                                    :source-table (data/id :users)
                                    :alias        "users__via__user_id"

test/metabase/query_processor_test/filter_test.clj

@@ -49,35 +49,44 @@
     nil false
     x))
 
-;;; filter = true
-(qp.test/expect-with-non-timeseries-dbs
-  [[1 "Tempest" true]
-   [2 "Bullit"  true]]
-  (qp.test/formatted-rows [int str ->bool] :format-nil-values
-    (data/dataset places-cam-likes
-      (data/run-mbql-query places
-        {:filter   [:= $liked true]
-         :order-by [[:asc $id]]}))))
-
-;;; filter != false
-(qp.test/expect-with-non-timeseries-dbs
-  [[1 "Tempest" true]
-   [2 "Bullit"  true]]
-  (qp.test/formatted-rows [int str ->bool] :format-nil-values
-    (data/dataset places-cam-likes
-      (data/run-mbql-query places
-        {:filter   [:!= $liked false]
-         :order-by [[:asc $id]]}))))
-
-;;; filter != true
-(qp.test/expect-with-non-timeseries-dbs
-  [[3 "The Dentist" false]]
-  (qp.test/formatted-rows [int str ->bool] :format-nil-values
-    (data/dataset places-cam-likes
-      (data/run-mbql-query places
-        {:filter   [:!= $liked true]
-         :order-by [[:asc $id]]}))))
-
+(deftest comparison-test
+  (datasets/test-drivers (qp.test/normal-drivers)
+    (testing "Can we use true literal in comparisons"
+      (is (= [[1 "Tempest" true]
+              [2 "Bullit"  true]]
+             (->> {:filter   [:= $liked true]
+                   :order-by [[:asc $id]]}
+                  (data/run-mbql-query places)
+                  (data/dataset places-cam-likes)
+                  (qp.test/formatted-rows [int str ->bool] :format-nil-values))))
+      (is (= [[3 "The Dentist" false]]
+             (->> {:filter   [:!= $liked true]
+                   :order-by [[:asc $id]]}
+                  (data/run-mbql-query places)
+                  (data/dataset places-cam-likes)
+                  (qp.test/formatted-rows [int str ->bool] :format-nil-values)))))
+    (testing "Can we use false literal in comparisons"
+      (is (= [[1 "Tempest" true]
+              [2 "Bullit"  true]]
+             (->> {:filter   [:!= $liked false]
+                   :order-by [[:asc $id]]}
+                  (data/run-mbql-query places)
+                  (data/dataset places-cam-likes)
+                  (qp.test/formatted-rows [int str ->bool] :format-nil-values)))))
+    (testing "Can we use nil literal in comparisons"
+      (is (= [[3]] (->> {:filter      [:!= $liked nil]
+                         :aggregation [[:count]]}
+                        (data/run-mbql-query places)
+                        (data/dataset places-cam-likes)
+                        (qp.test/formatted-rows [int]))))
+      (is (= true (->> {:filter      [:= $liked nil]
+                        :aggregation [[:count]]}
+                       (data/run-mbql-query places)
+                       (data/dataset places-cam-likes)
+                       (qp.test/formatted-rows [int])
+                       first
+                       ;; Some DBs like Mongo don't return any results at all in this case, and there's no easy workaround
+                       (contains? #{[0] [0M] [nil] nil})))))))
 
 (deftest between-test
   (datasets/test-drivers (qp.test/normal-drivers)

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

@@ -117,3 +117,15 @@
   (transduce identity
              (insights [{:base_type :type/DateTime} {:base_type :type/Number} {:base_type :type/Number}])
              ts))
+
+(deftest change-test
+  (is (= 0.0 (change 1 1)))
+  (is (= -0.5 (change 1 2)))
+  (is (= 1.0 (change 2 1)))
+  (is (= nil (change 1 0)))
+  (is (= -1.0 (change 0 1)))
+  (is (= 2.0 (change 1 -1)))
+  (is (= -2.0 (change -1 1)))
+  (is (= 1.0 (change -1 -2)))
+  (is (= nil (change -1 0)))
+  (is (= 1.0 (change 0 -1))))

test/metabase/sync/analyze/query_results_test.clj

@@ -9,10 +9,15 @@
             [metabase.mbql.schema :as mbql.s]
             [metabase.models.card :refer [Card]]
             [metabase.query-processor.test-util :as qp.test-util]
-            [metabase.sync.analyze.fingerprint.fingerprinters :as fprint]
+            [metabase.sync.analyze.fingerprint
+             [fingerprinters :as fprint]
+             [insights :as insights]]
             [metabase.sync.analyze.query-results :as qr]
-            [metabase.test.mock.util :as mock.u]
-            [metabase.test.util :as tu]))
+            [metabase.test
+             [data :as data]
+             [sync :as sync-test]
+             [util :as tu]]
+            [metabase.test.mock.util :as mock.u]))
 
 (defn- column->name-keyword [field-or-column-metadata]
   (-> field-or-column-metadata
@@ -99,3 +104,20 @@
     (mt/with-temp Card [card {:dataset_query {:database (mt/id), :type :native, :native {:query "select longitude from venues"}}}]
       (is (= (select-keys mock.u/venue-fingerprints [:longitude])
              (name->fingerprints (query->result-metadata (query-for-card card))))))))
+
+(defn- timeseries-dataset
+  []
+  (->> {:aggregation [[:count]]
+        :breakout    [[:datetime-field [:field-id (data/id :checkins :date)] :month]]}
+       (mt/run-mbql-query checkins)
+       :data))
+
+(deftest error-resilience-test
+  (testing "Data should come back even if there is an error during fingerprinting"
+    (is (= 36 (mt/suppress-output
+                (with-redefs [fprint/earliest sync-test/crash-fn]
+                  (-> (timeseries-dataset) :rows count))))))
+  (testing "Data should come back even if there is an error when calculating insights"
+    (is (= 36 (mt/suppress-output
+                (with-redefs [insights/change sync-test/crash-fn]
+                  (-> (timeseries-dataset) :rows count)))))))