Merge pull request #367 from metabase/query_preprocessor

Query preprocessor

src/metabase/driver.clj

@@ -6,6 +6,7 @@
             [medley.core :refer :all]
             [metabase.db :refer [exists? ins sel upd]]
             (metabase.driver [interface :as i]
+                             [query-processor :as qp]
                              [result :as result])
             (metabase.models [database :refer [Database]]
                              [query-execution :refer [QueryExecution]])
@@ -39,10 +40,12 @@
    (fn [engine]
      {:pre [(keyword? engine)]}
      (let [ns-symb (symbol (format "metabase.driver.%s" (name engine)))]
+       (log/debug (format "Loading metabase.driver.%s..." (name engine)))
        (require ns-symb)
        (let [driver (some-> (ns-resolve ns-symb 'driver)
                             var-get)]
          (assert driver)
+         (log/debug "Ok.")
          driver)))))
 
 ;; Can the type of a DB change?
@@ -86,7 +89,9 @@
 (defn process-query
   "Process a structured or native query, and return the result."
   [query]
-  (i/process-query (database-id->driver (:database query)) query))
+  (binding [qp/*query* query]
+    (i/process-query (database-id->driver (:database query))
+                     (qp/preprocess query))))
 
 
 ;; ## Query Execution Stuff

src/metabase/driver/generic_sql/query_processor.clj

@@ -19,26 +19,21 @@
          query-is-cumulative-sum?
          apply-cumulative-sum)
 
-(def ^:dynamic ^:private *query*
-  "Query dictionary that we're currently processing"
-  nil)
-
 ;; # INTERFACE
 
 (defn process
   "Convert QUERY into a korma `select` form."
   [{{:keys [source_table] :as query} :query}]
   (when-not (zero? source_table)
-    (binding [*query* query]
-      (let [forms (->> (map apply-form query)                    ; call `apply-form` for each clause and strip out nil results
-                       (filter identity)
-                       (mapcat (fn [form] (if (vector? form) form ; some `apply-form` implementations return a vector of multiple korma forms; if only one was
-                                             [form])))           ; returned wrap it in a vec so `mapcat` can build a flattened sequence of forms
-                       doall)]
-        (when (config/config-bool :mb-db-logging)
-          (log-query query forms))
-        `(let [entity# (table-id->korma-entity ~source_table)]
-           (select entity# ~@forms))))))
+    (let [forms (->> (map apply-form query)                    ; call `apply-form` for each clause and strip out nil results
+                     (filter identity)
+                     (mapcat (fn [form] (if (vector? form) form ; some `apply-form` implementations return a vector of multiple korma forms; if only one was
+                                           [form])))           ; returned wrap it in a vec so `mapcat` can build a flattened sequence of forms
+                     doall)]
+      (when (config/config-bool :mb-db-logging)
+        (log-query query forms))
+      `(let [entity# (table-id->korma-entity ~source_table)]
+         (select entity# ~@forms)))))
 
 
 (defn process-structured
@@ -108,20 +103,9 @@
 ;;
 ;;     [1412 1413]
 (defmethod apply-form :breakout [[_ field-ids]]
-  (match field-ids
-    []    nil ; empty clause
-    [nil] nil ; empty clause
-    _     (let [field-names (map field-id->kw field-ids)
-                order-by-field-names (some->> (:order_by *query*) ; get set of names of all fields specified in `order_by`
-                                              (map first)
-                                              (map field-id->kw)
-                                              set)]
-            `[(group  ~@field-names)
-              (fields ~@field-names)
-              ~@(->> field-names                                                    ; Add an implicit `order :ASC` clause for every field specified in `breakout`
-                     (filter (complement (partial contains? order-by-field-names))) ; that is *not* specified *explicitly* in `order_by`.
-                     (map (fn [field-name]
-                            `(order ~field-name :ASC))))])))
+  (let [field-names (map field-id->kw field-ids)]
+    `[(group  ~@field-names)
+      (fields ~@field-names)]))
 
 ;; ### `:fields`
 ;; ex.
@@ -166,9 +150,6 @@
 
 (defmethod apply-form :filter [[_ filter-clause]]
   (match filter-clause
-    nil                  nil ; empty clause
-    [nil nil]            nil ; empty clause
-    []                   nil ; empty clause
     ["AND" & subclauses] `(where (~'and ~@(map filter-subclause->predicate
                                                subclauses)))
     ["OR" & subclauses]  `(where (~'or  ~@(map filter-subclause->predicate
@@ -180,8 +161,7 @@
 ;;
 ;;     10
 (defmethod apply-form :limit [[_ value]]
-  (when value
-    `(limit ~value)))
+  `(limit ~value))
 
 ;; ### `:order_by`
 ;; ex.

src/metabase/driver/interface.clj

@@ -35,7 +35,9 @@
 
   ;; Query Processing
   (process-query [this query]
-    "Process a native or structured query."))
+    "Process a native or structured query.
+     (Don't use this directly; instead, use `metabase.driver/process-query`,
+     which does things like preprocessing before calling the appropriate implementation.)"))
 
 
 ;; ## ISyncDriverTableFKs Protocol (Optional)

src/metabase/driver/query_processor.clj

+(ns metabase.driver.query-processor
+  "Preprocessor that does simple transformations to all incoming queries, simplifing the driver-specific implementations.")
+
+(declare add-implicit-breakout-order-by
+         preprocess-structured
+         remove-empty-clauses)
+
+(def ^:dynamic *query* "The structured query we're currently processing, before any preprocessing occurs (i.e. the `:query` part of the API call body)"
+  nil)
+
+(defn preprocess [{query-type :type :as query}]
+  (case (keyword query-type)
+    :query (preprocess-structured query)
+    :native query))
+
+(defn preprocess-structured [query]
+  (update-in query [:query] #(->> %
+                                  remove-empty-clauses
+                                  add-implicit-breakout-order-by)))
+
+
+;; ## PREPROCESSOR FNS
+
+;; ### REMOVE-EMPTY-CLAUSES
+(def ^:const clause->empty-forms
+  "Clause values that should be considered empty and removed during preprocessing."
+  {:breakout #{[nil]}
+   :filter   #{[nil nil]}})
+
+(defn remove-empty-clauses
+  "Remove all QP clauses whose value is:
+   1.  is `nil`
+   2.  is an empty sequence (e.g. `[]`)
+   3.  matches a form in `clause->empty-forms`"
+  [query]
+  (->> query
+       (map (fn [[clause clause-value]]
+              (when (and clause-value
+                         (or (not (sequential? clause-value))
+                             (seq clause-value)))
+                (when-not (contains? (clause->empty-forms clause) clause-value)
+                  [clause clause-value]))))
+       (into {})))
+
+
+;; ### ADD-IMPLICIT-BREAKOUT-ORDER-BY
+
+(defn add-implicit-breakout-order-by
+  "Field IDs specified in `breakout` should add an implicit ascending `order_by` subclause *unless* that field is *explicitly* referenced in `order_by`."
+  [{breakout-field-ids :breakout order-by-subclauses :order_by :as query}]
+  (let [order-by-field-ids (set (map first order-by-subclauses))
+        implicit-breakout-order-by-field-ids (filter (partial (complement contains?) order-by-field-ids)
+                                                     breakout-field-ids)]
+    (if-not (seq implicit-breakout-order-by-field-ids) query
+            (->> implicit-breakout-order-by-field-ids
+                 (mapv (fn [field-id]
+                         [field-id "ascending"]))
+                 (apply conj (or order-by-subclauses []))
+                 (assoc query :order_by)))))