Merge pull request #8763 from metabase/make-sure-GA-metrics-come-back-with-type-info

Make sure GA metrics come back with correct type info

src/metabase/driver/bigquery.clj

@@ -564,10 +564,9 @@
       (let [sql     (str "-- " (qputil/query->remark outer-query) "\n" (if (seq params)
                                                                          (unprepare/unprepare (cons sql params))
                                                                          sql))
-            results (process-native* database sql)
-            results (cond->> results
-                      mbql? (post-process-mbql table-name))]
-        (assoc results :annotate? mbql?)))))
+            results (process-native* database sql)]
+        (cond->> results
+          mbql? (post-process-mbql table-name))))))
 
 
 ;; BigQuery doesn't return a timezone with it's time strings as it's always UTC, JodaTime parsing also defaults to UTC

src/metabase/driver/druid/query_processor.clj

@@ -1054,10 +1054,9 @@
     ;; rename any occurances of `:timestamp___int` to `:timestamp` in the results so the user doesn't know about our
     ;; behind-the-scenes conversion and apply any other post-processing on the value such as parsing some units to int
     ;; and rounding up approximate cardinality values.
-    {:columns   (->> columns
-                     (replace {:timestamp___int :timestamp :distinct___count :count})
-                     (map u/keyword->qualified-name))
-     :rows      (for [row (:results post-proc-map)]
-                  (for [getter getters]
-                    (getter row)))
-     :annotate? mbql?}))
+    {:columns (->> columns
+                   (replace {:timestamp___int :timestamp :distinct___count :count})
+                   (map u/keyword->qualified-name))
+     :rows    (for [row (:results post-proc-map)]
+                (for [getter getters]
+                  (getter row)))}))

src/metabase/driver/googleanalytics/query_processor.clj

@@ -5,6 +5,7 @@
             [clojure.tools.reader.edn :as edn]
             [metabase.mbql.util :as mbql.u]
             [metabase.query-processor.store :as qp.store]
+            [metabase.util :as u]
             [metabase.util
              [date :as du]
              [i18n :as ui18n :refer [tru]]
@@ -328,13 +329,11 @@
 (defn execute-query
   "Execute a QUERY using the provided DO-QUERY function, and return the results in the usual format."
   [do-query query]
-  (let [mbql?            (:mbql? (:native query))
-        ^GaData response (do-query query)
+  (let [^GaData response (do-query query)
         columns          (map header->column (.getColumnHeaders response))
         getters          (map header->getter-fn (.getColumnHeaders response))]
-    {:columns  (map :name columns)
-     :cols     columns
+    {:cols     columns
+     :columns  (map (comp u/keyword->qualified-name :name) columns)
      :rows     (for [row (.getRows response)]
                  (for [[data getter] (map vector row getters)]
-                   (getter data)))
-     :annotate mbql?}))
+                   (getter data)))}))

src/metabase/driver/mongo/query_processor.clj

@@ -641,7 +641,6 @@
                           projections))]
     (when mbql?
       (check-columns columns results))
-    {:columns   (map name columns)
-     :rows      (for [row results]
-                  (mapv row columns))
-     :annotate? mbql?}))
+    {:columns (map name columns)
+     :rows    (for [row results]
+                (mapv row columns))}))

src/metabase/query_processor/middleware/annotate.clj

 (ns metabase.query-processor.middleware.annotate
-  "Middleware for annotating (adding type information to) the results of a query and sorting the columns in the
-  results."
+  "Middleware for annotating (adding type information to) the results of a query, under the `:cols` column."
   (:require [clojure.string :as str]
             [metabase
              [driver :as driver]
@@ -187,9 +186,7 @@
            (ag->name-info &match))))
 
 
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; |                                                   Middleware                                                   |
-;;; +----------------------------------------------------------------------------------------------------------------+
+;;; ----------------------------------------- Putting it all together (MBQL) -----------------------------------------
 
 (defn- check-correct-number-of-columns-returned [mbql-cols results]
   (let [expected-count (count mbql-cols)
@@ -242,17 +239,19 @@
 ;;; |                                               GENERAL MIDDLEWARE                                               |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- add-column-info* [{query-type :type, :as query} results]
-  (if-not (or (= query-type :query)
-              (:annotate? results))
-    (add-native-column-info results)
-    (add-mbql-column-info query results)))
+(defn- add-column-info* [{query-type :type, :as query} {cols-returned-by-driver :cols, :as results}]
+  (cond-> (if-not (= query-type :query)
+            (add-native-column-info results)
+            (add-mbql-column-info query results))
+    ;; If the driver returned a `:cols` map with its results, which is completely optional, merge our `:cols` derived
+    ;; from logic above with theirs. We'll prefer the values in theirs to ours. This is important for wacky drivers
+    ;; like GA that use things like native metrics, which we have no information about.
+    ;;
+    ;; It's the responsibility of the driver to make sure the `:cols` are returned in the correct number and order.
+    (seq cols-returned-by-driver) (update :cols #(map merge % cols-returned-by-driver))))
 
 (defn add-column-info
-  "Middleware for adding type information to columns returned by running a query, and sorting the columns in the
-  results."
+  "Middleware for adding type information about the columns in the query results (the `:cols` key)."
   [qp]
   (fn [query]
-    (let [results (qp query)]
-      (-> (add-column-info* query results)
-          (dissoc :annotate?)))))
+    (add-column-info* query (qp query))))

test/metabase/query_processor/middleware/annotate_test.clj

@@ -179,3 +179,21 @@
     (data/$ids venues
       (qp.store/store-field! (Field $price))
       (col-info-for-aggregation-clause [:sum [:+ [:field-id $price] 1]]))))
+
+;; if a driver is kind enough to supply us with some information about the `:cols` that come back, we should include
+;; that information in the results. Their information should be preferred over ours
+(expect
+  {:cols    [{:name         "totalEvents"
+              :display_name "Total Events"
+              :base_type    :type/Text
+              :source       :aggregation}]
+   :columns ["totalEvents"]}
+  (binding [qp.i/*driver* (H2Driver.)]
+    ((annotate/add-column-info (constantly {:cols    [{:name         "totalEvents"
+                                                       :display_name "Total Events"
+                                                       :base_type    :type/Text}]
+                                            :columns ["totalEvents"]}))
+     {:database (data/id)
+      :type     :query
+      :query    {:source-table (data/id :venues)
+                 :aggregation  [[:metric "ga:totalEvents"]]}})))