Merge pull request #508 from metabase/expand_tables

Expand source Table + DB in expanded query

src/metabase/api/common.clj

@@ -442,7 +442,17 @@
       (check-403 @~'can_read)
       obj#))
   ([entity id]
-   `(read-check (sel :one ~entity :id ~id))))
+   (cond
+     ;; simple optimization : since @can-read is always true for a Database
+     ;; the read-check macro will just resolve to true in this simple case
+     ;; use `name` so we can match 'Database or 'metabase.models.database/Database
+     ;;
+     ;; TODO - it would be nice to generalize the read-checking pattern, and make it
+     ;; a separate multimethod or protocol so other models besides DB can write optimized
+     ;; implementations. Currently, we always fetch an *entire* object to do read checking,
+     ;; which is wasteful.
+     (= (name entity) "Database") `(comment "@(:can-read database) is always true.") ; put some non-constant value here so Eastwood doesn't complain about unused return values
+     :else                        `(read-check (sel :one ~entity :id ~id)))))
 
 (defmacro write-check
   "Checks that `@can_write` is true for this object."

src/metabase/driver/generic_sql/native.clj

@@ -22,13 +22,12 @@
 (defn process-and-run
   "Process and run a native (raw SQL) QUERY."
   {:arglists '([query])}
-  [{{sql :query} :native
-    database-id :database :as query}]
+  [{{sql :query} :native, database-id :database, :as query}]
   {:pre [(string? sql)
          (integer? database-id)]}
   (log/debug "QUERY: \n"
              (with-out-str (clojure.pprint/pprint query)))
-  (try (let [database (sel :one Database :id database-id)
+  (try (let [database (sel :one [Database :engine :details] :id database-id)
              db (-> database
                     db->korma-db
                     korma.db/get-connection)
@@ -36,16 +35,16 @@
                                                 ;; If timezone is specified in the Query and the driver supports setting the timezone then execute SQL to set it
                                                 (when-let [timezone (or (-> query :native :timezone)
                                                                         (driver/report-timezone))]
-                                                  (when-let [timezone->set-timezone-sql (:timezone->set-timezone-sql (driver/database-id->driver database-id))]
+                                                  (when-let [timezone->set-timezone-sql (:timezone->set-timezone-sql (driver/engine->driver (:engine database)))]
                                                     (log/debug "Setting timezone to:" timezone)
                                                     (jdbc/db-do-prepared conn (timezone->set-timezone-sql timezone))))
                                                 (jdbc/query conn sql :as-arrays? true))]
-         {:rows rows
+         {:rows    rows
           :columns columns
-          :cols (map (fn [column first-value]
-                       {:name column
-                        :base_type (value->base-type first-value)})
-                     columns first-row)})
+          :cols    (map (fn [column first-value]
+                          {:name      column
+                           :base_type (value->base-type first-value)})
+                        columns first-row)})
        (catch java.sql.SQLException e
          (let [^String message (or (->> (.getMessage e)     ; error message comes back like 'Column "ZID" not found; SQL statement: ... [error-code]' sometimes
                                         (re-find #"^(.*);") ; the user already knows the SQL, and error code is meaningless

src/metabase/driver/generic_sql/query_processor.clj

@@ -4,7 +4,6 @@
             [clojure.tools.logging :as log]
             [korma.core :refer :all]
             [metabase.config :as config]
-            [metabase.db :refer :all]
             [metabase.driver.query-processor :as qp]
             (metabase.driver.generic-sql [native :as native]
                                          [util :refer :all])
@@ -32,10 +31,10 @@
 
 (defn process-structured
   "Convert QUERY into a korma `select` form, execute it, and annotate the results."
-  [{{:keys [source_table]} :query, :as query}]
+  [{{:keys [source-table]} :query, database :database, :as query}]
   (try
     ;; Process the expanded query and generate a korma form
-    (let [korma-form `(let [entity# (table-id->korma-entity ~source_table)]
+    (let [korma-form `(let [entity# (korma-entity ~database ~source-table)]
                         (select entity# ~@(->> (map apply-form (:query query))
                                                (filter identity)
                                                (mapcat #(if (vector? %) % [%])))))]

src/metabase/driver/generic_sql/util.clj

@@ -6,12 +6,8 @@
             [colorize.core :as color]
             [korma.core :as korma]
             [korma.db :as kdb]
-            [metabase.db :refer [sel]]
             [metabase.driver :as driver]
-            [metabase.driver.query-processor :as qp]
-            (metabase.models [database :refer [Database]]
-                             [field :refer [Field]]
-                             [table :refer [Table]])))
+            [metabase.driver.query-processor :as qp]))
 
 ;; Cache the Korma DB connections for a given Database for 60 seconds instead of creating new ones every single time
 (defn- db->connection-spec [database]
@@ -66,21 +62,16 @@
        ~@body)))
 
 (defn korma-entity
-  "Return a Korma entity for TABLE.
+  "Return a Korma entity for [DB and] TABLE .
 
     (-> (sel :one Table :id 100)
         korma-entity
         (select (aggregate (count :*) :count)))"
-  [{:keys [name db] :as table}]
-  {:pre [(delay? db)]}
-  {:table name
-   :pk    :id
-   :db    (db->korma-db @db)})
-
-(defn table-id->korma-entity
-  "Lookup `Table` with TABLE-ID and return a korma entity that can be used in a korma form."
-  [table-id]
-  {:pre  [(integer? table-id)]
-   :post [(map? %)]}
-  (korma-entity (or (sel :one Table :id table-id)
-                    (throw (Exception. (format "Table with ID %d doesn't exist!" table-id))))))
+  ([{db-delay :db, :as table}]
+   {:pre [(delay? db-delay)]}
+   (korma-entity @db-delay table))
+  ([db {table-name :name}]
+   {:pre [(map? db)]}
+   {:table table-name
+    :pk    :id
+    :db    (db->korma-db db)}))

src/metabase/driver/mongo/query_processor.clj

@@ -10,12 +10,10 @@
                     [query :refer :all])
             [metabase.db :refer :all]
             [metabase.driver :as driver]
-            [metabase.driver.interface :as i]
-            [metabase.driver.query-processor :as qp :refer [*query*]]
+            (metabase.driver [interface :as i]
+                             [query-processor :as qp :refer [*query*]])
             [metabase.driver.mongo.util :refer [with-mongo-connection *mongo-connection* values->base-type]]
-            (metabase.models [database :refer [Database]]
-                             [field :refer [Field]]
-                             [table :refer [Table]])
+            [metabase.models.field :refer [Field]]
             [metabase.util :as u])
   (:import (com.mongodb CommandResult
                         DBApiLayer)
@@ -34,8 +32,8 @@
 
 (defn process-and-run
   "Process and run a MongoDB QUERY."
-  [{query-type :type database-id :database :as query}]
-  (with-mongo-connection [_ (sel :one :fields [Database :details] :id database-id)]
+  [{query-type :type, database :database, :as query}]
+  (with-mongo-connection [_ database]
     (case (keyword query-type)
       :query (let [generated-query (process-structured (:query query))]
                (when-not qp/*disable-qp-logging*
@@ -220,8 +218,8 @@
    *  queries that contain `breakout` clauses are handled by `do-breakout`
    *  other queries are handled by `match-aggregation`, which hands off to the
       appropriate fn defined by a `defaggregation`."
-  [{:keys [source_table aggregation breakout] :as query}]
-  (binding [*collection-name* (sel :one :field [Table :name] :id source_table) ;; TODO - can remove this once expand handles Tables
+  [{:keys [source-table aggregation breakout] :as query}]
+  (binding [*collection-name* (:name source-table)
             *constraints*     (when-let [filter-clause (:filter query)]
                                 (apply-clause [:filter filter-clause]))]
     (if (seq breakout) (do-breakout query)

src/metabase/driver/query_processor.clj

@@ -80,7 +80,7 @@
                             expand
                             preprocess-cumulative-sum)]
     (when-not *disable-qp-logging*
-      (log/debug (u/format-color 'magenta "\n\nPREPROCESSED/EXPANDED:\n%s" (u/pprint-to-str preprocessed-query))))
+      (log/debug (u/format-color 'magenta "\n\nPREPROCESSED/EXPANDED:\n%s" (u/pprint-to-str (assoc-in preprocessed-query [:database :details] "**********"))))) ; obscure DB details when logging
     preprocessed-query))
 
 
@@ -439,12 +439,12 @@
 (defn annotate
   "Take a sequence of RESULTS of executing QUERY and return the \"annotated\" results we pass to postprocessing -- the map with `:cols`, `:columns`, and `:rows`.
    RESULTS should be a sequence of *maps*, keyed by result column -> value."
-  [{{:keys [source_table]} :query, :as query}, results & [uncastify-fn]]
-  {:pre [(integer? source_table)]}
+  [{{{source-table-id :id} :source-table} :query, :as query}, results & [uncastify-fn]]
+  {:pre [(integer? source-table-id)]}
   (let [results         (if-not uncastify-fn results
                                 (for [row results]
                                   (m/map-keys uncastify-fn row)))
-        fields          (sel :many :fields [Field :id :table_id :name :description :base_type :special_type], :table_id source_table, :active true)
+        fields          (sel :many :fields [Field :id :table_id :name :description :base_type :special_type], :table_id source-table-id, :active true)
         ordered-col-kws (order-cols query results fields)]
     {:rows    (for [row results]
                 (mapv row ordered-col-kws))                         ; might as well return each row and col info as vecs because we're not worried about making

src/metabase/driver/query_processor/expand.clj

@@ -42,7 +42,9 @@
             [medley.core :as m]
             [swiss.arrows :refer [-<>]]
             [metabase.db :refer [sel]]
-            [metabase.models.field :as field]
+            (metabase.models [database :refer [Database]]
+                             [field :as field]
+                             [table :refer [Table]])
             [metabase.util :as u])
   (:import (clojure.lang Keyword)))
 
@@ -50,27 +52,32 @@
          parse-breakout
          parse-fields
          parse-filter
-         parse-order-by
-         with-resolved-fields)
+         parse-order-by)
+
 
 ;; ## -------------------- Protocols --------------------
 
-(defprotocol IResolveField
-  "Methods called during `Field` resolution. Placeholder types should implement this protocol."
+(defprotocol IResolve
+  "Methods called during `Field` and `Table` resolution. Placeholder types should implement this protocol."
   (resolve-field [this field-id->fields]
     "This method is called when walking the Query after fetching `Fields`.
      Placeholder objects should lookup the relevant Field in FIELD-ID->FIELDS and
-     return their expanded form. Other objects should just return themselves."))
+     return their expanded form. Other objects should just return themselves.")
+  (resolve-table [this table-id->tables]
+    "Called when walking the Query after `Fields` have been resolved and `Tables` have been fetched.
+     Objects like `Fields` can add relevant information like the name of their `Table`."))
 
 ;; Default impls are just identity
 (extend Object
-  IResolveField {:resolve-field (fn [this _] this)})
+  IResolve {:resolve-field (fn [this _] this)
+            :resolve-table (fn [this _] this)})
 
 (extend nil
-  IResolveField {:resolve-field (constantly nil)})
+  IResolve {:resolve-field (constantly nil)
+            :resolve-table (constantly nil)})
 
 
-;; ## -------------------- Public Interface --------------------
+;; ## -------------------- Expansion - Impl --------------------
 
 (defn- parse [query-dict]
   (update-in query-dict [:query] #(-<> (assoc %
@@ -79,38 +86,84 @@
                                               :fields      (parse-fields      (:fields %))
                                               :filter      (parse-filter      (:filter %))
                                               :order_by    (parse-order-by    (:order_by %)))
-                                       (set/rename-keys <> {:order_by :order-by})
+                                       (set/rename-keys <> {:order_by     :order-by
+                                                            :source_table :source-table})
                                        (m/filter-vals identity <>))))
 
+(def ^:private ^:dynamic *field-ids*
+  "Bound to an atom containing a set when a parsing function is ran"
+  nil)
+
+(defn- resolve-fields
+  "Resolve the `Fields` in an EXPANDED-QUERY-DICT."
+  [expanded-query-dict field-ids]
+  (if-not (seq field-ids) expanded-query-dict ; No need to do a DB call or walk expanded-query-dict if we didn't see any Field IDs
+          (let [fields (->> (sel :many :id->fields [field/Field :name :base_type :special_type :table_id] :id [in field-ids])
+                            (m/map-vals #(set/rename-keys % {:id           :field-id
+                                                             :name         :field-name
+                                                             :special_type :special-type
+                                                             :base_type    :base-type
+                                                             :table_id     :table-id})))]
+            ;; This is performed depth-first so we don't end up walking the newly-created Field/Value objects
+            ;; they may have nil values; this was we don't have to write an implementation of resolve-field for nil
+            (walk/postwalk #(resolve-field % fields) expanded-query-dict))))
+
+(defn- resolve-database
+  "Resolve the `Database` in question for an EXPANDED-QUERY-DICT."
+  [{database-id :database, :as expanded-query-dict}]
+  (assoc expanded-query-dict :database (sel :one :fields [Database :name :id :engine :details] :id database-id)))
+
+(defn- resolve-tables
+  "Resolve the `Tables` in an EXPANDED-QUERY-DICT."
+  [{{source-table-id :source-table} :query, database-id :database, :as expanded-query-dict}]
+  ;; TODO - this doesn't handle join tables yet
+  (let [table (sel :one :fields [Table :name :id] :id source-table-id)]
+    (->> (assoc-in expanded-query-dict [:query :source-table] table)
+         (walk/postwalk #(resolve-table % {(:id table) table})))))
+
+
+;; ## -------------------- Public Interface --------------------
+
 (defn expand
-  "Expand a query-dict."
+  "Expand a QUERY-DICT."
   [query-dict]
-  (with-resolved-fields parse query-dict))
+  (binding [*field-ids* (atom #{})]
+    (some-> query-dict
+            parse
+            (resolve-fields @*field-ids*)
+            resolve-database
+            resolve-tables)))
 
 
 ;; ## -------------------- Field + Value --------------------
 
 ;; Field is the expansion of a Field ID in the standard QL
-(defrecord Field [field-id
-                  field-name
-                  base-type
-                  special-type])
+(defrecord Field [^Integer field-id
+                  ^String  field-name
+                  ^Keyword base-type
+                  ^Keyword special-type
+                  ^Integer table-id
+                  ^String  table-name]
+  IResolve
+  (resolve-table [this table-id->table]
+    (cond-> this
+      table-id (assoc :table-name (:name (table-id->table table-id))))))
 
 ;; Value is the expansion of a value within a QL clause
 ;; Information about the associated Field is included for convenience
 (defrecord Value [value              ; e.g. parsed Date / timestamp
                   original-value     ; e.g. original YYYY-MM-DD string
-                  base-type
-                  special-type
-                  field-id
-                  field-name])
+                  ^Keyword base-type
+                  ^Keyword special-type
+                  ^Integer field-id
+                  ^String  field-name])
 
 
 ;; ## -------------------- Placeholders --------------------
 
 ;; Replace Field IDs with these during first pass
 (defrecord FieldPlaceholder [field-id]
-  IResolveField
+  IResolve
   (resolve-field [this field-id->fields]
     (-> (:field-id this)
         field-id->fields
@@ -132,7 +185,7 @@
 ;; Replace values with these during first pass over Query.
 ;; Include associated Field ID so appropriate the info can be found during Field resolution
 (defrecord ValuePlaceholder [field-id value]
-  IResolveField
+  IResolve
   (resolve-field [this field-id->fields]
     (-> (:field-id this)
         field-id->fields
@@ -140,10 +193,6 @@
         parse-value
         map->Value)))
 
-(def ^:private ^:dynamic *field-ids*
-  "Bound to an atom containing a set when a parsing function is ran"
-  nil)
-
 (defn- ph
   "Create a new placeholder object for a Field ID or value.
    If `*field-ids*` is bound, "
@@ -155,26 +204,6 @@
    (->ValuePlaceholder field-id value)))
 
 
-;; ## -------------------- Field Resolution --------------------
-
-(defn- with-resolved-fields
-  "Call (PARSER-FN FORM), collecting the `Field` IDs encountered; then fetch the relevant Fields
-   and walk the parsed form, calling `resolve-field` on each element."
-  [parser-fn form]
-  (when form
-    (binding [*field-ids* (atom #{})]
-      (when-let [parsed-form (parser-fn form)]
-        (if-not (seq @*field-ids*) parsed-form ; No need to do a DB call or walk parsed-form if we didn't see any Field IDs
-                (let [fields (->> (sel :many :id->fields [field/Field :name :base_type :special_type] :id [in @*field-ids*])
-                                  (m/map-vals #(set/rename-keys % {:id           :field-id
-                                                                   :name         :field-name
-                                                                   :special_type :special-type
-                                                                   :base_type    :base-type})))]
-                  ;; This is performed depth-first so we don't end up walking the newly-created Field/Value objects
-                  ;; they may have nil values; this was we don't have to write an implementation of resolve-field for nil
-                  (walk/postwalk #(resolve-field % fields) parsed-form)))))))
-
-
 ;; # ======================================== CLAUSE DEFINITIONS ========================================
 
 (defmacro defparser