Don't optimize temporal filters with unbucketed exprs against relative datetimes (#42310)

* Don't optimize [:between <expr w/ :default bucketing> <relative-datetime> ...] filters

* Update tests 

src/metabase/driver/h2.clj

@@ -283,7 +283,7 @@
          (h2x/literal unit)
          (if (number? amount)
            (sql.qp/inline-num (long amount))
-           (h2x/cast :long amount))
+           (h2x/cast-unless-type-in "integer" #{"long" "integer"} amount))
          expr]
         (h2x/with-database-type-info (h2x/database-type expr)))))
 

src/metabase/driver/sql/query_processor.clj

@@ -346,15 +346,13 @@
      (inline? offset) (recur driver day-of-week-honeysql-expr (second offset) mod-fn)
      (zero? offset)   day-of-week-honeysql-expr
      (neg? offset)    (recur driver day-of-week-honeysql-expr (+ offset 7) mod-fn)
-     :else            [:case
-                       [:=
-                        (mod-fn (h2x/+ day-of-week-honeysql-expr offset) (inline-num 7))
-                        (inline-num 0)]
-                       (inline-num 7)
-                       :else
-                       (mod-fn
-                        (h2x/+ day-of-week-honeysql-expr offset)
-                        (inline-num 7))])))
+     :else            (-> [:coalesce
+                           [:nullif
+                            (mod-fn (h2x/+ day-of-week-honeysql-expr offset) (inline-num 7))
+                            [:inline 0]]
+                           [:inline 7]]
+                          (h2x/with-database-type-info (or (h2x/database-type day-of-week-honeysql-expr)
+                                                           "integer"))))))
 
 (defmulti quote-style
   "Return the dialect that should be used by Honey SQL 2 when building a SQL statement. Defaults to `:ansi`, but other
@@ -883,9 +881,7 @@
     (recur (second denominator))
 
     :else
-    [:case
-     [:= denominator (inline-num 0)] nil
-     :else                           denominator]))
+    [:nullif denominator [:inline 0]]))
 
 (defmethod ->honeysql [:sql :/]
   [driver [_ & mbql-exprs]]

src/metabase/query_processor/middleware/optimize_temporal_filters.clj

@@ -182,15 +182,17 @@
   (let [target-unit (target-unit-for-new-bound unit temporal-unit)]
     [:absolute-datetime (temporal-literal-upper-bound target-unit t) :default]))
 
-(mu/defmethod temporal-value-lower-bound :relative-datetime :- mbql.s/relative-datetime
+(mu/defmethod temporal-value-lower-bound :relative-datetime :- [:maybe mbql.s/relative-datetime]
   [[_ n unit] temporal-unit]
-  (let [target-unit (target-unit-for-new-bound unit temporal-unit)]
-    [:relative-datetime (if (= n :current) 0 n) target-unit]))
+  (when-not (= temporal-unit :default)
+    (let [target-unit (target-unit-for-new-bound unit temporal-unit)]
+      [:relative-datetime (if (= n :current) 0 n) target-unit])))
 
-(mu/defmethod temporal-value-upper-bound :relative-datetime :- mbql.s/relative-datetime
+(mu/defmethod temporal-value-upper-bound :relative-datetime :- [:maybe mbql.s/relative-datetime]
   [[_ n unit] temporal-unit]
-  (let [target-unit (target-unit-for-new-bound unit temporal-unit)]
-    [:relative-datetime (inc (if (= n :current) 0 n)) target-unit]))
+  (when-not (= temporal-unit :default)
+    (let [target-unit (target-unit-for-new-bound unit temporal-unit)]
+      [:relative-datetime (inc (if (= n :current) 0 n)) target-unit])))
 
 (defn- date-field-with-day-bucketing? [x]
   (and (isa? (field-or-expression-effective-type x) :type/Date)

test/metabase/driver/mysql_test.clj

@@ -95,13 +95,15 @@
                                       [["CAST("
                                         "  1 + CEIL("
                                         "    ("
-                                        "      DAYOFYEAR(weeks.d) - ("
-                                        "        8 - CASE"
-                                        "          WHEN ((DAYOFWEEK(MAKEDATE(YEAR(weeks.d), 1)) + 5) % 7) = 0 THEN 7"
-                                        "          ELSE (DAYOFWEEK(MAKEDATE(YEAR(weeks.d), 1)) + 5) % 7"
-                                        "        END"
-                                        "      )"
-                                        "    ) / 7.0"
+                                        "      ("
+                                        "        DAYOFYEAR(weeks.d) - ("
+                                        "          8 - COALESCE("
+                                        "            NULLIF((DAYOFWEEK(MAKEDATE(YEAR(weeks.d), 1)) + 5) % 7, 0),"
+                                        "            7"
+                                        "          )"
+                                        "        )"
+                                        "      ) / 7.0"
+                                        "    )"
                                         "  ) AS signed"
                                         ")"]]}]
       (mt/with-temporary-setting-values [start-of-week start-of-week]

test/metabase/driver/sql/query_processor_test.clj

@@ -479,8 +479,7 @@
                 VENUES.PRICE       AS PRICE
                 CAST (VENUES.PRICE AS float)
                 /
-                CASE WHEN CategoriesStats.AvgPrice = 0 THEN NULL
-                ELSE CategoriesStats.AvgPrice END AS RelativePrice
+                NULLIF (CategoriesStats.AvgPrice, 0) AS RelativePrice
                 CategoriesStats.CATEGORY_ID AS CategoriesStats__CATEGORY_ID
                 CategoriesStats.MaxPrice    AS CategoriesStats__MaxPrice
                 CategoriesStats.AvgPrice    AS CategoriesStats__AvgPrice
@@ -784,9 +783,7 @@
     (is (= '{:select   [CAST
                         (VENUES.PRICE AS float)
                         /
-                        CASE WHEN (VENUES.PRICE + 2) = 0 THEN NULL
-                        ELSE VENUES.PRICE + 2
-                        END AS my_cool_new_field]
+                        NULLIF (VENUES.PRICE + 2, 0) AS my_cool_new_field]
              :from     [VENUES]
              :order-by [VENUES.ID ASC]
              :limit    [3]}

