Fix datetime filter issue with Metabase 0.47

Fix datetime filter issue with Metabase 0.47

.github/actions/init-dependencies/action.yml

@@ -48,7 +48,7 @@ runs:
     - name: "Use Node.js"
       uses: actions/setup-node@v3
       with:
-        node-version: 16
+        node-version: 18
     - name: Cache Clojure install
       id: cache-clojure
       uses: actions/cache@v3

Makefile

@@ -48,7 +48,7 @@ endif
 
 front_end:
 	@echo "Building Front End..."
-	cd $(makefile_dir)/metabase/; export WEBPACK_BUNDLE=production && yarn build && yarn build-static-viz
+	cd $(makefile_dir)/metabase/; export WEBPACK_BUNDLE=production && yarn build-release && yarn build-static-viz
 
 driver: update_deps_files
 	@echo "Building Starburst driver..."

app_versions.json

@@ -1,5 +1,5 @@
 {
     "trino": "426",
-    "clojure": "1.11.1.1208",
-    "metabase": "v1.46.4"
+    "clojure": "1.11.1.1262",
+    "metabase": "v1.47.2"
 }

drivers/starburst/src/metabase/driver/implementation/driver_helpers.clj

@@ -37,4 +37,4 @@
                               :datetime-diff                   true
                               :convert-timezone                true
                               :now                             true}]
-  (defmethod driver/supports? [:starburst feature] [_ _] supported?))
+  (defmethod driver/database-supports? [:starburst feature] [_ _ _] supported?))

drivers/starburst/src/metabase/driver/implementation/execute.clj

@@ -21,6 +21,8 @@
             [metabase.driver.sql.parameters.substitution :as sql.params.substitution]
             [metabase.query-processor.timezone :as qp.timezone]
             [metabase.api.common :as api]
+            [metabase.query-processor.store :as qp.store]
+            [metabase.lib.metadata :as lib.metadata]
             [metabase.util.date-2 :as u.date]
             [metabase.util.i18n :refer [trs]])
   (:import com.mchange.v2.c3p0.C3P0ProxyConnection
@@ -160,6 +162,14 @@
     (.clearSessionUser (.unwrap conn TrinoConnection))
     nil))
 
+; Metabase tests require a specific error when an invalid number of parameters are passed
+(defn handle-execution-error
+  [e]
+  (log/fatalf (.getMessage e))
+  (if (clojure.string/includes? (.getMessage e) "Incorrect number of parameters")
+    (throw (Exception. "It looks like we got more parameters than we can handle, remember that parameters cannot be used in comments or as identifiers."))
+    (throw e)))
+
 (defmethod sql-jdbc.execute/prepared-statement :starburst
   [driver ^Connection conn ^String sql params]
   ;; with Starburst driver, result set holdability must be HOLD_CURSORS_OVER_COMMIT
@@ -177,9 +187,11 @@
       (sql-jdbc.execute/set-parameters! driver stmt params)
       (proxy [java.sql.PreparedStatement] []
         (executeQuery []
-          (let [rs (.executeQuery stmt)]
-            (remove-impersonation conn)
-            rs))
+          (try
+            (let [rs (.executeQuery stmt)]
+              (remove-impersonation conn)
+              rs)
+              (catch Throwable e (handle-execution-error e))))
         (setMaxRows [nb] (.setMaxRows stmt nb))
         (close [] (.close stmt)))
       (catch Throwable e
@@ -200,9 +212,11 @@
         (log/debug e (trs "Error setting statement fetch direction to FETCH_FORWARD"))))
     (proxy [java.sql.Statement] []
       (execute [sql]
-        (let [rs (.execute stmt sql)]
-          (remove-impersonation conn)
-          rs))
+        (try
+          (let [rs (.execute stmt sql)]
+            rs)
+            (catch Throwable e (handle-execution-error e))
+            (finally (remove-impersonation conn))))
       (getResultSet [] (.getResultSet stmt))
       (setMaxRows [nb] (.setMaxRows stmt nb))
       (close [] (.close stmt)))))
@@ -220,7 +234,7 @@
   ;; (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 :starburst)
+  (let [report-zone       (qp.timezone/report-timezone-id-if-supported :starburst (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))))

drivers/starburst/src/metabase/driver/implementation/query_processor.clj

@@ -18,6 +18,8 @@
             [metabase.driver.sql.query-processor :as sql.qp]
             [metabase.driver.sql.util :as sql.u]
             [metabase.query-processor.timezone :as qp.timezone]
+            [metabase.query-processor.store :as qp.store]
+            [metabase.lib.metadata :as lib.metadata]
             [metabase.util.date-2 :as u.date]
             [metabase.util.honey-sql-2 :as h2x])
     (:import [java.time OffsetDateTime ZonedDateTime]))
@@ -93,7 +95,7 @@
   "Returns a HoneySQL form to interpret the `expr` (a temporal value) in the current report time zone, via Trino's
   `AT TIME ZONE` operator. See https://trino.io/docs/current/functions/datetime.html#time-zone-conversion"
   [expr]
