Merge pull request #1490 from metabase/condense-qp-process-fns

Tighten up QP process-* functions [WIP]

src/metabase/driver/query_processor.clj

@@ -38,6 +38,13 @@
 ;; |                                     QP INTERNAL IMPLEMENTATION                                     |
 ;; +----------------------------------------------------------------------------------------------------+
 
+
+(defn structured-query?
+  "Predicate function which returns `true` if the given query represents a structured style query, `false` otherwise."
+  [query]
+  (= :query (keyword (:type query))))
+
+
 (defn- wrap-catch-exceptions [qp]
   (fn [query]
     (try (qp query)
@@ -55,7 +62,9 @@
 
 (defn- pre-expand [qp]
   (fn [query]
-    (qp (resolve/resolve (expand/expand query)))))
+    (qp (if (structured-query? query)
+          (resolve/resolve (expand/expand query))
+          query))))
 
 
 (defn- post-add-row-count-and-status
@@ -80,40 +89,46 @@
   "Add an implicit `fields` clause to queries with `rows` aggregations."
   [qp]
   (fn [{{:keys [source-table], {source-table-id :id} :source-table} :query, :as query}]
-    (qp (if-not (should-add-implicit-fields? query)
-          query
-          (let [fields (for [field (sel :many :fields [Field :name :display_name :base_type :special_type :preview_display :display_name :table_id :id :position :description]
-                                        :table_id   source-table-id
-                                        :active     true
-                                        :field_type [not= "sensitive"]
-                                        :parent_id  nil
-                                        (k/order :position :asc) (k/order :id :desc))]
-                         (let [field (-> (resolve/rename-mb-field-keys field)
-                                         map->Field
-                                         (resolve/resolve-table {source-table-id source-table}))]
-                           (if (or (contains? #{:DateField :DateTimeField} (:base-type field))
-                                   (contains? #{:timestamp_seconds :timestamp_milliseconds} (:special-type field)))
-                             (map->DateTimeField {:field field, :unit :day})
-                             field)))]
-            (if-not (seq fields)
-              (do (log/warn (format "Table '%s' has no Fields associated with it." (:name source-table)))
-                  query)
-              (-> query
-                  (assoc-in [:query :fields-is-implicit] true)
-                  (assoc-in [:query :fields] fields))))))))
+    (if (structured-query? query)
+      (qp (if-not (should-add-implicit-fields? query)
+            query
+            (let [fields (for [field (sel :many :fields [Field :name :display_name :base_type :special_type :preview_display :display_name :table_id :id :position :description]
+                                          :table_id   source-table-id
+                                          :active     true
+                                          :field_type [not= "sensitive"]
+                                          :parent_id  nil
+                                          (k/order :position :asc) (k/order :id :desc))]
+                           (let [field (-> (resolve/rename-mb-field-keys field)
+                                           map->Field
+                                           (resolve/resolve-table {source-table-id source-table}))]
+                             (if (or (contains? #{:DateField :DateTimeField} (:base-type field))
+                                     (contains? #{:timestamp_seconds :timestamp_milliseconds} (:special-type field)))
+                               (map->DateTimeField {:field field, :unit :day})
+                               field)))]
+              (if-not (seq fields)
+                (do (log/warn (format "Table '%s' has no Fields associated with it." (:name source-table)))
+                    query)
+                (-> query
+                    (assoc-in [:query :fields-is-implicit] true)
+                    (assoc-in [:query :fields] fields))))))
+      ;; for non-structured queries we do nothing
+      (qp query))))
 
 
 (defn- pre-add-implicit-breakout-order-by
   "`Fields` specified in `breakout` should add an implicit ascending `order-by` subclause *unless* that field is *explicitly* referenced in `order-by`."
   [qp]
   (fn [{{breakout-fields :breakout, order-by :order-by} :query, :as query}]
-    (let [order-by-fields                   (set (map :field order-by))
-          implicit-breakout-order-by-fields (filter (partial (complement contains?) order-by-fields)
-                                                    breakout-fields)]
-      (qp (cond-> query
-            (seq implicit-breakout-order-by-fields) (update-in [:query :order-by] concat (for [field implicit-breakout-order-by-fields]
-                                                                                           (map->OrderBySubclause {:field     field
-                                                                                                                   :direction :ascending}))))))))
+    (if (structured-query? query)
+      (let [order-by-fields                   (set (map :field order-by))
+            implicit-breakout-order-by-fields (filter (partial (complement contains?) order-by-fields)
+                                                      breakout-fields)]
+        (qp (cond-> query
+              (seq implicit-breakout-order-by-fields) (update-in [:query :order-by] concat (for [field implicit-breakout-order-by-fields]
+                                                                                             (map->OrderBySubclause {:field     field
+                                                                                                                     :direction :ascending}))))))
+      ;; for non-structured queries we do nothing
+      (qp query))))
 
 
 (defn- pre-cumulative-sum
@@ -173,9 +188,12 @@
 
 (defn- cumulative-sum [qp]
   (fn [query]
-    (let [[cumulative-sum-field query] (pre-cumulative-sum query)]
-      (cond->> (qp query)
-        cumulative-sum-field (post-cumulative-sum cumulative-sum-field)))))
+    (if (structured-query? query)
+      (let [[cumulative-sum-field query] (pre-cumulative-sum query)]
+        (cond->> (qp query)
+                 cumulative-sum-field (post-cumulative-sum cumulative-sum-field)))
+      ;; for non-structured queries we do nothing
+      (qp query))))
 
 
 (defn- limit
@@ -193,7 +211,8 @@
 
 (defn- pre-log-query [qp]
   (fn [query]
-    (when-not *disable-qp-logging*
+    (when (and (structured-query? query)
+               (not *disable-qp-logging*))
       (log/debug (u/format-color 'magenta "\n\nPREPROCESSED/EXPANDED: 😻\n%s"
                                  (u/pprint-to-str
                                   ;; Remove empty kv pairs because otherwise expanded query is HUGE
@@ -208,6 +227,16 @@
     (qp query)))
 
 
+(defn- wrap-guard-multiple-calls
+  "Throw an exception if a QP function accidentally calls (QP QUERY) more than once."
+  [qp]
+  (let [called? (atom false)]
+    (fn [query]
+      (assert (not @called?) "(QP QUERY) IS BEING CALLED MORE THAN ONCE!")
+      (reset! called? true)
+      (qp query))))
+
+
 ;; +------------------------------------------------------------------------------------------------------------------------+
 ;; |                                                     QUERY PROCESSOR                                                    |
 ;; +------------------------------------------------------------------------------------------------------------------------+
@@ -237,18 +266,14 @@
 ;; Pre-processing happens from top-to-bottom, i.e. the QUERY passed to the function returned by POST-ADD-ROW-COUNT-AND-STATUS is the
 ;; query as modified by PRE-EXPAND.
 ;;
-;; Pre-processing then happens in order from bottom-to-top; i.e. POST-ANNOTATE gets to modify the results, then LIMIT, then CUMULATIVE-SUM, etc.
+;; Post-processing then happens in order from bottom-to-top; i.e. POST-ANNOTATE gets to modify the results, then LIMIT, then CUMULATIVE-SUM, etc.
 
-(defn- wrap-guard-multiple-calls
-  "Throw an exception if a QP function accidentally calls (QP QUERY) more than once."
-  [qp]
-  (let [called? (atom false)]
-    (fn [query]
-      (assert (not @called?) "(QP QUERY) IS BEING CALLED MORE THAN ONCE!")
-      (reset! called? true)
-      (qp query))))
 
-(defn- process-structured [{:keys [driver], :as query}]
+(defn process
+  "Process a QUERY and return the results."
+  [driver query]
+  (when-not *disable-qp-logging*
+    (log/debug (u/format-color 'blue "\nQUERY: 😎\n%s" (u/pprint-to-str query))))
   (let [driver-process-query      (:process-query driver)
         driver-wrap-process-query (or (:process-query-in-context driver)
                                       (fn [qp] qp))]
@@ -263,26 +288,5 @@
           annotate/post-annotate
           pre-log-query
           wrap-guard-multiple-calls
-          driver-process-query) query)))
-
-(defn- process-native [{:keys [driver], :as query}]
-  (let [driver-process-query      (:process-query driver)
-        driver-wrap-process-query (or (:process-query-in-context driver)
-                                      (fn [qp] qp))]
-    ((<<- wrap-catch-exceptions
-          driver-wrap-process-query
-          post-add-row-count-and-status
-          limit
-          wrap-guard-multiple-calls
-          driver-process-query) query)))
-
-(defn process
-  "Process a QUERY and return the results."
-  [driver query]
-  (when-not *disable-qp-logging*
-    (log/debug (u/format-color 'blue "\nQUERY: 😎\n%s" (u/pprint-to-str query))))
-  ((case (keyword (:type query))
-     :native process-native
-     :query  process-structured)
-   (assoc query
-          :driver driver)))
+          driver-process-query) (assoc query
+                                       :driver driver))))

src/metabase/driver/query_processor/annotate.clj

@@ -248,12 +248,15 @@
       expected by the frontend."
   [qp]
   (fn [query]
-    (let [results     (qp query)
-          result-keys (set (keys (first results)))
-          cols        (resolve-sort-and-format-columns (:query query) result-keys)
-          columns     (mapv :name cols)]
-      {:cols    (vec (for [col cols]
-                       (update col :name name)))
-       :columns (mapv name columns)
-       :rows    (for [row results]
-                  (mapv row columns))})))
+    (if (= :query (keyword (:type query)))
+      (let [results     (qp query)
+            result-keys (set (keys (first results)))
+            cols        (resolve-sort-and-format-columns (:query query) result-keys)
+            columns     (mapv :name cols)]
+        {:cols    (vec (for [col cols]
+                         (update col :name name)))
+         :columns (mapv name columns)
+         :rows    (for [row results]
+                    (mapv row columns))})
+      ;; for non-structured queries we do nothing
+      (qp query))))

test/metabase/driver/query_processor_test.clj

@@ -223,6 +223,12 @@
 
 ;; # THE TESTS THEMSELVES (!)
 
+;; structured-query?
+(expect false (structured-query? {}))
+(expect false (structured-query? {:type "native"}))
+(expect true (structured-query? {:type "query"}))
+
+
 ;; ### "COUNT" AGGREGATION
 (qp-expect-with-all-datasets
     {:rows    [[100]]