Fixes filters against datetime binned expressions (#38874)

* Fixes filters against datetime binned expressions

Fixes #33528

Turns out that `mbql/normalize` and `lib/normalize` behaved slightly
different and `[:expression "abc" {...}]` refs would drop their opts in
the former path. In order to properly query against binned datetimes
it's important that expression ref does not lose its type or else the
optimizer will not see that `time-interval` needs to convert to a
`between` rather than an `=`.

* Fix test

* Only keep specific keys on expression opts for these expression filters

* Don't run checkin dataset on snowflake or athena

src/metabase/mbql/normalize.cljc

@@ -86,12 +86,30 @@
   (cond-> [:aggregation aggregation-index]
     (some? option) (conj option)))
 
+(defn- normalize-ref-opts [opts]
+  (let [opts (normalize-tokens opts :ignore-path)]
+   (cond-> opts
+       (:base-type opts)     (update :base-type keyword)
+       (:temporal-unit opts) (update :temporal-unit keyword)
+       (:binning opts)       (update :binning (fn [binning]
+                                                (cond-> binning
+                                                  (:strategy binning) (update :strategy keyword)))))))
+
 (defmethod normalize-mbql-clause-tokens :expression
   ;; For expression references (`[:expression \"my_expression\"]`) keep the arg as is but make sure it is a string.
-  [[_ expression-name]]
-  [:expression (if (keyword? expression-name)
-                 (mbql.u/qualified-name expression-name)
-                 expression-name)])
+  [[_ expression-name opts]]
+  (let [expression [:expression (if (keyword? expression-name)
+                                  (mbql.u/qualified-name expression-name)
+                                  expression-name)]
+        opts (->> opts
+                  normalize-ref-opts
+                  ;; Only keep fields required for handling binned&datetime expressions (#33528)
+                  ;; Allowing added alias-info through here breaks
+                  ;; [[metabase.query-processor.util.nest-query-test/nest-expressions-ignore-source-queries-test]]
+                  (m/filter-keys #{:base-type :temporal-unit :binning})
+                  not-empty)]
+    (cond-> expression
+      opts (conj opts))))
 
 (defmethod normalize-mbql-clause-tokens :binning-strategy
   ;; For `:binning-strategy` clauses (which wrap other Field clauses) normalize the strategy-name and recursively
@@ -103,15 +121,9 @@
 
 (defmethod normalize-mbql-clause-tokens :field
   [[_ id-or-name opts]]
-  (let [opts (normalize-tokens opts :ignore-path)]
-    [:field
-     id-or-name
-     (cond-> opts
-       (:base-type opts)     (update :base-type keyword)
-       (:temporal-unit opts) (update :temporal-unit keyword)
-       (:binning opts)       (update :binning (fn [binning]
-                                                (cond-> binning
-                                                  (:strategy binning) (update :strategy keyword)))))]))
+  [:field
+   id-or-name
+   (normalize-ref-opts opts)])
 
 (defmethod normalize-mbql-clause-tokens :field-literal
   ;; Similarly, for Field literals, keep the arg as-is, but make sure it is a string."

test/metabase/mbql/normalize_test.cljc

@@ -558,37 +558,43 @@
 ;; this is also covered
 (t/deftest ^:parallel normalize-expressions-test
   (normalize-tests
-   "Expression names should get normalized to strings."
-   {{:query {"expressions" {:abc ["+" 1 2]}
-             :fields       [["expression" :abc]]}}
-    {:query {:expressions {"abc" [:+ 1 2]}
-             :fields      [[:expression "abc"]]}}}
+    "Expression names should get normalized to strings."
+    {{:query {"expressions" {:abc ["+" 1 2]}
+              :fields       [["expression" :abc]]}}
+     {:query {:expressions {"abc" [:+ 1 2]}
+              :fields      [[:expression "abc"]]}}}
 
-   "are expression names exempt from lisp-casing/lower-casing?"
-   {{"query" {"expressions" {:sales_tax ["-" ["field-id" 10] ["field-id" 20]]}}}
-    {:query {:expressions {"sales_tax" [:- [:field-id 10] [:field-id 20]]}}}}
+    "are expression names exempt from lisp-casing/lower-casing?"
+    {{"query" {"expressions" {:sales_tax ["-" ["field-id" 10] ["field-id" 20]]}}}
+     {:query {:expressions {"sales_tax" [:- [:field-id 10] [:field-id 20]]}}}}
 
-   "expressions should handle datetime arithemtics"
-   {{:query {:expressions {:prev_month ["+" ["field-id" 13] ["interval" -1 "month"]]}}}
-    {:query {:expressions {"prev_month" [:+ [:field-id 13] [:interval -1 :month]]}}}
+    "expressions should handle datetime arithemtics"
+    {{:query {:expressions {:prev_month ["+" ["field-id" 13] ["interval" -1 "month"]]}}}
+     {:query {:expressions {"prev_month" [:+ [:field-id 13] [:interval -1 :month]]}}}
 
-    {:query {:expressions {:prev_month ["-" ["field-id" 13] ["interval" 1 "month"] ["interval" 1 "day"]]}}}
-    {:query {:expressions {"prev_month" [:- [:field-id 13] [:interval 1 :month] [:interval 1 :day]]}}}
+     {:query {:expressions {:prev_month ["-" ["field-id" 13] ["interval" 1 "month"] ["interval" 1 "day"]]}}}
+     {:query {:expressions {"prev_month" [:- [:field-id 13] [:interval 1 :month] [:interval 1 :day]]}}}
 
-    {:query {:expressions {:datetime-diff ["datetime-diff" ["field" 1 nil] ["field" 2 nil] "month"]}}}
-    {:query {:expressions {"datetime-diff" [:datetime-diff [:field 1 nil] [:field 2 nil] :month]}}}
+     {:query {:expressions {:datetime-diff ["datetime-diff" ["field" 1 nil] ["field" 2 nil] "month"]}}}
+     {:query {:expressions {"datetime-diff" [:datetime-diff [:field 1 nil] [:field 2 nil] :month]}}}
 
-    {:query {:expressions {:datetime-add ["datetime-add" ["field" 1 nil] 1 "month"]}}}
-    {:query {:expressions {"datetime-add" [:datetime-add [:field 1 nil] 1 :month]}}}
+     {:query {:expressions {:datetime-add ["datetime-add" ["field" 1 nil] 1 "month"]}}}
+     {:query {:expressions {"datetime-add" [:datetime-add [:field 1 nil] 1 :month]}}}
 
-    {:query {:expressions {:datetime-subtract ["datetime-subtract" ["field" 1 nil] 1 "month"]}}}
-    {:query {:expressions {"datetime-subtract" [:datetime-subtract [:field 1 nil] 1 :month]}}}}
+     {:query {:expressions {:datetime-subtract ["datetime-subtract" ["field" 1 nil] 1 "month"]}}}
+     {:query {:expressions {"datetime-subtract" [:datetime-subtract [:field 1 nil] 1 :month]}}}}
 
-   "expressions handle namespaced keywords correctly"
-   {{:query {"expressions" {:abc/def ["+" 1 2]}
-             :fields       [["expression" :abc/def]]}}
-    {:query {:expressions {"abc/def" [:+ 1 2]}
-             :fields      [[:expression "abc/def"]]}}}))
+    "expressions handle namespaced keywords correctly"
+    {{:query {"expressions" {:abc/def ["+" 1 2]}
+              :fields       [["expression" :abc/def]]}}
+     {:query {:expressions {"abc/def" [:+ 1 2]}
+              :fields      [[:expression "abc/def"]]}}}
+
+    "expression refs can have opts (#33528)"
+    {{:query {:expressions {"abc" [+ 1 2]}
+              :fields      [[:expression "abc" {"base-type" "type/Number"}]]}}
+     {:query {:expressions {"abc" [+ 1 2]}
+              :fields      [[:expression "abc" {:base-type :type/Number}]]}}}))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1378,7 +1384,7 @@
 
 (t/deftest ^:parallel normalize-fragment-filter-test
   (t/is (= [:!=
-            [:expression "expr"]
+            [:expression "expr" {:base-type :type/Date}]
             [:field 66302 {:base-type :type/DateTime}]]
            (mbql.normalize/normalize-fragment
                [:query :filter]

test/metabase/query_processor_test/date_bucketing_test.clj

@@ -23,6 +23,7 @@
    [metabase.driver.sql-jdbc.sync :as sql-jdbc.sync]
    [metabase.driver.sql.query-processor :as sql.qp]
    [metabase.driver.sql.query-processor-test-util :as sql.qp-test-util]
+   [metabase.lib.convert :as lib.convert]
    [metabase.lib.core :as lib]
    [metabase.lib.metadata :as lib.metadata]
    [metabase.lib.metadata.jvm :as lib.metadata.jvm]
@@ -1343,6 +1344,28 @@
                  [[0]])
                (mt/formatted-rows [int] (qp/process-query query)))))))))
 
