From 1f4965d6b24a86666fc8b80d02d8b56f2c5202c0 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Mon, 12 Feb 2024 18:24:21 +0300
Subject: [PATCH] Compare datetimes w/o timezone fields with literals as
strings (#38642)
* Compare datetimes w/o timezone fields with literals as strings
---
.../src/metabase/driver/sqlserver.clj | 57 ++++++++++++++++++-
.../test/metabase/driver/sqlserver_test.clj | 51 +++++++++++++++++
2 files changed, 106 insertions(+), 2 deletions(-)
diff --git a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
index d0e0de4caf9..38aeb8546c1 100644
--- a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
+++ b/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
diff --git a/modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj b/modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj
index b48a38a66b3..417cac7726d 100644
--- a/modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj
+++ b/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))))))))))))
--
GitLab