Merge pull request #265 from metabase/native_query_processor_maintains_result_order

Native QP Improvements

src/metabase/api/qs.clj

@@ -10,8 +10,6 @@
             [metabase.util :refer [contains-many? now-iso8601]]))
 
 
-(declare execute-query)
-
 (defendpoint POST "/" [:as {{:keys [timezone database sql] :as body} :body}]
   {database [Required Integer]
    sql      [Required NonEmptyString]} ; TODO - check timezone

src/metabase/api/query.clj

@@ -108,7 +108,7 @@
   (let-404 [{database-id :database_id
              {:keys [sql timezone]} :details :as query} (sel :one Query :id id)]
     (read-check query)
-    (let [dataset-query {:type "native"
+    (let [dataset-query {:type :native
                          :database database-id
                          :native {:query sql
                                   :timezone timezone}}

src/metabase/driver.clj

@@ -69,19 +69,20 @@
 (defn dataset-query
   "Process and run a json based dataset query and return results.
 
-   Takes 2 arguments:
-   1.  the json query as a dictionary
-   2.  query execution options specified in a dictionary
-
-   Depending on the database specified in the query this function will delegate to a driver specific implementation.
-   For the purposes of tracking we record each call to this function as a QueryExecution in the database.
-
-   Possible caller-options include:
-     :executed_by [int]               (user_id of caller)
-     :saved_query [{}]                (dictionary representing Query model)
-     :synchronously [true|false]      (default true)
-     :cache_result [true|false]       (default false)
-  "
+  Takes 2 arguments:
+
+  1.  the json query as a dictionary
+  2.  query execution options specified in a dictionary
+
+  Depending on the database specified in the query this function will delegate to a driver specific implementation.
+  For the purposes of tracking we record each call to this function as a QueryExecution in the database.
+
+  Possible caller-options include:
+
+    :executed_by [int]               (user_id of caller)
+    :saved_query [{}]                (dictionary representing Query model)
+    :synchronously [true|false]      (default true)
+    :cache_result [true|false]       (default false)"
   {:arglists '([query caller-options])}
   [query {:keys [executed_by synchronously saved_query]
           :or {synchronously true}
@@ -115,16 +116,17 @@
         ;; this ensures the currently saved query-execution is what gets returned
         query-execution))))
 
-
 (defn -dataset-query
   "Execute a query and record the outcome.  Entire execution is wrapped in a try-catch to prevent Exceptions
-   from leaking outside the function call."
+  from leaking outside the function call."
   [query options query-execution]
   (let [query-execution (assoc query-execution :start_time_millis (System/currentTimeMillis))]
     (try
       (let [query-result (execute-query query)]
-        (when-not (contains? query-result :status) (throw (Exception. "invalid response from database driver. no :status provided")))
-        (when (= :failed (:status query-result)) (throw (Exception. ^String (get query-result :error "general error"))))
+        (when-not (contains? query-result :status)
+          (throw (Exception. "invalid response from database driver. no :status provided")))
+        (when (= :failed (:status query-result))
+          (throw (Exception. ^String (get query-result :error "general error"))))
         (query-complete query-execution query-result (:cache_result options)))
       (catch Exception ex
         (log/warn ex)
@@ -140,14 +142,14 @@
                  :running_time (- (System/currentTimeMillis) (:start_time_millis query-execution))}]
     ;; record our query execution and format response
     (-> query-execution
-      (dissoc :start_time_millis)
-      (merge updates)
-      (save-query-execution)
-      ;; this is just for the response for clien
-      (assoc :row_count 0
-             :data {:rows []
-                    :cols []
-                    :columns []}))))
+        (dissoc :start_time_millis)
+        (merge updates)
+        (save-query-execution)
+        ;; this is just for the response for clien
+        (assoc :row_count 0
+               :data {:rows []
+                      :cols []
+                      :columns []}))))
 
 (defn query-complete
   "Save QueryExecution state and construct a completed (successful) query response"

src/metabase/driver/generic_sql/native.clj

 (ns metabase.driver.generic-sql.native
   "The `native` query processor."
   (:import com.metabase.corvus.api.ApiException)
-  (:require [clojure.tools.logging :as log]
+  (:require [clojure.java.jdbc :as jdbc]
+            [clojure.tools.logging :as log]
             [korma.core :as korma]
+            korma.db
             [metabase.db :refer [sel]]
             [metabase.driver.generic-sql.util :refer :all]
             [metabase.models.database :refer [Database]]))
@@ -23,23 +25,28 @@
           (or (class->base-type (type v))
               (throw (ApiException. (int 500) (format "Missing base type mapping for %s in metabase.driver.native/class->base-type. Please add an entry."
                                                       (str (type v))))))))
-
-(defn- get-cols [row]
-  (->> row
-       (map (fn [[k v]]
-              {:name k
-               :base_type (value->base-type v)}))))
-
-
-(defn process-and-run [{:keys [native database] :as query}]
+(defn process-and-run
+  "Process and run a native (raw SQL) QUERY."
+  {:arglists '([query])}
+  [{{sql :query} :native
+    database-id :database :as query}]
+  {:pre [(string? sql)
+         (integer? database-id)]}
   (log/debug "QUERY: " query)
-  (let [db (sel :one Database :id database)
-        sql (:query native)
-        ;; this is where we actually run things
-        ;; TODO - exception catching
-        results (korma/exec-raw (korma-db db) sql :results)]
-    {:status :completed
-     :row_count (count results)
-     :data {:rows (map vals results)
-            :columns (keys (first results))
-            :cols (get-cols (first results))}}))
+  (try (let [db (-> (sel :one Database :id database-id)
+                    korma-db
+                    korma.db/get-connection)
+             [columns & [first-row :as rows]] (jdbc/query db sql :as-arrays? true)]
+         {:status :completed
+          :row_count (count rows)
+          :data {:rows rows
+                 :columns columns
+                 :cols (map (fn [column first-value]
+                              {:name column
+                               :base_type (value->base-type first-value)})
+                            columns first-row)}})
+       (catch java.sql.SQLException e
+         {:status :failed
+          :error (->> (.getMessage e)     ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]
+                      (re-find #"^(.*);") ; the user already knows the SQL, and error code is meaningless
+                      second)})))         ; so just return the part of the exception that is relevant

test/metabase/driver/generic_sql/native_test.clj

+(ns metabase.driver.generic-sql.native-test
+  (:require [expectations :refer :all]
+            [metabase.driver.generic-sql.native :refer :all]
+            [metabase.test-data :refer :all]
+            metabase.test-setup))
+
+;; Just check that a basic query works
+(expect {:status :completed
+         :row_count 2
+         :data {:rows [[100]
+                       [99]]
+                :columns [:id]
+                :cols [{:name :id, :base_type :IntegerField}]}}
+  (process-and-run {:native {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}
+                    :database @db-id}))
+
+;; Check that column ordering is maintained
+(expect
+    {:status :completed
+     :row_count 2
+     :data {:rows [[100 "Mohawk Bend" 46]
+                   [99 "Golden Road Brewing" 10]]
+            :columns [:id :name :category_id]
+            :cols [{:name :id, :base_type :IntegerField}
+                   {:name :name, :base_type :TextField}
+                   {:name :category_id, :base_type :IntegerField}]}}
+  (process-and-run {:native {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2;"}
+                    :database @db-id}))
+
+;; Check that we get proper error responses for malformed SQL
+(expect {:status :failed
+         :error "Column \"ZID\" not found"}
+  (process-and-run {:native {:query "SELECT ZID FROM CHECKINS LIMIT 2;"}
+                    :database @db-id}))