Adds multi-agg support and fixes nil falsey values

src/metabase/driver/presto.clj

@@ -62,21 +62,20 @@
   (let [parsers (map (comp field-type->parser :type) columns)]
     (for [row data]
       (for [[value parser] (partition 2 (interleave row parsers))]
-        (when value
+        (when (some? value)
           (parser value))))))
 
 (defn- fetch-presto-results! [details {prev-columns :columns, prev-rows :rows} uri]
-  (ssh/with-ssh-tunnel [details-with-tunnel details]
-    (let [{{:keys [columns data nextUri error]} :body} (http/get uri (assoc (details->request details-with-tunnel) :as :json))]
-      (when error
-        (throw (ex-info (or (:message error) "Error running query.") error)))
-      (let [rows    (parse-presto-results columns data)
-            results {:columns (or columns prev-columns)
-                     :rows    (vec (concat prev-rows rows))}]
-        (if (nil? nextUri)
-          results
-          (do (Thread/sleep 100) ; Might not be the best way, but the pattern is that we poll Presto at intervals
-              (fetch-presto-results! details-with-tunnel results nextUri)))))))
+  (let [{{:keys [columns data nextUri error]} :body} (http/get uri (assoc (details->request details) :as :json))]
+    (when error
+      (throw (ex-info (or (:message error) "Error running query.") error)))
+    (let [rows    (parse-presto-results columns data)
+          results {:columns (or columns prev-columns)
+                   :rows    (vec (concat prev-rows rows))}]
+      (if (nil? nextUri)
+        results
+        (do (Thread/sleep 100) ; Might not be the best way, but the pattern is that we poll Presto at intervals
+            (fetch-presto-results! details results nextUri))))))
 
 (defn- execute-presto-query! [details query]
   (ssh/with-ssh-tunnel [details-with-tunnel details]
@@ -100,6 +99,13 @@
 (defn- quote+combine-names [& names]
   (str/join \. (map quote-name names)))
 
+(defn- rename-duplicates [values]
+  ;; Appends _2, _3 and so on to duplicated values
+  (loop [acc [], [h & tail] values, seen {}]
+    (let [value (if (seen h) (str h "_" (inc (seen h))) h)]
+      (if tail
+        (recur (conj acc value) tail (assoc seen h (inc (get seen h 0))))
+        (conj acc value)))))
 
 ;;; IDriver implementation
 
@@ -192,10 +198,13 @@
 
 (defn- execute-query [{:keys [database settings], {sql :query, params :params} :native, :as outer-query}]
   (let [sql                    (str "-- " (qputil/query->remark outer-query) "\n"
-                                          (unprepare/unprepare (cons sql params) :quote-escape "'", :iso-8601-fn  :from_iso8601_timestamp))
+                                          (unprepare/unprepare (cons sql params) :quote-escape "'", :iso-8601-fn :from_iso8601_timestamp))
         details                (merge (:details database) settings)
-        {:keys [columns rows]} (execute-presto-query! details sql)]
-    {:columns (map (comp keyword :name) columns)
+        {:keys [columns rows]} (execute-presto-query! details sql)
+        columns                (for [[col name] (map vector columns (rename-duplicates (map :name columns)))]
+                                 {:name name, :base_type (presto-type->base-type (:type col))})]
+    {:cols    columns
+     :columns (map (comp keyword :name) columns)
      :rows    rows}))
 
 (defn- field-values-lazy-seq [{field-name :name, :as field}]

test/metabase/driver/presto_test.clj

@@ -10,7 +10,7 @@
             [toucan.db :as db])
   (:import metabase.driver.presto.PrestoDriver))
 
-(resolve-private-vars metabase.driver.presto details->uri details->request parse-presto-results quote-name quote+combine-names apply-page)
+(resolve-private-vars metabase.driver.presto details->uri details->request parse-presto-results quote-name quote+combine-names rename-duplicates apply-page)
 
 ;;; HELPERS
 
@@ -53,6 +53,11 @@
   (parse-presto-results [{:type "date"} {:type "timestamp with time zone"} {:type "timestamp"} {:type "decimal(10,4)"} {:type "varchar(255)"}]
                         [["2017-04-03", "2017-04-03 10:19:17.417 America/Toronto", "2017-04-03 10:19:17.417", "3.1416", "test"]]))
 
+(expect
+  [[0, false, "", nil]]
+  (parse-presto-results [{:type "integer"} {:type "boolean"} {:type "varchar(255)"} {:type "date"}]
+                        [[0, false, "", nil]]))
+
 (expect
   "\"weird.table\"\" name\""
   (quote-name "weird.table\" name"))
@@ -61,6 +66,10 @@
   "\"weird . \"\"schema\".\"weird.table\"\" name\""
   (quote+combine-names "weird . \"schema" "weird.table\" name"))
 
+(expect
+  ["name" "count" "count_2" "sum", "sum_2", "sum_3"]
+  (rename-duplicates ["name" "count" "count" "sum" "sum" "sum"]))
+
 ;; DESCRIBE-DATABASE
 (datasets/expect-with-engine :presto
   {:tables #{{:name "categories" :schema "default"}

test/metabase/query_processor_test/aggregation_test.clj

@@ -149,8 +149,8 @@
             (ql/aggregation (ql/avg $price) (ql/count) (ql/sum $price))))))
 
 ;; make sure that multiple aggregations of the same type have the correct metadata (#4003)
-;; (TODO - this isn't tested against Mongo, BigQuery or Presto because those drivers don't currently work correctly with multiple columns with the same name)
-(datasets/expect-with-engines (disj non-timeseries-engines :mongo :bigquery :presto)
+;; (TODO - this isn't tested against Mongo or BigQuery because those drivers don't currently work correctly with multiple columns with the same name)
+(datasets/expect-with-engines (disj non-timeseries-engines :mongo :bigquery)
   [(aggregate-col :count)
    (assoc (aggregate-col :count)
      :display_name    "Count 2"