+(deftest filter-by-expression-time-interval-test
+  (testing "Datetime expressions can filter to a date range (#33528)"
+    (mt/test-drivers (mt/normal-drivers-except #{:snowflake :athena})
+      (mt/dataset
+        checkins:1-per-day
+        (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+              query (as-> (lib/query mp (lib.metadata/table mp (mt/id :checkins))) $q
+                      (lib/expression $q "customdate" (m/find-first (comp #{(mt/id :checkins :timestamp)} :id) (lib/visible-columns $q)))
+                      (lib/filter $q (lib/time-interval (lib/expression-ref $q "customdate") :current :week)))
+              mbql-query (mt/mbql-query
+                           checkins
+                           {:expressions {"customdate" $timestamp}
+                            :filter [:time-interval [:expression "customdate" {:base-type :type/DateTime}] :current :week]})
+              processed  (qp/process-query query)
+              mbql-processed (qp/process-query mbql-query)]
+          ;; Test both path ways since only mbql-queries were affected.
+          (is (= 7 (count (mt/rows processed))))
+          (is (= 7 (count (mt/rows mbql-processed))))
+          (is (= (get-in (qp/process-query mbql-query) [:data :native_form])
+                 (get-in (qp/process-query (lib.convert/->pMBQL mbql-query)) [:data :native_form])
+                 (get-in (qp/process-query query) [:data :native_form]))))))))
+
 ;; TODO -- is this really date BUCKETING? Does this BELONG HERE?!
 (deftest june-31st-test
   (testing "What happens when you try to add 3 months to March 31st? It should still work (#10072, #21968, #21969)"