From 92ff768e576e448bad3a56234a60aaac5e8cbba4 Mon Sep 17 00:00:00 2001
From: Cam Saul <cam@geotip.com>
Date: Wed, 29 Apr 2015 17:44:17 -0700
Subject: [PATCH] the first combined QP test
---
.dir-locals.el | 2 +
.../generic_sql/query_processor/annotate.clj | 37 +-----
src/metabase/driver/mongo.clj | 16 +--
src/metabase/driver/mongo/query_processor.clj | 22 ++--
src/metabase/driver/mongo/util.clj | 22 +++-
src/metabase/driver/query_processor.clj | 44 ++++++-
src/metabase/models/field_values.clj | 6 +-
.../generic_sql/query_processor_test.clj | 18 ---
test/metabase/driver/mongo/test_data.clj | 9 +-
test/metabase/driver/query_processor_test.clj | 124 ++++++++++++++++++
test/metabase/test_data.clj | 7 +
11 files changed, 222 insertions(+), 85 deletions(-)
create mode 100644 test/metabase/driver/query_processor_test.clj
diff --git a/.dir-locals.el b/.dir-locals.el
index ac1a14bb216..ee8ad0ec4f3 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -19,6 +19,7 @@
(expect-eval-actual-first 1)
(expect-expansion 0)
(expect-let 1)
+ (expect-with-all-drivers 1)
(expect-with-data-loaded 1)
(ins 1)
(let-400 1)
@@ -29,6 +30,7 @@
(macrolet 1)
(org-perms-case 1)
(pdoseq 1)
+ (qp-expect-with-all-drivers 1)
(symbol-macrolet 1)
(sync-in-context 2)
(upd 2)
diff --git a/src/metabase/driver/generic_sql/query_processor/annotate.clj b/src/metabase/driver/generic_sql/query_processor/annotate.clj
index 0d5cd9231ae..063d8e615a7 100644
--- a/src/metabase/driver/generic_sql/query_processor/annotate.clj
+++ b/src/metabase/driver/generic_sql/query_processor/annotate.clj
@@ -1,11 +1,11 @@
(ns metabase.driver.generic-sql.query-processor.annotate
"Functions related to annotating results returned by the Query Processor."
(:require [metabase.db :refer :all]
+ [metabase.driver.query-processor :as qp]
[metabase.models.field :refer [Field field->fk-table]]))
(declare get-column-names
get-column-info
- get-special-column-info
uncastify)
(defn annotate
@@ -95,36 +95,7 @@
(or (second (re-find #"CAST\(([^\s]+) AS [\w]+\)" column-name))
column-name))
-(defn- get-column-info
- "Get extra information about result columns. This is done by looking up matching `Fields` for the `Table` in QUERY or looking up
- information about special columns such as `count` via `get-special-column-info`."
+(defn get-column-info
+ "Wrapper for `metabase.driver.query-processor/get-column-info` that calls `uncastify` on column names."
[query column-names]
- (let [table-id (get-in query [:query :source_table])
- column-names (map uncastify column-names)
- columns (->> (sel :many [Field :id :table_id :name :description :base_type :special_type] ; lookup columns with matching names for this Table
- :table_id table-id :name [in (set column-names)])
- (map (fn [{:keys [name] :as column}] ; build map of column-name -> column
- {name (-> (select-keys column [:id :table_id :name :description :base_type :special_type])
- (assoc :extra_info (if-let [fk-table (field->fk-table column)]
- {:target_table_id (:id fk-table)}
- {})))}))
- (into {}))]
- (->> column-names
- (map (fn [column-name]
- (or (columns column-name) ; try to get matching column from the map we build earlier
- (get-special-column-info query column-name))))))) ; if it's not there then it's a special column like `count`
-
-(defn- get-special-column-info
- "Get info like `:base_type` and `:special_type` for a special aggregation column like `count` or `sum`."
- [query column-name]
- (merge {:name column-name
- :id nil
- :table_id nil
- :description nil}
- (let [aggregation-type (keyword column-name) ; For aggregations of a specific Field (e.g. `sum`)
- field-aggregation? (contains? #{:avg :stddev :sum} aggregation-type)] ; lookup the field we're aggregating and return its
- (if field-aggregation? (sel :one :fields [Field :base_type :special_type] ; type info. (The type info of the aggregate result
- :id (-> query :query :aggregation second)) ; will be the same.)
- (case aggregation-type ; Otherwise for general aggregations such as `count`
- :count {:base_type :IntegerField ; just return hardcoded type info
- :special_type :number})))))
+ (qp/get-column-info query (map uncastify column-names)))
diff --git a/src/metabase/driver/mongo.clj b/src/metabase/driver/mongo.clj
index 5181ce41de8..fadcfa50d28 100644
--- a/src/metabase/driver/mongo.clj
+++ b/src/metabase/driver/mongo.clj
@@ -13,7 +13,7 @@
[metabase.driver :as driver]
[metabase.driver.interface :refer :all]
(metabase.driver.mongo [query-processor :as qp]
- [util :refer [*mongo-connection* with-mongo-connection]])))
+ [util :refer [*mongo-connection* with-mongo-connection values->base-type]])))
(declare driver)
@@ -29,22 +29,12 @@
(r/reduce set/union))))
(defn- field->base-type
- "Determine the base type of FIELD in the most ghetto way possible.
- This just gets counts the types of *every* value of FIELD and returns the class whose count was highest."
+ "Determine the base type of FIELD in the most ghetto way possible, via `values->base-type`."
[field]
{:pre [(map? field)]
:post [(keyword? %)]}
(with-mongo-connection [_ @(:db @(:table field))]
- (or (->> (field-values-lazy-seq driver field)
- (filter identity)
- (group-by type)
- (map (fn [[type valus]]
- [type (count valus)]))
- (sort-by second)
- first
- first
- driver/class->base-type)
- :UnknownField)))
+ (values->base-type (field-values-lazy-seq driver field))))
;;; ## MongoDriver
diff --git a/src/metabase/driver/mongo/query_processor.clj b/src/metabase/driver/mongo/query_processor.clj
index 9099953f666..94d4903e4c4 100644
--- a/src/metabase/driver/mongo/query_processor.clj
+++ b/src/metabase/driver/mongo/query_processor.clj
@@ -9,8 +9,8 @@
[query :refer :all])
[metabase.db :refer :all]
[metabase.driver :as driver]
- [metabase.driver.query-processor :refer [*query*]]
- [metabase.driver.mongo.util :refer [with-mongo-connection *mongo-connection*]]
+ [metabase.driver.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]]))
@@ -146,23 +146,17 @@
(defn annotate-results [{:keys [source_table] :as query} results]
{:pre [(integer? source_table)]}
- (let [field-name->id (sel :many :field->id [Field :name] :table_id source_table)
- column-names (keys (first results))]
+ (let [field-name->field (sel :many :field->obj [Field :name] :table_id source_table)
+ column-keys (keys (first results))
+ column-names (map name column-keys)]
{:row_count (count results)
:status :completed
:data {:columns column-names
- :cols (map (fn [column-name]
- {:name column-name
- :id (field-name->id (name column-name))
- :table_id source_table
- :description nil
- :base_type :UnknownField
- :special_type nil
- :extra_info {}})
- column-names)
- :rows (map #(map % column-names)
+ :cols (qp/get-column-info {:query query} column-names)
+ :rows (map #(map % column-keys)
results)}}))
+
;; ## CLAUSE APPLICATION 2.0
(def field-id->kw
diff --git a/src/metabase/driver/mongo/util.clj b/src/metabase/driver/mongo/util.clj
index c6c45078164..71876b88973 100644
--- a/src/metabase/driver/mongo/util.clj
+++ b/src/metabase/driver/mongo/util.clj
@@ -2,7 +2,8 @@
"`*mongo-connection*`, `with-mongo-connection`, and other functions shared between several Mongo driver namespaces."
(:require [clojure.tools.logging :as log]
[colorize.core :as color]
- [monger.core :as mg]))
+ [monger.core :as mg]
+ [metabase.driver :as driver]))
(def ^:dynamic *mongo-connection*
"Connection to a Mongo database.
@@ -41,3 +42,22 @@
~@body)]
(if *mongo-connection* (f# *mongo-connection*)
(-with-mongo-connection f# ~database))))
+
+;; TODO - this is actually more sophisticated than the one used for annotation in the GenericSQL driver, which just takes the
+;; types of the values in the first row.
+;; We should move this somewhere where it can be shared amongst the drivers and rewrite GenericSQL to use it instead.
+(defn values->base-type
+ "Given a sequence of values, return `Field` `base_type` in the most ghetto way possible.
+ This just gets counts the types of *every* value and returns the `base_type` for class whose count was highest."
+ [values-seq]
+ {:pre [(sequential? values-seq)]}
+ (or (->> values-seq
+ (filter identity)
+ (group-by type)
+ (map (fn [[type valus]]
+ [type (count valus)]))
+ (sort-by second)
+ first
+ first
+ driver/class->base-type)
+ :UnknownField))
diff --git a/src/metabase/driver/query_processor.clj b/src/metabase/driver/query_processor.clj
index 338a6b582b5..6ecdb14c17d 100644
--- a/src/metabase/driver/query_processor.clj
+++ b/src/metabase/driver/query_processor.clj
@@ -1,7 +1,10 @@
(ns metabase.driver.query-processor
- "Preprocessor that does simple transformations to all incoming queries, simplifing the driver-specific implementations.")
+ "Preprocessor that does simple transformations to all incoming queries, simplifing the driver-specific implementations."
+ (:require [metabase.db :refer :all]
+ [metabase.models.field :refer [Field field->fk-table]]))
(declare add-implicit-breakout-order-by
+ get-special-column-info
preprocess-structured
remove-empty-clauses)
@@ -57,3 +60,42 @@
[field-id "ascending"]))
(apply conj (or order-by-subclauses []))
(assoc query :order_by)))))
+
+
+;; # COMMON ANNOTATION FNS
+
+(defn get-column-info
+ "Get extra information about result columns. This is done by looking up matching `Fields` for the `Table` in QUERY or looking up
+ information about special columns such as `count` via `get-special-column-info`."
+ [query column-names]
+ {:pre [(:query query)]}
+ (let [table-id (get-in query [:query :source_table])
+ columns (->> (sel :many [Field :id :table_id :name :description :base_type :special_type] ; lookup columns with matching names for this Table
+ :table_id table-id :name [in (set column-names)])
+ (map (fn [{:keys [name] :as column}] ; build map of column-name -> column
+ {name (-> (select-keys column [:id :table_id :name :description :base_type :special_type])
+ (assoc :extra_info (if-let [fk-table (field->fk-table column)]
+ {:target_table_id (:id fk-table)}
+ {})))}))
+ (into {}))]
+ (->> column-names
+ (map (fn [column-name]
+ (or (columns column-name) ; try to get matching column from the map we build earlier
+ (get-special-column-info query column-name))))))) ; if it's not there then it's a special column like `count`
+
+
+(defn get-special-column-info
+ "Get info like `:base_type` and `:special_type` for a special aggregation column like `count` or `sum`."
+ [query column-name]
+ {:pre [(:query query)]}
+ (merge {:name column-name
+ :id nil
+ :table_id nil
+ :description nil}
+ (let [aggregation-type (keyword column-name) ; For aggregations of a specific Field (e.g. `sum`)
+ field-aggregation? (contains? #{:avg :stddev :sum} aggregation-type)] ; lookup the field we're aggregating and return its
+ (if field-aggregation? (sel :one :fields [Field :base_type :special_type] ; type info. (The type info of the aggregate result
+ :id (-> query :query :aggregation second)) ; will be the same.)
+ (case aggregation-type ; Otherwise for general aggregations such as `count`
+ :count {:base_type :IntegerField ; just return hardcoded type info
+ :special_type :number})))))
diff --git a/src/metabase/models/field_values.clj b/src/metabase/models/field_values.clj
index c65191233d2..71439494707 100644
--- a/src/metabase/models/field_values.clj
+++ b/src/metabase/models/field_values.clj
@@ -2,7 +2,6 @@
(:require [clojure.tools.logging :as log]
[korma.core :refer :all]
[metabase.db :refer :all]
- [metabase.db.metadata-queries :as metadata]
[metabase.util :as u]))
;; ## Entity + DB Multimethods
@@ -36,6 +35,9 @@
(or (contains? #{:category :city :state :country} (keyword special_type))
(= (keyword base_type) :BooleanField)))
+(def ^:private field-distinct-values
+ (u/runtime-resolved-fn 'metabase.db.metadata-queries 'field-distinct-values))
+
(defn create-field-values
"Create `FieldValues` for a `Field`."
{:arglists '([field]
@@ -46,7 +48,7 @@
(log/debug (format "Creating FieldValues for Field %d..." field-id))
(ins FieldValues
:field_id field-id
- :values (metadata/field-distinct-values field)
+ :values (field-distinct-values field)
:human_readable_values human-readable-values))
(defn create-field-values-if-needed
diff --git a/test/metabase/driver/generic_sql/query_processor_test.clj b/test/metabase/driver/generic_sql/query_processor_test.clj
index a78883a79a7..80789d91d2e 100644
--- a/test/metabase/driver/generic_sql/query_processor_test.clj
+++ b/test/metabase/driver/generic_sql/query_processor_test.clj
@@ -15,24 +15,6 @@
{:extra_info {} :special_type :latitude, :base_type :FloatField, :description nil, :name "LATITUDE", :table_id (table->id :venues), :id (field->id :venues :latitude)}
{:extra_info {} :special_type nil, :base_type :TextField, :description nil, :name "NAME", :table_id (table->id :venues), :id (field->id :venues :name)}]))
-;; ## "COUNT" AGGREGATION
-(expect {:status :completed
- :row_count 1
- :data {:rows [[100]]
- :columns ["count"]
- :cols [{:base_type :IntegerField
- :special_type :number
- :name "count"
- :id nil
- :table_id nil
- :description nil}]}}
- (driver/process-query {:type :query
- :database @db-id
- :query {:source_table (table->id :venues)
- :filter [nil nil]
- :aggregation ["count"]
- :breakout [nil]
- :limit nil}}))
;; ## "SUM" AGGREGATION
(expect {:status :completed
diff --git a/test/metabase/driver/mongo/test_data.clj b/test/metabase/driver/mongo/test_data.clj
index 0d80c7623c1..eda1d5b6af7 100644
--- a/test/metabase/driver/mongo/test_data.clj
+++ b/test/metabase/driver/mongo/test_data.clj
@@ -1,6 +1,7 @@
(ns metabase.driver.mongo.test-data
"Functionality related to creating / loading a test database for the Mongo driver."
- (:require [colorize.core :as color]
+ (:require [clojure.tools.logging :as log]
+ [colorize.core :as color]
[medley.core :as m]
(monger [collection :as mc]
[core :as mg])
@@ -10,7 +11,7 @@
(metabase.models [database :refer [Database]]
[field :refer [Field]]
[table :refer [Table]])
- [metabase.test-data :refer :all]
+ [metabase.test-data :refer [org-id]]
[metabase.test-data.data :as data]))
(declare load-data)
@@ -37,8 +38,10 @@
:name mongo-test-db-name
:engine :mongo
:details {:conn_str mongo-test-db-conn-str})]
+ (log/debug (color/cyan "Loading Mongo test data..."))
(load-data)
(driver/sync-database! db)
+ (log/debug (color/cyan "Done."))
db))))
(def mongo-test-db-id
@@ -108,4 +111,4 @@
(mc/insert mongo-db (name collection) (assoc (zipmap fields row)
:_id (inc i)))
(catch com.mongodb.MongoException$DuplicateKey _)))
- (println (color/cyan (format "Loaded data for collection '%s'." (name collection))))))))
+ (log/debug (color/cyan (format "Loaded data for collection '%s'." (name collection))))))))
diff --git a/test/metabase/driver/query_processor_test.clj b/test/metabase/driver/query_processor_test.clj
new file mode 100644
index 00000000000..1f1e4bd40c7
--- /dev/null
+++ b/test/metabase/driver/query_processor_test.clj
@@ -0,0 +1,124 @@
+(ns metabase.driver.query-processor-test
+ "Query processing tests that can be ran between any of the available drivers, and should give the same results."
+ (:require [expectations :refer :all]
+ [metabase.db :refer :all]
+ [metabase.driver :as driver]
+ [metabase.driver.mongo.test-data :as mongo-data]
+ (metabase.models [table :refer [Table]])
+ [metabase.test-data :as generic-sql-data]))
+
+;; ## Functionality for writing driver-independent tests
+
+(def ^:dynamic *db* "Bound to `Database` for the current driver inside body of `with-driver`.")
+(def ^:dynamic *db-id* "Bound to ID of `Database` for the current driver inside body of `with-driver`.")
+(def ^:dynamic *driver-data*)
+
+(defprotocol DriverData
+ (db [this]
+ "Return `Database` containing test data for this driver.")
+ (table-name->table [this table-name]
+ "Given a TABLE-NAME keyword like `:venues`, *fetch* the corresponding `Table`."))
+
+(deftype MongoDriverData []
+ DriverData
+ (db [_]
+ @mongo-data/mongo-test-db)
+ (table-name->table [_ table-name]
+ (mongo-data/table-name->table table-name)))
+
+(deftype GenericSqlDriverData []
+ DriverData
+ (db [_]
+ @generic-sql-data/test-db)
+ (table-name->table [_ table-name]
+ (generic-sql-data/table-name->table table-name)))
+
+(def driver-name->driver-data
+ {:mongo (MongoDriverData.)
+ :generic-sql (GenericSqlDriverData.)})
+
+(def driver-name->db-delay
+ "Map of `driver-name` to a delay that will return the corresponding `Database`."
+ {:mongo mongo-data/mongo-test-db
+ :generic-sql generic-sql-data/test-db})
+(def valid-driver-names (set (keys driver-name->db-delay)))
+
+(defmacro with-driver
+ "Execute BODY with `*db*` and `*db-id*` bound to appropriate places for "
+ [driver-name & body]
+ {:pre [(keyword? driver-name)
+ (contains? valid-driver-names driver-name)]}
+ `(let [driver-data# (driver-name->driver-data ~driver-name)
+ db# (db driver-data#)]
+ (binding [*driver-data* driver-data#
+ *db* db#
+ *db-id* (:id db#)]
+ (assert (integer? *db-id*))
+ ~@body)))
+
+(defmacro expect-with-all-drivers
+ "Like expect, but runs a test inside of `with-driver` for *each* of the available drivers."
+ [expected actual]
+ `(do ~@(mapcat (fn [driver-name]
+ `[(expect
+ (with-driver ~driver-name
+ ~expected)
+ (with-driver ~driver-name
+ ~actual))])
+ valid-driver-names)))
+
+
+;; ## Driver-Independent Data Fns
+
+(defn ->table
+ "Given keyword TABLE-NAME, fetch corresponding `Table` in `*db*`."
+ [table-name]
+ {:pre [*driver-data*]
+ :post [(map? %)]}
+ (table-name->table *driver-data* table-name))
+
+(defn ->table-id [table-name]
+ {:pre [*driver-data*]
+ :post [(integer? %)]}
+ (:id (->table table-name)))
+
+#_(def ^{:arglists '([table-name])} table-name->id
+ "Given keyword TABLE-NAME, fetch ID of corresponding `Table` in `*db*`."
+ (let [-table-name->id (memoize (fn [db-id table-name]
+ {:pre [(integer? db-id)
+ (keyword? table-name)]
+ :post [(integer? %)]}
+ (sel :one :id Table :db_id db-id :name (name table-name))))]
+ (fn [table-name]
+ {:pre [(integer? *db-id*)]}
+ (-table-name->id *db-id* table-name))))
+
+
+;; ## Driver-Independent QP Tests
+
+(defmacro qp-expect-with-all-drivers
+ "Slightly more succinct way of writing QP tests. Adds standard boilerplate to actual/expected forms."
+ [data query]
+ `(expect-with-all-drivers
+ {:status :completed
+ :row_count ~(count (:rows data))
+ :data ~data}
+ (driver/process-query {:type :query
+ :database *db-id*
+ :query ~query})))
+
+;; ### "COUNT" AGGREGATION
+(qp-expect-with-all-drivers
+ {:rows [[100]]
+ :columns ["count"]
+ :cols [{:base_type :IntegerField
+ :special_type :number
+ :name "count"
+ :id nil
+ :table_id nil
+ :description nil}]}
+ {:source_table (->table-id :venues)
+ :filter [nil nil]
+ :aggregation ["count"]
+ :breakout [nil]
+ :limit nil})
diff --git a/test/metabase/test_data.clj b/test/metabase/test_data.clj
index 88e2b01b0a1..15e66cea811 100644
--- a/test/metabase/test_data.clj
+++ b/test/metabase/test_data.clj
@@ -75,6 +75,13 @@
(not (zero? %))]}
(@tables table-name))
+(defn table-name->table
+ "Fetch `Table` with TABLE-NAME."
+ [table-name]
+ {:pre [(keyword? table-name)]
+ :post [(map? %)]}
+ (sel :one Table :id (table->id table-name)))
+
;; ## Test Organization
--
GitLab