From 8a571ac31090d231c7d86618a845c346778b3805 Mon Sep 17 00:00:00 2001 From: "metabase-bot[bot]" <109303359+metabase-bot[bot]@users.noreply.github.com> Date: Mon, 12 Feb 2024 16:29:28 +0000 Subject: [PATCH] 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> --- .../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 d0e0de4caf96b..38aeb8546c1da 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 b48a38a66b3f2..417cac7726df1 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))))))))))))