test/metabase/formatter/datetime_test.clj

@@ -9,7 +9,7 @@
 
 (def ^:private now "2020-07-16T18:04:00Z[UTC]")
 
-(deftest determine-time-format-test
+(deftest ^:parallel determine-time-format-test
   (testing "Capture the behaviors of determine-time-format"
     (testing "When :time-enabled is set to nil no time format is returned"
       (is (nil? (#'datetime/determine-time-format {:time-enabled nil}))))
@@ -242,7 +242,7 @@
         (is (= "15:30:45Z"
                (datetime/format-temporal-str "UTC" "15:30:45Z" col nil)))))))
 
-(deftest year-in-dates-near-start-or-end-of-year-is-correct-test
+(deftest ^:parallel year-in-dates-near-start-or-end-of-year-is-correct-test
   (testing "When the date is at the start/end of the year, the year is formatted properly. (#40306)"
     ;; Our datetime formatter relies on the `java-time.api`, for which there are many different, sometimes confusing,
     ;; formatter patterns: https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatterBuilder.html#appendPattern-java.lang.String-

test/metabase/query_processor/middleware/optimize_temporal_filters_test.clj

@@ -5,6 +5,8 @@
    [java-time.api :as t]
    [metabase.driver :as driver]
    [metabase.legacy-mbql.util :as mbql.u]
+   [metabase.lib.convert :as lib.convert]
+   [metabase.lib.core :as lib]
    [metabase.lib.test-metadata :as meta]
    [metabase.lib.test-util :as lib.tu]
    [metabase.lib.test-util.macros :as lib.tu.macros]
@@ -155,6 +157,34 @@
                      [:absolute-datetime filter-value unit]
                      [:absolute-datetime filter-value unit]])))))))))
 
+(deftest ^:parallel optimize-less-than-or-equal-to-relative-datetime-test
+  (testing "Optimize [:<= x <16 weeks ago>] correctly (#42291)"
+    (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
+                    (lib/with-fields [(meta/field-metadata :orders :id)])
+                    (lib/filter (lib/<=
+                                 (-> (meta/field-metadata :orders :created-at)
+                                     (lib/with-temporal-bucket :default))
+                                 (lib/relative-datetime -16 :week))))]
+      (is (=? {:query {:filter [:<=
+                                [:field (meta/id :orders :created-at) {:base-type :type/DateTimeWithLocalTZ, :temporal-unit :default}]
+                                [:relative-datetime -16 :week]]}}
+              (optimize-temporal-filters (lib.convert/->legacy-MBQL query)))))))
+
+(deftest ^:parallel optimize-between-relative-datetime-test
+  (testing "Optimize [:between x <17 weeks ago> <16 weeks ago>] correctly (#42291)"
+    (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
+                    (lib/with-fields [(meta/field-metadata :orders :id)])
+                    (lib/filter (lib/between
+                                 (-> (meta/field-metadata :orders :created-at)
+                                     (lib/with-temporal-bucket :default))
+                                 (lib/relative-datetime -17 :week)
+                                 (lib/relative-datetime -16 :week))))]
+      (is (=? {:query {:filter [:between
+                                [:field (meta/id :orders :created-at) {:base-type :type/DateTimeWithLocalTZ, :temporal-unit :default}]
+                                [:relative-datetime -17 :week]
+                                [:relative-datetime -16 :week]]}}
+              (optimize-temporal-filters (lib.convert/->legacy-MBQL query)))))))
+
 (defn- optimize-filter-clauses [t]
   (let [query {:database 1
                :type     :query