Compare datetimes w/o timezone fields with literals as strings (#38642)…

… (#38671)

* Compare datetimes w/o timezone fields with literals as strings

Co-authored-by: metamben <103100869+metamben@users.noreply.github.com>

modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj

@@ -25,7 +25,8 @@
    [metabase.util.log :as log])
   (:import
    (java.sql Connection ResultSet Time)
-   (java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime)))
+   (java.time LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime)
+   (java.time.format DateTimeFormatter)))
 
 (set! *warn-on-reflection* true)
 
@@ -281,7 +282,7 @@
 
 (defonce
   ^{:private true
-    :doc     "A map of all zone-id to the corresponding window-zone.
+    :doc     "A map of all zone-id to the corresponding windows-zone.
              I.e {\"Asia/Tokyo\" \"Tokyo Standard Time\"}"}
   zone-id->windows-zone
   (let [data (-> (io/resource "timezones/windowsZones.xml")
@@ -528,6 +529,58 @@
   [driver [_ arg]]
   (sql.qp/->honeysql driver [:percentile arg 0.5]))
 
+(def ^:private ^:dynamic *compared-field-options*
+  "This variable is set to the options of the field we are comparing
+  (presumably in a filter)."
+  nil)
+
+(defn- timezoneless-comparison?
+  "Returns if we are currently comparing a timezoneless data type."
+  []
+  (contains? #{:type/DateTime :type/Time} (:base-type *compared-field-options*)))
+
+;; For some strange reason, comparing a datetime or datetime2 column
+;; against a Java LocaDateTime object sometimes doesn't work (see
+;; [[metabase.driver.sqlserver-test/filter-by-datetime-fields-test]]).
+;; Instead of this, we format a string which SQL Server then parses. Ugly.
+
+(defn- format-without-trailing-zeros
+  "Since there is no pattern for fractional seconds that produces a variable
+  number of digits, we remove any trailing zeros. The resulting string then
+  can be parsed as any data type supporting the required precision. (E.g.,
+  datetime support 3 fractional digits, datetime2 supports 7. If we read a
+  datetime value and then send back as a filter value, it will be formatted
+  with 7 fractional digits and then the zeros get removed so that SQL Server
+  can parse the result as a datetime value.)"
+  [value  ^DateTimeFormatter formatter]
+  (-> (.format formatter value)
+      (str/replace #"\.?0*$" "")))
+
+(def ^:private ^DateTimeFormatter time-format
+  (DateTimeFormatter/ofPattern "HH:mm:ss.SSSSSSS"))
+
+(defmethod sql.qp/->honeysql [:sqlserver OffsetTime]
+  [_driver t]
+  (cond-> t
+    (timezoneless-comparison?) (format-without-trailing-zeros time-format)))
+
+(def ^:private ^DateTimeFormatter datetime-format
+  (DateTimeFormatter/ofPattern "y-MM-dd HH:mm:ss.SSSSSSS"))
+
+(doseq [c [OffsetDateTime ZonedDateTime]]
+  (defmethod sql.qp/->honeysql [:sqlserver c]
+    [_driver t]
+    (cond-> t
+      (timezoneless-comparison?) (format-without-trailing-zeros datetime-format))))
+
+(doseq [op [:= :!= :< :<= :> :>= :between]]
+  (defmethod sql.qp/->honeysql [:sqlserver op]
+    [driver [_ field :as clause]]
+    (binding [*compared-field-options* (when (and (vector? field)
+                                                  (= (get field 0) :field))
+                                         (get field 2))]
+      ((get-method sql.qp/->honeysql [:sql-jdbc op]) driver clause))))
+
 (defmethod driver/db-default-timezone :sqlserver
   [driver database]
   (sql-jdbc.execute/do-with-connection-with-options

modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj

@@ -16,8 +16,10 @@
    [metabase.models :refer [Database]]
    [metabase.query-processor :as qp]
    [metabase.query-processor.interface :as qp.i]
+   [metabase.query-processor.test-util :as qp.test-util]
    [metabase.query-processor.timezone :as qp.timezone]
    [metabase.test :as mt]
+   [metabase.test.util.timezone :as test.tz]
    [toucan2.tools.with-temp :as t2.with-temp]))
 
 (set! *warn-on-reflection* true)
@@ -403,3 +405,52 @@
                           qp/process-userland-query
                           mt/rows
                           ffirst))))))))
+
+(deftest filter-by-datetime-fields-test
+  (mt/test-driver :sqlserver
+    (testing "Should match datetime fields even in non-default timezone (#30454)"
+      (mt/dataset attempted-murders
+        (let [limit 10
+              get-query (mt/mbql-query attempts
+                          {:fields [$id $datetime]
+                           :order-by [[:asc $id]]
+                           :limit limit})
+              filter-query (mt/mbql-query attempts
+                             {:fields [$id $datetime]
+                              :filter [:= [:field %attempts.datetime {:base-type :type/DateTime}]]
+                              :order-by [[:asc $id]]
+                              :limit limit})]
+          (doseq [with-tz-setter [#'qp.test-util/do-with-report-timezone-id
+                                  #'test.tz/do-with-system-timezone-id
+                                  #'qp.test-util/do-with-database-timezone-id
+                                  #'qp.test-util/do-with-results-timezone-id]
+                  timezone ["UTC" "Pacific/Auckland"]]
+            (testing (str with-tz-setter " " timezone)
+              (with-tz-setter timezone
+                (fn []
+                  (let [expected-result (-> get-query qp/process-query mt/rows)
+                        filter-query (update-in filter-query [:query :filter] into (map second) expected-result)]
+                    (mt/with-native-query-testing-context filter-query
+                      (is (= expected-result
+                             (-> filter-query qp/process-query mt/rows))))))))))))
+    #_(testing "Demo that filtering datetime fields by localdatetime objects doesn't work"
+        (mt/dataset attempted-murders
+          (let [details (mt/dbdef->connection-details :sqlserver :db {:database-name "attempted-murders"})
+                tricky-datetime "2019-11-02T00:14:14.247Z"
+                datetime-string (-> tricky-datetime
+                                    (str/replace #"T" " ")
+                                    (str/replace #"0*Z$" ""))
+                datetime-localdatetime (-> tricky-datetime
+                                           t/offset-date-time
+                                           t/local-date-time)
+                base-query "SELECT id FROM [attempts] WHERE datetime = ?"]
+            (doseq [param [datetime-string datetime-localdatetime]
+                    :let [query [base-query param]]]
+              (testing (pr-str query)
+                (is (= [{:id 2}]
+                       (sql-jdbc.execute/do-with-connection-with-options
+                        :sqlserver
+                        (sql-jdbc.conn/connection-details->spec :sqlserver details)
+                        {}
+                        (fn [^java.sql.Connection conn]
+                          (next.jdbc/execute! conn query))))))))))))