Fixes for Oracle [ci drivers]

src/metabase/driver/oracle.clj

@@ -156,7 +156,8 @@
 (defn- apply-limit [honeysql-query {value :limit}]
   {:pre [(integer? value)]}
   {:select [:*]
-   :from   [honeysql-query]
+   :from   [(merge {:select [:*]}   ; if `honeysql-query` doesn't have a `SELECT` clause yet (which might be the case when using a source query)
+                   honeysql-query)] ; fall back to including a `SELECT *` just to make sure a valid query is produced
    :where  [:<= (hsql/raw "rownum") value]})
 
 (defn- apply-page [honeysql-query {{:keys [items page]} :page}]
@@ -167,7 +168,9 @@
       ;; if we need to do an offset we have to do double-nesting
       {:select [:*]
        :from   [{:select [:__table__.* [(hsql/raw "rownum") :__rownum__]]
-                 :from   [[honeysql-query :__table__]]
+                 :from   [[(merge {:select [:*]}
+                                  honeysql-query)
+                           :__table__]]
                  :where  [:<= (hsql/raw "rownum") (+ offset items)]}]
        :where  [:> :__rownum__ offset]})))
 

src/metabase/util.clj

@@ -533,6 +533,7 @@
    function from `colorize.core`.
 
      (pprint-to-str 'green some-obj)"
+  {:style/indent 1}
   (^String [x]
    (when x
      (with-out-str (pprint x))))

test/metabase/query_processor_test/nested_queries_test.clj

@@ -7,6 +7,7 @@
              [query-processor :as qp]
              [query-processor-test :refer :all]
              [util :as u]]
+            [metabase.driver.generic-sql :as generic-sql]
             [metabase.models
              [card :refer [Card]]
              [database :as database]
@@ -37,30 +38,40 @@
           [5 "Brite Spot Family Restaurant" 20 34.0778 -118.261 2]]
    :cols [{:name "id",          :base_type (data/id-field-type)}
           {:name "name",        :base_type :type/Text}
-          {:name "category_id", :base_type :type/Integer}
+          {:name "category_id", :base_type (data/expected-base-type->actual :type/Integer)}
           {:name "latitude",    :base_type :type/Float}
           {:name "longitude",   :base_type :type/Float}
-          {:name "price",       :base_type :type/Integer}]}
-  (rows+cols
-    (qp/process-query
-      {:database (data/id)
-       :type     :query
-       :query    {:source-query {:source-table (data/id :venues)
-                                 :order-by     [:asc (data/id :venues :id)]
-                                 :limit        10}
-                  :limit        5}})))
+          {:name "price",       :base_type (data/expected-base-type->actual :type/Integer)}]}
+  (format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]
+    (rows+cols
+      (qp/process-query
+        {:database (data/id)
+         :type     :query
+         :query    {:source-query {:source-table (data/id :venues)
+                                   :order-by     [:asc (data/id :venues :id)]
+                                   :limit        10}
+                    :limit        5}}))))
 
