From 16ec3abacef13ff1b863d9963f7ff5f6ffbdc34a Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Fri, 8 Feb 2019 16:59:42 -0800
Subject: [PATCH] Fix SQLServer datetime grouping for locales with yyyy-dd-MM
dates [ci sqlserver]
---
.../src/metabase/driver/sqlserver.clj | 59 ++++++++++++++-----
.../test/metabase/driver/sqlserver_test.clj | 26 +++++++-
.../test/metabase/test/data/sqlserver.clj | 6 +-
src/metabase/util/honeysql_extensions.clj | 1 +
4 files changed, 73 insertions(+), 19 deletions(-)
diff --git a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
index adfd16449f3..34f9feae4e6 100644
--- a/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
+++ b/modules/drivers/sqlserver/src/metabase/driver/sqlserver.clj
@@ -91,21 +91,23 @@
(defn- date-add [unit & exprs]
(apply hsql/call :dateadd (hsql/raw (name unit)) exprs))
-;; See [this page](https://msdn.microsoft.com/en-us/library/ms187752.aspx) for details on the functions we're using.
-
-(defmethod sql.qp/date [:sqlserver :default] [_ _ expr] expr)
-(defmethod sql.qp/date [:sqlserver :minute] [_ _ expr] (hx/cast :smalldatetime expr))
-(defmethod sql.qp/date [:sqlserver :minute-of-hour] [_ _ expr] (date-part :minute expr))
-(defmethod sql.qp/date [:sqlserver :hour] [_ _ expr] (hx/->datetime (hx/format "yyyy-MM-dd HH:00:00" expr)))
-(defmethod sql.qp/date [:sqlserver :hour-of-day] [_ _ expr] (date-part :hour expr))
-(defmethod sql.qp/date [:sqlserver :day-of-week] [_ _ expr] (date-part :weekday expr))
-(defmethod sql.qp/date [:sqlserver :day-of-month] [_ _ expr] (date-part :day expr))
-(defmethod sql.qp/date [:sqlserver :day-of-year] [_ _ expr] (date-part :dayofyear expr))
-(defmethod sql.qp/date [:sqlserver :week-of-year] [_ _ expr] (date-part :iso_week expr))
-(defmethod sql.qp/date [:sqlserver :month] [_ _ expr] (hx/->datetime (hx/format "yyyy-MM-01" expr)))
-(defmethod sql.qp/date [:sqlserver :month-of-year] [_ _ expr] (date-part :month expr))
-(defmethod sql.qp/date [:sqlserver :quarter-of-year] [_ _ expr] (date-part :quarter expr))
-(defmethod sql.qp/date [:sqlserver :year] [_ _ expr] (date-part :year expr))
+;; See https://docs.microsoft.com/en-us/sql/t-sql/functions/date-and-time-data-types-and-functions-transact-sql for
+;; details on the functions we're using.
+
+(defmethod sql.qp/date [:sqlserver :default] [_ _ expr]
+ expr)
+
+(defmethod sql.qp/date [:sqlserver :minute] [_ _ expr]
+ (hx/cast :smalldatetime expr))
+
+(defmethod sql.qp/date [:sqlserver :minute-of-hour] [_ _ expr]
+ (date-part :minute expr))
+
+(defmethod sql.qp/date [:sqlserver :hour] [_ _ expr]
+ (hsql/call :datetime2fromparts (hx/year expr) (hx/month expr) (hx/day expr) (date-part :hour expr) 0 0 0 0))
+
+(defmethod sql.qp/date [:sqlserver :hour-of-day] [_ _ expr]
+ (date-part :hour expr))
;; jTDS is wack; I sense an ongoing theme here. It returns DATEs as strings instead of as java.sql.Dates like every
;; other SQL DB we support. Work around that by casting to DATE for truncation then back to DATETIME so we get the
@@ -116,6 +118,15 @@
(defmethod sql.qp/date [:sqlserver :day] [_ _ expr]
(hx/->datetime (hx/->date expr)))
+(defmethod sql.qp/date [:sqlserver :day-of-week] [_ _ expr]
+ (date-part :weekday expr))
+
+(defmethod sql.qp/date [:sqlserver :day-of-month] [_ _ expr]
+ (date-part :day expr))
+
+(defmethod sql.qp/date [:sqlserver :day-of-year] [_ _ expr]
+ (date-part :dayofyear expr))
+
;; Subtract the number of days needed to bring us to the first day of the week, then convert to date
;; The equivalent SQL looks like:
;; CAST(DATEADD(day, 1 - DATEPART(weekday, %s), CAST(%s AS DATE)) AS DATETIME)
@@ -125,13 +136,29 @@
(hx/- 1 (date-part :weekday expr))
(hx/->date expr))))
+(defmethod sql.qp/date [:sqlserver :week-of-year] [_ _ expr]
+ (date-part :iso_week expr))
+
+(defmethod sql.qp/date [:sqlserver :month] [_ _ expr]
+ (hsql/call :datefromparts (hx/year expr) (hx/month expr) 1))
+
+(defmethod sql.qp/date [:sqlserver :month-of-year] [_ _ expr]
+ (date-part :month expr))
+
;; Format date as yyyy-01-01 then add the appropriate number of quarter
;; Equivalent SQL:
;; DATEADD(quarter, DATEPART(quarter, %s) - 1, FORMAT(%s, 'yyyy-01-01'))
(defmethod sql.qp/date [:sqlserver :quarter] [_ _ expr]
(date-add :quarter
(hx/dec (date-part :quarter expr))
- (hx/format "yyyy-01-01" expr)))
+ (hsql/call :datefromparts (hx/year expr) 1 1)))
+
+(defmethod sql.qp/date [:sqlserver :quarter-of-year] [_ _ expr]
+ (date-part :quarter expr))
+
+(defmethod sql.qp/date [:sqlserver :year] [_ _ expr]
+ (date-part :year expr))
+
(defmethod driver/date-interval :sqlserver [_ unit amount]
(date-add unit amount :%getdate))
diff --git a/modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj b/modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj
index e8dfafef67d..9a333cdeb15 100644
--- a/modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj
+++ b/modules/drivers/sqlserver/test/metabase/driver/sqlserver_test.clj
@@ -1,19 +1,22 @@
(ns metabase.driver.sqlserver-test
- (:require [clojure.string :as str]
+ (:require [clojure.java.jdbc :as jdbc]
+ [clojure.string :as str]
[expectations :refer [expect]]
+ [honeysql.core :as hsql]
[medley.core :as m]
[metabase
[driver :as driver]
[query-processor :as qp]
[query-processor-test :as qp.test]]
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
+ [metabase.driver.sql.query-processor :as sql.qp]
[metabase.query-processor.test-util :as qp.test-util]
[metabase.test
[data :as data]
[util :as tu :refer [obj->json->obj]]]
[metabase.test.data
[datasets :as datasets]
- [interface :refer [def-database-definition]]]))
+ [interface :as tx :refer [def-database-definition]]]))
;;; -------------------------------------------------- VARCHAR(MAX) --------------------------------------------------
@@ -142,3 +145,22 @@
:order-by [[:asc $id]]
:limit 5}
:limit 3}}))))
+
+;; Make sure datetime bucketing functions work properly with languages that format dates like yyyy-dd-MM instead of
+;; yyyy-MM-dd (i.e. not American English) (#9057)
+(datasets/expect-with-driver :sqlserver
+ [{:my-date #inst "2019-02-01T00:00:00.000-00:00"}]
+ ;; we're doing things here with low-level calls to HoneySQL (emulating what the QP does) instead of using normal QP
+ ;; pathways because `SET LANGUAGE` doesn't seem to persist to subsequent executions so to test that things are
+ ;; working we need to add to in from of the query we're trying to check
+ (jdbc/with-db-transaction [t-conn (sql-jdbc.conn/connection-details->spec :sqlserver
+ (tx/dbdef->connection-details :sqlserver :db {:database-name "test-data"}))]
+ (try
+ (jdbc/execute! t-conn "CREATE TABLE temp (d DATETIME2);")
+ (jdbc/execute! t-conn ["INSERT INTO temp (d) VALUES (?)" #inst "2019-02-08T00:00:00Z"])
+ (jdbc/query t-conn (let [[sql & args] (hsql/format {:select [[(sql.qp/date :sqlserver :month :temp.d) :my-date]]
+ :from [:temp]}
+ :quoting :ansi, :allow-dashed-names? true)]
+ (cons (str "SET LANGUAGE Italian; " sql) args)))
+ ;; rollback transaction so `temp` table gets discarded
+ (finally (.rollback (jdbc/get-connection t-conn))))))
diff --git a/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj b/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj
index 90948d2ffda..57d61c7006b 100644
--- a/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj
+++ b/modules/drivers/sqlserver/test/metabase/test/data/sqlserver.clj
@@ -68,6 +68,9 @@
;; Clean up any leftover DBs that weren't destroyed by the last test run (eg, if it failed for some reason). This is
;; important because we're limited to a quota of 30 DBs on RDS.
+;;
+;; This doesn't kill databases with active connections (i.e. CI instances testing against them) -- `DROP DATABASE`
+;; will fail if the DB has open connections
(defmethod tx/before-run :sqlserver [_]
(let [connection-spec (sql-jdbc.conn/connection-details->spec :sqlserver
(tx/dbdef->connection-details :sqlserver :server nil))
@@ -80,6 +83,7 @@
(doseq [db leftover-dbs]
(u/ignore-exceptions
(printf "Deleting leftover SQL Server DB '%s'...\n" db)
- ;; Don't try to kill other connections to this DB with SET SINGLE_USER -- some other instance (eg CI) might be using it
+ ;; Don't try to kill other connections to this DB with SET SINGLE_USER -- some other instance (eg CI) might
+ ;; be using it
(jdbc/execute! connection-spec [(format "DROP DATABASE \"%s\";" db)])
(println "[ok]"))))))
diff --git a/src/metabase/util/honeysql_extensions.clj b/src/metabase/util/honeysql_extensions.clj
index 810d1602457..ec208e11dd8 100644
--- a/src/metabase/util/honeysql_extensions.clj
+++ b/src/metabase/util/honeysql_extensions.clj
@@ -141,6 +141,7 @@
(def ^{:arglists '([& exprs])} floor "SQL `floor` function." (partial hsql/call :floor))
(def ^{:arglists '([& exprs])} hour "SQL `hour` function." (partial hsql/call :hour))
(def ^{:arglists '([& exprs])} minute "SQL `minute` function." (partial hsql/call :minute))
+(def ^{:arglists '([& exprs])} day "SQL `day` function." (partial hsql/call :day))
(def ^{:arglists '([& exprs])} week "SQL `week` function." (partial hsql/call :week))
(def ^{:arglists '([& exprs])} month "SQL `month` function." (partial hsql/call :month))
(def ^{:arglists '([& exprs])} quarter "SQL `quarter` function."(partial hsql/call :quarter))
--
GitLab