HOLY GRAIL OF PRs UNBLOCK PARALLEL-TEST-SAFE SETTINGS!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!!

modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj

@@ -317,8 +317,8 @@
 (defn- in-report-zone
   "Returns a HoneySQL form to interpret the `expr` (a temporal value) in the current report time zone, via Presto's
   `AT TIME ZONE` operator. See https://prestodb.io/docs/current/functions/datetime.html"
-  [expr]
-  (let [report-zone (qp.timezone/report-timezone-id-if-supported :presto-jdbc (lib.metadata/database (qp.store/metadata-provider)))
+  [driver expr]
+  (let [report-zone (qp.timezone/report-timezone-id-if-supported driver (lib.metadata/database (qp.store/metadata-provider)))
         ;; if the expression itself has type info, use that, or else use a parent expression's type info if defined
         type-info   (h2x/type-info expr)
         db-type     (h2x/type-info->db-type type-info)]
@@ -339,76 +339,76 @@
   expr)
 
 (defmethod sql.qp/date [:presto-jdbc :minute]
-  [_driver _unit expr]
-  [:date_trunc (h2x/literal :minute) (in-report-zone expr)])
+  [driver _unit expr]
+  [:date_trunc (h2x/literal :minute) (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :minute-of-hour]
-  [_driver _unit expr]
-  [:minute (in-report-zone expr)])
+  [driver _unit expr]
+  [:minute (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :hour]
-  [_driver _unit expr]
-  [:date_trunc (h2x/literal :hour) (in-report-zone expr)])
+  [driver _unit expr]
+  [:date_trunc (h2x/literal :hour) (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :hour-of-day]
-  [_driver _unit expr]
-  [:hour (in-report-zone expr)])
+  [driver _unit expr]
+  [:hour (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :day]
-  [_driver _unit expr]
-  [:date (in-report-zone expr)])
+  [driver _unit expr]
+  [:date (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :day-of-week]
-  [_driver _unit expr]
-  (sql.qp/adjust-day-of-week :presto-jdbc [:day_of_week (in-report-zone expr)]))
+  [driver _unit expr]
+  (sql.qp/adjust-day-of-week :presto-jdbc [:day_of_week (in-report-zone driver expr)]))
 
 (defmethod sql.qp/date [:presto-jdbc :day-of-month]
-  [_driver _unit expr]
-  [:day (in-report-zone expr)])
+  [driver _unit expr]
+  [:day (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :day-of-year]
-  [_driver _unit expr]
-  [:day_of_year (in-report-zone expr)])
+  [driver _unit expr]
+  [:day_of_year (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :week]
-  [_driver _unit expr]
+  [driver _unit expr]
   (letfn [(truncate [x]
             [:date_trunc (h2x/literal :week) x])]
-    (sql.qp/adjust-start-of-week :presto-jdbc truncate (in-report-zone expr))))
+    (sql.qp/adjust-start-of-week :presto-jdbc truncate (in-report-zone driver expr))))
 
 (defmethod sql.qp/date [:presto-jdbc :month]
-  [_driver _unit expr]
-  [:date_trunc (h2x/literal :month) (in-report-zone expr)])
+  [driver _unit expr]
+  [:date_trunc (h2x/literal :month) (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :month-of-year]
-  [_driver _unit expr]
-  [:month (in-report-zone expr)])
+  [driver _unit expr]
+  [:month (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :quarter]
-  [_driver _unit expr]
-  [:date_trunc (h2x/literal :quarter) (in-report-zone expr)])
+  [driver _unit expr]
+  [:date_trunc (h2x/literal :quarter) (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :quarter-of-year]
-  [_driver _unit expr]
-  [:quarter (in-report-zone expr)])
+  [driver _unit expr]
+  [:quarter (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :year]
-  [_driver _unit expr]
-  [:date_trunc (h2x/literal :year) (in-report-zone expr)])
+  [driver _unit expr]
+  [:date_trunc (h2x/literal :year) (in-report-zone driver expr)])
 
 (defmethod sql.qp/date [:presto-jdbc :year-of-era]
-  [_driver _unit expr]
-  [:year (in-report-zone expr)])
+  [driver _unit expr]
+  [:year (in-report-zone driver expr)])
 
 (defmethod sql.qp/unix-timestamp->honeysql [:presto-jdbc :seconds]
-  [_driver _unit expr]
-  (let [report-zone (qp.timezone/report-timezone-id-if-supported :presto-jdbc (lib.metadata/database (qp.store/metadata-provider)))]
+  [driver _unit expr]
+  (let [report-zone (qp.timezone/report-timezone-id-if-supported driver (lib.metadata/database (qp.store/metadata-provider)))]
     [:from_unixtime expr (h2x/literal (or report-zone "UTC"))]))
 
 (defmethod sql.qp/unix-timestamp->honeysql [:presto-jdbc :milliseconds]
-  [_driver _unit expr]
+  [driver _unit expr]
   ;; from_unixtime doesn't support milliseconds directly, but we can add them back in
-  (let [report-zone (qp.timezone/report-timezone-id-if-supported :presto-jdbc (lib.metadata/database (qp.store/metadata-provider)))
+  (let [report-zone (qp.timezone/report-timezone-id-if-supported driver (lib.metadata/database (qp.store/metadata-provider)))
         millis      [::mod expr [:inline 1000]]
         expr        [:from_unixtime [:/ expr [:inline 1000]] (h2x/literal (or report-zone "UTC"))]]
     (date-add :millisecond millis expr)))
@@ -717,12 +717,12 @@
   (sql.params.substitution/make-stmt-subs "from_iso8601_timestamp(?)" [ts-str]))
 
 (defmethod sql.params.substitution/->prepared-substitution [:presto-jdbc ZonedDateTime]
-  [_ ^ZonedDateTime t]
+  [driver ^ZonedDateTime t]
   ;; for native query parameter substitution, in order to not conflict with the `PrestoConnection` session time zone
   ;; (which was set via report time zone), it is necessary to use the `from_iso8601_timestamp` function on the string
   ;; representation of the `ZonedDateTime` instance, but converted to the report time zone
   #_(date-time->substitution (.format (t/offset-date-time (t/local-date-time t) (t/zone-offset 0)) DateTimeFormatter/ISO_OFFSET_DATE_TIME))
-  (let [report-zone       (qp.timezone/report-timezone-id-if-supported :presto-jdbc (lib.metadata/database (qp.store/metadata-provider)))
+  (let [report-zone       (qp.timezone/report-timezone-id-if-supported driver (lib.metadata/database (qp.store/metadata-provider)))
         ^ZonedDateTime ts (if (str/blank? report-zone) t (t/with-zone-same-instant t (t/zone-id report-zone)))]
     ;; the `from_iso8601_timestamp` only accepts timestamps with an offset (not a zone ID), so only format with offset
     (date-time->substitution (.format ts DateTimeFormatter/ISO_OFFSET_DATE_TIME))))

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

@@ -458,10 +458,6 @@
        " */ "
        (qp.util/default-query->remark query)))
 
-(defmethod sql-jdbc.execute/set-parameter [:redshift java.time.ZonedDateTime]
-  [driver ps i t]
-  (sql-jdbc.execute/set-parameter driver ps i (t/sql-timestamp (t/with-zone-same-instant t (t/zone-id "UTC")))))
-
 (defmethod driver/upload-type->database-type :redshift
   [_driver upload-type]
   (case upload-type

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

@@ -53,7 +53,8 @@
                               :connection-impersonation-requires-role true
                               :convert-timezone                       true
                               :datetime-diff                          true
-                              :now                                    true}]
+                              :now                                    true
+                              :set-timezone                           true}]
   (defmethod driver/database-supports? [:snowflake feature] [_driver _feature _db] supported?))
 
 (defmethod driver/humanize-connection-error-message :snowflake
@@ -149,10 +150,7 @@
                 ;; other SESSION parameters
                 ;; not 100% sure why we need to do this but if we don't set the connection to UTC our report timezone
                 ;; stuff doesn't work, even though we ultimately override this when we set the session timezone
-                :timezone                                   "UTC"
-                ;; tell Snowflake to use the same start of week that we have set for the
-                ;; [[metabase.public-settings/start-of-week]] Setting.
-                :week_start                                 (start-of-week-setting->snowflake-offset)}
+                :timezone                                   "UTC"}
                (-> details
                    ;; original version of the Snowflake driver incorrectly used `dbname` in the details fields instead
                    ;; of `db`. If we run across `dbname`, correct our behavior
@@ -613,13 +611,27 @@
 
 (defmethod sql.qp/current-datetime-honeysql-form :snowflake [_] :%current_timestamp)
 
-(defmethod sql-jdbc.execute/set-timezone-sql :snowflake [_] "ALTER SESSION SET TIMEZONE = %s;")
+(defmethod sql-jdbc.execute/do-with-connection-with-options :snowflake
+  [driver db-or-id-or-spec {:keys [session-timezone], :as options} f]
+  (let [parent-method (get-method sql-jdbc.execute/do-with-connection-with-options :sql-jdbc)]
+    (parent-method
+     driver
+     db-or-id-or-spec
+     options
+     (fn [^Connection conn]
+       (let [session-timezone (or session-timezone "UTC")]
+         (with-open [stmt (.createStatement conn)]
+           (.addBatch stmt (format "ALTER SESSION SET TIMEZONE = '%s';" session-timezone))
+           (.addBatch stmt (format "ALTER SESSION SET WEEK_START = %d;" (start-of-week-setting->snowflake-offset)))
+           (.executeBatch stmt)))
+       (f conn)))))
 
 (defmethod driver/db-default-timezone :snowflake
   [driver database]
   (sql-jdbc.execute/do-with-connection-with-options
    driver database nil
    (fn [^java.sql.Connection conn]
+     ;; is this actually the SYSTEM default timezone? Not SESSION? I thought Snowflake's default was US/Pacific.
      (with-open [stmt (.prepareStatement conn "show parameters like 'TIMEZONE' in user;")
                  rset (.executeQuery stmt)]
        (when (.next rset)

modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj

@@ -577,10 +577,10 @@
 (mt/defdataset dst-change
   [["dst_tz_test" [{:field-name "dtz"
                     :base-type :type/DateTimeWithTZ}]
-    [["2023-09-30 23:59:59 +1000"]      ; September
-     ["2023-10-01 00:00:00 +1000"]      ; October before DST starts
-     ["2023-10-01 05:00:00 +1100"]      ; October after DST starts
-     ["2023-11-01 05:00:00 +1100"]]]])  ; November
+    [["2023-09-30 23:59:59 +1000"]     ; September
+     ["2023-10-01 00:00:00 +1000"]     ; October before DST starts
+     ["2023-10-01 05:00:00 +1100"]     ; October after DST starts
+     ["2023-11-01 05:00:00 +1100"]]]]) ; November
 
 (deftest date-bucketing-test
   (testing "#37065"
@@ -593,8 +593,8 @@
                              (lib/breakout dtz)
                              (lib/aggregate (lib/count)))]
           (mt/with-native-query-testing-context query
-            (is (= [["2023-09-30T00:00:00+10:00" 1]
-                    ["2023-10-01T00:00:00+10:00" 2]
-                    ["2023-11-01T00:00:00+11:00" 1]]
-                   (mt/with-temporary-setting-values [report-timezone "Australia/Sydney"]
+            (mt/with-temporary-setting-values [report-timezone "Australia/Sydney"]
+              (is (= [["2023-09-30T00:00:00+10:00" 1]
+                      ["2023-10-01T00:00:00+10:00" 2]
+                      ["2023-11-01T00:00:00+11:00" 1]]
                      (mt/rows (qp/process-query query)))))))))))

src/metabase/driver/sql_jdbc.clj

@@ -78,8 +78,8 @@
   (sql-jdbc.execute/execute-reducible-query driver query context respond))
 
 (defmethod driver/notify-database-updated :sql-jdbc
-  [_ database]
-  (sql-jdbc.conn/invalidate-pool-for-db! database))
+  [_driver database]
+  (sql-jdbc.conn/invalidate-pool-for-db-if-needed! database))
 
 (defmethod driver/dbms-version :sql-jdbc
   [driver database]

src/metabase/driver/sql_jdbc/connection.clj

@@ -210,11 +210,23 @@
   (swap! database-id->jdbc-spec-hash assoc database-id (jdbc-spec-hash database))
   nil)
 
