Errors combining datetime interval addition with datetime functions (#26279)

* Add failing tests

* Fix failing tests

* Update shared/src/metabase/mbql/util.cljc

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>

* datetime-add and datetime subtract should annotate type by col type

* Fix infer-expression-type for datetime-add/subtract with second, minute, hour

* Undo last commit; they actually always return :type/DateTime

* Fix test based on last commit

* Undo unrelated refactor

* Only test drivers that support expressions

* Only test drivers that support expressions, again

* Update tests from legacy mbql

* Change infered-col-type to be a function again, not macro

* Fix test

Co-authored-by: Ngoc Khuat <qn.khuat@gmail.com>

shared/src/metabase/mbql/schema.cljc

@@ -493,7 +493,7 @@
 
 (def date-arithmetic-functions
   "Functions to do math with date, datetime."
-  #{:datetime-add :datetime-subtract})
+  #{:+ :datetime-add :datetime-subtract})
 
 (def date+time+timezone-functions
   "Date, time, and timezone related functions."
@@ -526,18 +526,18 @@
 
 (def ^:private DateTimeExpressionArg
   (s/conditional
-    (partial is-clause? aggregations)
-    (s/recursive #'Aggregation)
+   (partial is-clause? aggregations)
+   (s/recursive #'Aggregation)
 
-    (partial is-clause? :value)
-    value
+   (partial is-clause? :value)
+   value
 
     ;; Recursively doing date math
-    (partial is-clause? date-arithmetic-functions)
-    (s/recursive #'DatetimeExpression)
+   (partial is-clause? date-arithmetic-functions)
+   (s/recursive #'DatetimeExpression)
 
-    :else
-    Field))
+   :else
+   Field))
 
 (def ^:private ExpressionArg
   (s/conditional
@@ -696,7 +696,7 @@
   unit     ArithmeticDateTimeUnit)
 
 (def ^:private DatetimeExpression*
-  (one-of temporal-extract datetime-add datetime-subtract
+  (one-of + temporal-extract datetime-add datetime-subtract
           ;; SUGAR drivers do not need to implement
           get-year get-quarter get-month get-week get-day get-day-of-week
           get-hour get-minute get-second))

shared/src/metabase/mbql/util.cljc

@@ -458,16 +458,6 @@
   (and (temporal-field? field)
        (not (time-field? field))))
 
-(defn datetime-arithmetics?
-  "Is a given artihmetics clause operating on datetimes?"
-  [clause]
-  (mbql.match/match-one clause
-    #{:interval :relative-datetime}
-    true
-
-    [:field _ (_ :guard :temporal-unit)]
-    true))
-
 
 ;;; --------------------------------- Unique names & transforming ags to have names ----------------------------------
 

shared/test/metabase/mbql/util_test.cljc

@@ -717,14 +717,6 @@
           (t/is (= expected
                    (mbql.u/query->max-rows-limit query))))))))
 
-(t/deftest ^:parallel datetime-arithmetics?-test
-  (t/is (mbql.u/datetime-arithmetics?
-         [:+ [:field-id 13] [:interval -1 :month]]))
-  (t/is (mbql.u/datetime-arithmetics?
-         [:field "a" {:temporal-unit :month}]))
-  (t/is (not (mbql.u/datetime-arithmetics?
-              [:+ [:field-id 13] 3]))))
-
 (t/deftest ^:parallel expression-with-name-test
   (t/is (= [:+ 1 1]
            (mbql.u/expression-with-name {:expressions  {"two" [:+ 1 1]}

src/metabase/driver/sql/query_processor.clj

@@ -506,11 +506,11 @@
   (hsql/call :power (->honeysql driver field) (->honeysql driver power)))
 
 (defn- interval? [expr]
-  (and (vector? expr) (= (first expr) :interval)))
+  (mbql.u/is-clause? :interval expr))
 
 (defmethod ->honeysql [:sql :+]
   [driver [_ & args]]
-  (if (mbql.u/datetime-arithmetics? args)
+  (if (some interval? args)
     (if-let [[field intervals] (u/pick-first (complement interval?) args)]
       (reduce (fn [hsql-form [_ amount unit]]
                 (add-interval-honeysql-form driver hsql-form amount unit))

src/metabase/query_processor/middleware/annotate.clj

@@ -8,6 +8,7 @@
             [metabase.mbql.predicates :as mbql.preds]
             [metabase.mbql.schema :as mbql.s]
             [metabase.mbql.util :as mbql.u]
+            [metabase.mbql.util.match :as mbql.match]
             [metabase.models.humanization :as humanization]
             [metabase.query-processor.error-type :as qp.error-type]
             [metabase.query-processor.reducible :as qp.reducible]
@@ -121,6 +122,22 @@
                     join-alias)]
     (format "%s → %s" qualifier field-display-name)))
 
+(defn- datetime-arithmetics?
+  "Helper for [[infer-expression-type]]. Returns true if a given clause returns a :type/DateTime type."
+  [clause]
+  (mbql.match/match-one clause
+
+    #{:datetime-add :datetime-subtract :relative-datetime}
+    true
+
+    [:field _ (_ :guard :temporal-unit)]
+    true
+
+    :+
+    (some (partial mbql.u/is-clause? :interval) (rest clause))
+
+    _ false))
+
 (declare col-info-for-field-clause)
 
 (def type-info-columns
@@ -159,7 +176,7 @@
            (select-keys (infer-expression-type expression) type-info-columns)))
        clauses))
 
-    (mbql.u/datetime-arithmetics? expression)
+    (datetime-arithmetics? expression)
     {:base_type :type/DateTime}
 
     (mbql.u/is-clause? mbql.s/string-expressions expression)

test/metabase/query_processor/middleware/annotate_test.clj

@@ -508,6 +508,22 @@
       (is (= {:base_type :type/Float}
              (infered-col-type [:case [[[:> [:field (mt/id :venues :price) nil] 2] [:+ [:field (mt/id :venues :price) nil] 1]]]]))))))
 