+;; TODO - `identifier`, `quoted-identifier` might belong in some sort of shared util namespace
 (defn- identifier
-  "Return a properly formatted identifier for a Table or Field.
+  "Return a properly formatted *UNQUOTED* identifier for a Table or Field.
   (This handles DBs like H2 who require uppercase identifiers, or databases like Redshift do clever hacks
    like prefixing table names with a unique schema for each test run because we're not
    allowed to create new databases.)"
-  ([table-kw]
+  (^String [table-kw]
    (let [{schema :schema, table-name :name} (db/select-one [Table :name :schema] :id (data/id table-kw))]
      (name (hsql/qualify schema table-name))))
-  ([table-kw field-kw]
+  (^String [table-kw field-kw]
    (db/select-one-field :name Field :id (data/id table-kw field-kw))))
 
+(defn- quote-identifier [identifier]
+  (first (hsql/format (keyword identifier)
+           :quoting (generic-sql/quote-style datasets/*driver*))))
+
+(def ^:private ^{:arglists '([table-kw] [table-kw field-kw])} ^String quoted-identifier
+  "Return a *QUOTED* identifier for a Table or Field. (This behaves just like `identifier`, but quotes the result)."
+  (comp quote-identifier identifier))
+
 ;; make sure we can do a basic query with a SQL source-query
 (datasets/expect-with-engines (engines-that-support :nested-queries)
   {:rows [[1 -165.374  4 3 "Red Medicine"                 10.0646]
@@ -70,24 +81,26 @@
           [5 -118.261 20 2 "Brite Spot Family Restaurant" 34.0778]]
    :cols [{:name "id",          :base_type :type/Integer}
           {:name "longitude",   :base_type :type/Float}
-          {:name "category_id", :base_type :type/Integer}
-          {:name "price",       :base_type :type/Integer}
+          {:name "category_id", :base_type (data/expected-base-type->actual :type/Integer)}
+          {:name "price",       :base_type (data/expected-base-type->actual :type/Integer)}
           {:name "name",        :base_type :type/Text}
           {:name "latitude",    :base_type :type/Float}]}
-  (rows+cols
-    (qp/process-query
-      {:database (data/id)
-       :type     :query
-       :query    {:source-query {:native (format "SELECT %s, %s, %s, %s, %s, %s FROM %s"
-                                                 (identifier :venues :id)
-                                                 (identifier :venues :longitude)
-                                                 (identifier :venues :category_id)
-                                                 (identifier :venues :price)
-                                                 (identifier :venues :name)
-                                                 (identifier :venues :latitude)
-                                                 (identifier :venues))}
-                  :order-by     [:asc [:field-literal (keyword (data/format-name :id)) :type/Integer]]
-                  :limit        5}})))
+  (format-rows-by [int (partial u/round-to-decimals 4) int int str (partial u/round-to-decimals 4)]
+    (rows+cols
+      (qp/process-query
+        {:database (data/id)
+         :type     :query
+         :query    {:source-query {:native (format "SELECT %s, %s, %s, %s, %s, %s FROM %s"
+                                                   (quoted-identifier :venues :id)
+                                                   (quoted-identifier :venues :longitude)
+                                                   (quoted-identifier :venues :category_id)
+                                                   (quoted-identifier :venues :price)
+                                                   (quoted-identifier :venues :name)
+                                                   (quoted-identifier :venues :latitude)
+                                                   (quoted-identifier :venues))}
+                    :order-by     [:asc [:field-literal (keyword (data/format-name :id)) :type/Integer]]
+                    :limit        5}}))))
+
 
 (def ^:private ^:const breakout-results
   {:rows [[1 22]
@@ -117,7 +130,7 @@
       (qp/process-query
         {:database (data/id)
          :type     :query
-         :query    {:source-query {:native (format "SELECT * FROM %s" (identifier :venues))}
+         :query    {:source-query {:native (format "SELECT * FROM %s" (quoted-identifier :venues))}
                     :aggregation  [:count]
                     :breakout     [[:field-literal (keyword (data/format-name :price)) :type/Integer]]}}))))
 

test/metabase/test/data.clj

@@ -170,7 +170,10 @@
 (defn default-schema [] (i/default-schema *driver*))
 (defn id-field-type  [] (i/id-field-type *driver*))
 
-(defn expected-base-type->actual [base-type]
+(defn expected-base-type->actual
+  "Return actual `base_type` that will be used for the given driver if we asked for BASE-TYPE.
+   Mainly for Oracle because it doesn't have `INTEGER` types and uses decimals instead."
+  [base-type]
   (i/expected-base-type->actual *driver* base-type))
 
 

test/metabase/test/data/generic_sql.clj

@@ -224,18 +224,19 @@
    the calls the resulting function with the rows to insert."
   [& wrap-insert-fns]
   (fn [driver {:keys [database-name], :as dbdef} {:keys [table-name], :as tabledef}]
-    (let [spec       (database->spec driver :db dbdef)
-          table-name (apply hx/qualify-and-escape-dots (qualified-name-components driver database-name table-name))
-          insert!    ((apply comp wrap-insert-fns) (partial do-insert! driver spec table-name))
-          rows       (load-data-get-rows driver dbdef tabledef)]
-      (insert! rows))))
+    (jdbc/with-db-connection [conn (database->spec driver :db dbdef)]
+      (.setAutoCommit (jdbc/get-connection conn) false)
+      (let [table-name (apply hx/qualify-and-escape-dots (qualified-name-components driver database-name table-name))
+            insert!    ((apply comp wrap-insert-fns) (partial do-insert! driver conn table-name))
+            rows       (load-data-get-rows driver dbdef tabledef)]
+        (insert! rows)))))
 
 (def load-data-all-at-once!            "Insert all rows at once."                             (make-load-data-fn))
 (def load-data-chunked!                "Insert rows in chunks of 200 at a time."              (make-load-data-fn load-data-chunked))
 (def load-data-one-at-a-time!          "Insert rows one at a time."                           (make-load-data-fn load-data-one-at-a-time))
 (def load-data-chunked-parallel!       "Insert rows in chunks of 200 at a time, in parallel." (make-load-data-fn load-data-add-ids (partial load-data-chunked pmap)))
 (def load-data-one-at-a-time-parallel! "Insert rows one at a time, in parallel."              (make-load-data-fn load-data-add-ids (partial load-data-one-at-a-time pmap)))
-
+;; ^ the parallel versions aren't neccesarily faster than the sequential versions for all drivers so make sure to do some profiling in order to pick the appropriate implementation
 
 (defn default-execute-sql! [driver context dbdef sql]
   (let [sql (some-> sql s/trim)]
@@ -293,27 +294,23 @@
   ;; Exec SQL for creating the DB
   (execute-sql! driver :server dbdef (str (drop-db-if-exists-sql driver dbdef) ";\n"
                                           (create-db-sql driver dbdef)))
-
   ;; Build combined statement for creating tables + FKs
   (let [statements (atom [])]
-
     ;; Add the SQL for creating each Table
     (doseq [tabledef table-definitions]
       (swap! statements conj (drop-table-if-exists-sql driver dbdef tabledef)
              (create-table-sql driver dbdef tabledef)))
-
     ;; Add the SQL for adding FK constraints
     (doseq [{:keys [field-definitions], :as tabledef} table-definitions]
       (doseq [{:keys [fk], :as fielddef} field-definitions]
         (when fk
           (swap! statements conj (add-fk-sql driver dbdef tabledef fielddef)))))
-
     ;; exec the combined statement
     (execute-sql! driver :db dbdef (s/join ";\n" (map hx/unescape-dots @statements))))
-
   ;; Now load the data for each Table
   (doseq [tabledef table-definitions]
-    (load-data! driver dbdef tabledef)))
+    (u/profile (format "load-data for %s %s %s" (name driver) (:database-name dbdef) (:table-name tabledef))
+      (load-data! driver dbdef tabledef))))
 
 (def IDriverTestExtensionsMixin
   "Mixin for `IGenericSQLTestExtensions` types to implement `create-db!` from `IDriverTestExtensions`."
@@ -323,26 +320,28 @@
 
 ;;; ## Various Util Fns
 
+(defn- do-when-testing-engine {:style/indent 1} [engine f]
+  (require 'metabase.test.data.datasets)
+  ((resolve 'metabase.test.data.datasets/do-when-testing-engine) engine f))
+
 (defn execute-when-testing!
   "Execute a prepared SQL-AND-ARGS against Database with spec returned by GET-CONNECTION-SPEC only when running tests against ENGINE.
    Useful for doing engine-specific setup or teardown."
   {:style/indent 2}
   [engine get-connection-spec & sql-and-args]
-  ((resolve 'metabase.test.data.datasets/do-when-testing-engine)
-   engine
-   (fn []
-     (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
-     (jdbc/execute! (get-connection-spec) sql-and-args)
-     (println (u/format-color 'blue "[OK]")))))
+  (do-when-testing-engine engine
+    (fn []
+      (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
+      (jdbc/execute! (get-connection-spec) sql-and-args)
+      (println (u/format-color 'blue "[OK]")))))
 
 (defn query-when-testing!
   "Execute a prepared SQL-AND-ARGS **query** against Database with spec returned by GET-CONNECTION-SPEC only when running tests against ENGINE.
    Useful for doing engine-specific setup or teardown where `execute-when-testing!` won't work because the query returns results."
   {:style/indent 2}
   [engine get-connection-spec & sql-and-args]
-  ((resolve 'metabase.test.data.datasets/do-when-testing-engine)
-   engine
-   (fn []
-     (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
-     (u/prog1 (jdbc/query (get-connection-spec) sql-and-args)
-       (println (u/format-color 'blue "[OK] -> %s" (vec <>)))))))
+  (do-when-testing-engine engine
+    (fn []
+      (println (u/format-color 'blue "[%s] %s" (name engine) (first sql-and-args)))
+      (u/prog1 (jdbc/query (get-connection-spec) sql-and-args)
+        (println (u/format-color 'blue "[OK] -> %s" (vec <>)))))))

test/metabase/test/data/interface.clj

@@ -113,7 +113,7 @@
     "*OPTIONAL*. Return the base type type that is actually used to store `Fields` of BASE-TYPE.
      The default implementation of this method is an identity fn. This is provided so DBs that don't support a given BASE-TYPE used in the test data
      can specifiy what type we should expect in the results instead.
-     For example, Oracle has `INTEGER` data types, so `:type/Integer` test values are instead stored as `NUMBER`, which we map to `:type/Decimal`.")
+     For example, Oracle has no `INTEGER` data types, so `:type/Integer` test values are instead stored as `NUMBER`, which we map to `:type/Decimal`.")
 
   (format-name ^String [this, ^String table-or-field-name]
     "*OPTIONAL* Transform a lowercase string `Table` or `Field` name in a way appropriate for this dataset

test/metabase/test/data/oracle.clj

@@ -2,7 +2,9 @@
   (:require [clojure.java.jdbc :as jdbc]
             [clojure.string :as s]
             [environ.core :refer [env]]
-            [metabase.driver.generic-sql :as sql]
+            [metabase.driver
+             [generic-sql :as sql]
+             oracle]
             [metabase.test.data
              [generic-sql :as generic]
              [interface :as i]]
@@ -79,7 +81,7 @@
           :drop-table-if-exists-sql  (u/drop-first-arg drop-table-if-exists-sql)
           :execute-sql!              generic/sequentially-execute-sql!
           :field-base-type->sql-type (u/drop-first-arg field-base-type->sql-type)
-          :load-data!                generic/load-data-one-at-a-time-parallel!
+          :load-data!                generic/load-data-one-at-a-time! ; Now that connections are reüsed doing this sequentially actually seems to be faster than parallel
           :pk-sql-type               (constantly "INTEGER GENERATED BY DEFAULT AS IDENTITY (START WITH 1 INCREMENT BY 1) NOT NULL") ; LOL
           :qualified-name-components (partial i/single-db-qualified-name-components session-schema)})
 
@@ -108,6 +110,12 @@
 (def ^:private execute-when-testing-oracle!
   (partial generic/execute-when-testing! :oracle dbspec))
 
+(defn- clean-session-schemas! []
+  "Delete any old session users that for some reason or another were never deleted. For REPL usage."
+  (doseq [schema (non-session-schemas)
+          :when  (re-find #"^CAM_" schema)]
+    (execute-when-testing-oracle! (format "DROP USER %s CASCADE" schema))))
+
 (defn- create-session-user!
   {:expectations-options :before-run}
   []