-  (let [report-zone (qp.timezone/report-timezone-id-if-supported :starburst)
+  (let [report-zone (qp.timezone/report-timezone-id-if-supported :starburst (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)]
@@ -236,7 +238,7 @@
 
 (defmethod sql.qp/unix-timestamp->honeysql [:starburst :seconds]
   [_ _ expr]
-  (let [report-zone (qp.timezone/report-timezone-id-if-supported :starburst)]
+  (let [report-zone (qp.timezone/report-timezone-id-if-supported :starburst (lib.metadata/database (qp.store/metadata-provider)))]
     [:from_unixtime expr (h2x/literal (or report-zone "UTC"))]))
 
 (defn- timestamp-with-time-zone? [expr]

drivers/starburst/test/metabase/driver/starburst_test.clj

@@ -30,6 +30,7 @@
             [metabase.sync :as sync]
             [metabase.test :as mt]
             [metabase.test.fixtures :as fixtures]
+            [toucan2.tools.with-temp :as t2.with-temp]
             [toucan.db :as db]))
 
 (use-fixtures :once (fixtures/initialize :db))
@@ -188,7 +189,7 @@
                                      (format "DROP SCHEMA IF EXISTS %s" s)
                                      (format "CREATE SCHEMA %s" s)
                                      (format "CREATE TABLE %s.%s (pk INTEGER, val1 VARCHAR(512))" s t)])
-                      (mt/with-temp Database [db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details with-schema}]
+                      (t2.with-temp/with-temp [Database db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details with-schema}]
                         (mt/with-db db
             ;; same as test_data, but with schema, so should NOT pick up venues, users, etc.
                           (sync/sync-database! db)
@@ -307,7 +308,7 @@
             :roles                        "sysadmin"
             :ssl                          false
             :impersonation                true}]
-              (mt/with-temp Database [db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details details}]
+              (t2.with-temp/with-temp [Database db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details details}]
               (mt/with-db db
               (qp/process-query
                 {:database     (mt/id)
@@ -325,7 +326,7 @@
               :roles                        "sysadmin"
               :ssl                          false
               :impersonation                true}]
-                (mt/with-temp Database [db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details details}]
+                (t2.with-temp/with-temp [Database db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details details}]
                 (mt/with-db db
                 (qp/process-query
                   {:database     (mt/id)
@@ -343,7 +344,7 @@
               :roles                        "sysadmin"
               :ssl                          false
               :impersonation                false}]
-                (mt/with-temp Database [db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details details}]
+                (t2.with-temp/with-temp [Database db {:engine :starburst, :name "Temp Trino JDBC Schema DB", :details details}]
                 (mt/with-db db
                 (qp/process-query
                   {:database     (mt/id)

drivers/starburst/test/metabase/test/data/starburst.clj

@@ -37,7 +37,8 @@
 (defmethod tx/sorts-nil-first? :starburst [_ _] false)
 
 ;; during unit tests don't treat Trino as having FK support
-(defmethod driver/supports? [:starburst :foreign-keys] [_ _] (not config/is-test?))
+(defmethod driver/database-supports? [:starburst :foreign-keys] [_ _ _] (not config/is-test?))
+(defmethod driver/database-supports? [:starburst :set-timezone] [_ _ _] true)
 
 (doseq [[base-type db-type] {:type/BigInteger             "BIGINT"
                              :type/Boolean                "BOOLEAN"
@@ -104,23 +105,28 @@
   combined with the fact that the Trino driver apparently closes the connection when it closes a prepare statement.
   Therefore, create a fresh connection from the DriverManager."
   ^Connection [jdbc-spec]
-  (DriverManager/getConnection (format "jdbc:%s:%s" (:subprotocol jdbc-spec) (:subname jdbc-spec))
-                               (connection-pool/map->properties (select-keys jdbc-spec [:user :SSL]))))
+  (let [conn (:connection jdbc-spec)]
+    (locking conn
+      (if (not (.getAutoCommit conn))
+        (.setAutoCommit conn true)
+        true))
+    conn))
 
 (defmethod load-data/do-insert! :starburst
   [driver spec table-identifier row-or-rows]
   (let [statements (ddl/insert-rows-ddl-statements driver table-identifier row-or-rows)]
-    (with-open [conn (jdbc-spec->connection spec)]
-      (doseq [[^String sql & params] statements]
-        (try
-          (with-open [^PreparedStatement stmt (.prepareStatement conn sql)]
-            (sql-jdbc.execute/set-parameters! driver stmt params)
-            (let [rows-affected (.executeUpdate stmt)]
-              (log/infof "[%s] Inserted %d rows into starburst table %s" driver rows-affected table-identifier)))
-          (catch Throwable e
-            (throw (ex-info (format "[%s] Error executing SQL: %s" driver (ex-message e))
-                            {:driver driver, :sql sql, :params params}
-                            e))))))))
+    (let [conn (jdbc-spec->connection spec)]
+      (locking conn
+        (doseq [[^String sql & params] statements]
+          (try
+            (with-open [^PreparedStatement stmt (.prepareStatement conn sql)]
+              (sql-jdbc.execute/set-parameters! driver stmt params)
+              (let [rows-affected (.executeUpdate stmt)]
+                (log/infof "[%s] Inserted %d rows into starburst table %s" driver rows-affected table-identifier)))
+            (catch Throwable e
+              (throw (ex-info (format "[%s] Error executing SQL: %s" driver (ex-message e))
+                              {:driver driver, :sql sql, :params params}
+                              e)))))))))
 
 (defmethod sql.tx/drop-db-if-exists-sql :starburst [_ _] nil)
 (defmethod sql.tx/create-db-sql         :starburst [_ _] nil)
@@ -143,7 +149,9 @@
     (str/replace sql #", PRIMARY KEY \([^)]+\)|NOT NULL" "")))
 
 (defmethod ddl.i/format-name :starburst [_ table-or-field-name]
-  (u/snake-key table-or-field-name))
+  (if config/is-test?
+    table-or-field-name
+    (u/->snake_case_en table-or-field-name)))
 
 ;; Trino doesn't support FKs, at least not adding them via DDL
 (defmethod sql.tx/add-fk-sql :starburst