-(defn invalidate-pool-for-db!
+(defn- invalidate-pool-for-db!
   "Invalidates the connection pool for the given database by closing it and removing it from the cache."
   [database]
   (set-pool! (u/the-id database) nil nil))
 
+(defn invalidate-pool-for-db-if-needed!
+  "Invalidate the connection pool for the given database if the connection details have changed."
+  [database]
+  (when-let [old-hash (get @database-id->jdbc-spec-hash (u/the-id database))]
+    (let [new-hash (jdbc-spec-hash database)]
+      (log/tracef "Maybe invalidating connection pool for %s... old hash: %s, new hash: %s"
+                  (pr-str (select-keys database [:id :name :engine]))
+                  (pr-str old-hash)
+                  (pr-str new-hash))
+      (when-not (= old-hash new-hash)
+        (invalidate-pool-for-db! database)))))
+
 (defn- log-ssh-tunnel-reconnect-msg! [db-id]
   (log/warn (u/format-color :red "ssh tunnel for database %s looks closed; marking pool invalid to reopen it" db-id))
   nil)

src/metabase/driver/sql_jdbc/execute.clj

@@ -181,7 +181,7 @@
     ;; default method for Postgres not covered by any [driver jdbc-type] methods
     (defmethod read-column-thunk :postgres
       ...)"
-  {:added "0.35.0", :arglists '([driver ^java.sql.ResultSet rs ^java.sql.ResultSetMetaData rsmeta i])}
+  {:added "0.35.0", :arglists '([driver ^java.sql.ResultSet rs ^java.sql.ResultSetMetaData rsmeta ^Long i])}
   (fn [driver _rs ^ResultSetMetaData rsmeta ^Long col-idx]
     [(driver/dispatch-on-initialized-driver driver) (.getColumnType rsmeta col-idx)])
   :hierarchy #'driver/hierarchy)
@@ -571,14 +571,14 @@
       (execute-prepared-statement! driver st))))
 
 (defmethod read-column-thunk :default
-  [driver ^ResultSet rs rsmeta ^long i]
+  [driver ^ResultSet rs rsmeta ^Long i]
   (let [driver-default-method (get-method read-column-thunk driver)]
     (if-not (= driver-default-method (get-method read-column-thunk :default))
       ^{:name (format "(read-column-thunk %s)" driver)} (driver-default-method driver rs rsmeta i)
       ^{:name (format "(.getObject rs %d)" i)} (fn []
                                                  (.getObject rs i)))))
 
-(defn- get-object-of-class-thunk [^ResultSet rs, ^long i, ^Class klass]
+(defn- get-object-of-class-thunk [^ResultSet rs ^Long i ^Class klass]
   ^{:name (format "(.getObject rs %d %s)" i (.getCanonicalName klass))}
   (fn []
     (.getObject rs i klass)))

test/metabase/driver/sql_jdbc/connection_test.clj

@@ -135,7 +135,7 @@
                                        (swap! hash-change-called-times inc)
                                        nil)]
         (try
-          (sql-jdbc.conn/invalidate-pool-for-db! db)
+          (#'sql-jdbc.conn/invalidate-pool-for-db! db)
           ;; a little bit hacky to redefine the log fn, but it's the most direct way to test
           (with-redefs [sql-jdbc.conn/log-jdbc-spec-hash-change-msg! hash-change-fn]
             (let [pool-spec-1 (sql-jdbc.conn/db->pooled-connection-spec db)