Merge pull request #9398 from metabase/fix-sqlserver-date-grouping-other-languages

Fix SQLServer datetime grouping for locales with yyyy-dd-MM dates

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))

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

 (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))))))

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]"))))))

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))