+(deftest ^:parallel datetime-arithmetics?-test
+  (is (#'annotate/datetime-arithmetics? [:+ [:field (mt/id :checkins :date) nil] [:interval -1 :month]]))
+  (is (#'annotate/datetime-arithmetics? [:field (mt/id :checkins :date) {:temporal-unit :month}]))
+  (is (not (#'annotate/datetime-arithmetics? [:+ 1 [:temporal-extract
+                                                    [:+ [:field (mt/id :checkins :date) nil] [:interval -1 :month]]
+                                                    :year]])))
+  (is (not (#'annotate/datetime-arithmetics? [:+ [:field (mt/id :checkins :date) nil] 3]))))
+
+(deftest temporal-extract-test
+  (is (= {:base_type :type/DateTime}
+         (infered-col-type [:datetime-add [:field (mt/id :checkins :date) nil] 2 :month])))
+  (is (= {:base_type :type/DateTime}
+         (infered-col-type [:datetime-add [:field (mt/id :checkins :date) nil] 2 :hour])))
+  (is (= {:base_type :type/DateTime}
+         (infered-col-type [:datetime-add [:field (mt/id :users :last_login) nil] 2 :month]))))
+
 (deftest test-string-extracts
   (is (= {:base_type :type/Text}
          (infered-col-type  [:trim "foo"])))

test/metabase/query_processor_test/date_time_zone_functions_test.clj

@@ -129,7 +129,6 @@
            (testing (format "extract %s function works as expected on %s column for driver %s" op col-type driver/*driver*)
              (is (= (set (expected-fn op)) (set (test-temporal-extract (query-fn op field-id))))))))))))
 
-
 (deftest temporal-extraction-with-filter-expresion-tests
   (mt/test-drivers (mt/normal-drivers-with-feature :temporal-extract)
     (mt/dataset times-mixed
@@ -163,6 +162,24 @@
         (testing title
           (is (= expected (test-temporal-extract query))))))))
 
+(deftest temporal-extraction-with-datetime-arithmetic-expression-tests
+  (mt/test-drivers (mt/normal-drivers-with-feature :temporal-extract :expressions)
+    (mt/dataset times-mixed
+      (doseq [{:keys [title expected query]}
+              [{:title    "Nested interval addition expression"
+                :expected [2005]
+                :query    {:expressions {"expr" [:abs [:get-year [:+ [:field (mt/id :times :dt) nil] [:interval 1 :year]]]]}
+                           :filter      [:= [:field (mt/id :times :index) nil] 1]
+                           :fields      [[:expression "expr"]]}}
+
+               {:title    "Interval addition nested in numeric addition"
+                :expected [2006]
+                :query    {:expressions {"expr" [:+ [:get-year [:+ [:field (mt/id :times :dt) nil] [:interval 1 :year]]] 1]}
+                           :filter      [:= [:field (mt/id :times :index) nil] 1]
+                           :fields      [[:expression "expr"]]}}]]
+        (testing title
+          (is (= expected (test-temporal-extract query))))))))
+
 (defmacro with-start-of-week
   "With start of week."
   [start-of-week & body]