MBQL '98

src/metabase/driver/query_processor.clj

@@ -172,7 +172,7 @@
    (Cumulative sum is a special case; it is implemented in post-processing).
 
    Return a pair of [`cumulative-sum-field?` `query`]."
-  [{{{ag-type :aggregation-type, ag-field :field} :aggregation, breakout-fields :breakout, order-by :order_by} :query, :as query}]
+  [{{{ag-type :aggregation-type, ag-field :field} :aggregation, breakout-fields :breakout} :query, :as query}]
   (let [cum-sum?                    (= ag-type :cumulative-sum)
         cum-sum-with-breakout?      (and cum-sum?
                                          (seq breakout-fields))
@@ -187,9 +187,8 @@
       ;; If there's only one breakout field that is the same as the cum_sum field, re-write this as a "rows" aggregation
       ;; to just fetch all the values of the field in question.
       cum-sum-with-same-breakout? [ag-field (update-in query [:query] #(-> %
-                                                                           (dissoc :breakout)
-                                                                           (assoc :aggregation {:aggregation-type :rows}
-                                                                                  :fields      [ag-field])))]
+                                                                           (dissoc :breakout :aggregation)
+                                                                           (assoc :fields [ag-field])))]
 
       ;; Otherwise if we're breaking out on different fields, rewrite the query as a "sum" aggregation
       cum-sum-with-breakout? [ag-field (assoc-in query [:query :aggregation] {:aggregation-type :sum, :field ag-field})]

src/metabase/driver/query_processor/expand.clj

@@ -4,12 +4,13 @@
   (:refer-clojure :exclude [< <= > >= = != and or filter count distinct sum])
   (:require (clojure [core :as core]
                      [string :as str])
+            [clojure.tools.logging :as log]
+            [schema.core :as s]
             [metabase.db :as db]
             [metabase.driver :as driver]
             [metabase.driver.query-processor.interface :refer [*driver*], :as i]
             [metabase.models.table :refer [Table]]
-            [metabase.util :as u]
-            [schema.core :as s])
+            [metabase.util :as u])
   (:import (metabase.driver.query_processor.interface AgFieldRef
                                                       BetweenFilter
                                                       ComparisonFilter
@@ -68,7 +69,7 @@
   "Aggregate field referece, e.g. for use in an `order-by` clause.
 
      (query (aggregate (count))
-            (order-by [(aggregate-field 0) :ascending])) ; order by :count"
+            (order-by (asc (aggregate-field 0)))) ; order by :count"
   [index :- s/Int]
   (i/map->AgFieldRef {:index index}))
 
@@ -151,11 +152,13 @@
      (aggregation {} :count 100))"
   [query ag & args]
   (if (map? ag)
-    (do (s/validate i/Aggregation ag)
-        (assoc query :aggregation ag))
+    (let [ag (update ag :aggregation-type normalize-token)]
+      (s/validate i/Aggregation ag)
+      (assoc query :aggregation ag))
     (let [ag-type (normalize-token ag)]
       (if (core/= ag-type :rows)
-        query
+        (do (log/warn (u/format-color 'yellow "Specifying :rows as the aggregation type is deprecated; this is the default behavior, so you don't need to specify it."))
+            query)
         (aggregation query (apply-fn-for-token ag-type args))))))
 
 
@@ -275,27 +278,29 @@
   (cond
     (map? subclause)    subclause
     (vector? subclause) (let [[f direction] subclause]
+                          (log/warn (u/format-color 'yellow (str "You're using legacy order-by syntax: [<field> :ascending/:descending] is deprecated. "
+                                                                 "Prefer [:asc <field>] or [:desc <field>] instead.")))
                           {:field (field f), :direction (normalize-token direction)})))
 
 (s/defn ^:always-validate asc :- i/OrderBy
   "order-by subclause. Specify that results should be returned in ascending order for Field or AgRef F.
 
      (order-by {} (asc 100))"
-  [f :- i/FieldPlaceholderOrAgRef]
+  [f]
   {:field (field f), :direction :ascending})
 
 (s/defn ^:always-validate desc :- i/OrderBy
   "order-by subclause. Specify that results should be returned in ascending order for Field or AgRef F.
 
      (order-by {} (desc 100))"
-  [f :- i/FieldPlaceholderOrAgRef]
+  [f]
   {:field (field f), :direction :descending})
 
 (defn order-by
   "Specify how ordering should be done for this query.
 
      (order-by {} (asc 20))        ; order by field 20
-     (order-by {} [20 :ascending]) ; order by field 20 (legacy syntax)
+     (order-by {} [20 :ascending]) ; order by field 20 (deprecated/legacy syntax)
      (order-by {} [(aggregate-field 0) :descending]) ; order by the aggregate field (e.g. :count)"
   [query & subclauses]
   (assoc query :order-by (mapv maybe-parse-order-by-subclause subclauses)))
@@ -363,15 +368,20 @@
        ~@body
        validate-query))
 
+(s/defn ^:always-validate wrap-inner-query
+  "Wrap inner QUERY with `:database` ID and other 'outer query' kvs. DB ID is fetched by looking up the Database for the query's `:source-table`."
+  {:style/indent 0}
+  [query :- i/Query]
+  {:database (db/sel :one :field [Table :db_id], :id (:source-table query))
+   :type     :query
+   :query    query})
+
 (s/defn ^:always-validate run-query*
   "Call `driver/process-query` on expanded inner QUERY, looking up the `Database` ID for the `source-table.`
 
      (run-query* (query (source-table 5) ...))"
   [query :- i/Query]
-  (let [db-id (db/sel :one :field [Table :db_id], :id (:source-table query))]
-    (driver/process-query {:database db-id
-                           :type     :query
-                           :query    query})))
+  (driver/process-query (wrap-inner-query query)))
 
 (defmacro run-query
   "Build and run a query.

src/metabase/models/card.clj

@@ -28,7 +28,9 @@
 
 
 (defn- populate-query-fields [card]
-  (let [{{table-id :source_table} :query database-id :database query-type :type} (:dataset_query card)
+  (let [{query :query, database-id :database, query-type :type} (:dataset_query card)
+        table-id (or (:source_table query)  ; legacy (MBQL '95)
+                     (:source-table query))
         defaults {:database_id database-id
                   :table_id    table-id
                   :query_type  (keyword query-type)}]

test/metabase/api/card_test.clj

@@ -3,13 +3,13 @@
   (:require [expectations :refer :all]
             [metabase.db :refer :all]
             [metabase.http-client :refer :all]
+            [metabase.driver.query-processor.expand :as ql]
             (metabase.models [card :refer [Card]]
                              [common :as common]
                              [database :refer [Database]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp]]
-            [metabase.test.util.q :refer [Q-expand]]))
+            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp obj->json->obj]]))
 
 ;; # CARD LIFECYCLE
 
@@ -20,17 +20,20 @@
                                            :can_read               true
                                            :can_write              true
                                            :display                "scalar"
-                                           :dataset_query          (Q-expand aggregate count of categories)
+                                           :dataset_query          (obj->json->obj (ql/wrap-inner-query
+                                                                                    (query categories
+                                                                                      (ql/aggregation :count))))
                                            :visualization_settings {:global {:title nil}}}))
 
 ;; ## GET /api/card
 ;; Filter cards by database
-(expect [true
-         false
-         true]
+(expect
+  [true
+   false
+   true]
   (with-temp Database [{dbid :id} {:name    (random-name)
-                                    :engine  :h2
-                                    :details {}}]
+                                   :engine  :h2
+                                   :details {}}]
     (with-temp Card [{id1 :id} {:name                   (random-name)
                                 :public_perms           common/perms-none
                                 :creator_id             (user->id :crowberto)
@@ -111,20 +114,22 @@
 ;; Test that we can make a card
 (let [card-name (random-name)]
   (expect-eval-actual-first (match-$ (sel :one Card :name card-name)
-                              {:description nil
-                               :organization_id nil
-                               :name card-name
-                               :creator_id (user->id :rasta)
-                               :updated_at $
-                               :dataset_query (Q-expand aggregate count of categories)
-                               :id $
-                               :display "scalar"
+                              {:description            nil
+                               :organization_id        nil
+                               :name                   card-name
+                               :creator_id             (user->id :rasta)
+                               :updated_at             $
+                               :dataset_query          (obj->json->obj (ql/wrap-inner-query
+                                                                        (query categories
+                                                                          (ql/aggregation :count))))
+                               :id                     $
+                               :display                "scalar"
                                :visualization_settings {:global {:title nil}}
-                               :public_perms 0
-                               :created_at $
-                               :database_id (id)
-                               :table_id (id :categories)
-                               :query_type "query"})
+                               :public_perms           0
+                               :created_at             $
+                               :database_id            (id)
+                               :table_id               (id :categories)
+                               :query_type             "query"})
     (post-card card-name)))
 
 ;; ## GET /api/card/:id
@@ -149,7 +154,9 @@
                      :email "rasta@metabase.com",
                      :id $})
          :updated_at $
-         :dataset_query (Q-expand aggregate count of categories)
+         :dataset_query (obj->json->obj (ql/wrap-inner-query
+                                         (query categories
+                                           (ql/aggregation :count))))
          :id $
          :display "scalar"
          :visualization_settings {:global {:title nil}}

test/metabase/api/dashboard_test.clj

@@ -3,6 +3,7 @@
   (:require [expectations :refer :all]
             [metabase.api.card-test :refer [post-card]]
             [metabase.db :refer :all]
+            [metabase.driver.query-processor.expand :as ql]
             (metabase.models [hydrate :refer [hydrate]]
                              [card :refer [Card]]
                              [common :as common]
@@ -11,8 +12,7 @@
                              [user :refer [User]])
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp]]
-            [metabase.test.util.q :refer [Q-expand]]))
+            [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp obj->json->obj]]))
 
 ;; # DASHBOARD LIFECYCLE
 
@@ -112,38 +112,40 @@
       (let [{card-id :id :as card} (sel :one Card :name card-name)
             dash-id (sel :one :id Dashboard :name dash-name)]
         [(match-$ (sel :one DashboardCard :dashboard_id dash-id :card_id card-id)
-           {:sizeX 2
-            :card (match-$ card
-                    {:description nil
-                     :creator (-> (User (user->id :rasta))
-                                  (select-keys [:date_joined :last_name :id :is_superuser :last_login :first_name :email :common_name]))
-                     :organization_id nil
-                     :name $
-                     :creator_id (user->id :rasta)
-                     :updated_at $
-                     :dataset_query (Q-expand aggregate count of categories)
-                     :id card-id
-                     :display "scalar"
-                     :visualization_settings {:global {:title nil}}
-                     :public_perms 0
-                     :created_at $
-                     :database_id (id)
-                     :table_id (id :categories)
-                     :query_type "query"})
-            :updated_at $
-            :col nil
-            :id $
-            :card_id card-id
+           {:sizeX        2
+            :card         (match-$ card
+                            {:description            nil
+                             :creator                (-> (User (user->id :rasta))
+                                                         (select-keys [:date_joined :last_name :id :is_superuser :last_login :first_name :email :common_name]))
+                             :organization_id        nil
+                             :name                   $
+                             :creator_id             (user->id :rasta)
+                             :updated_at             $
+                             :dataset_query          (obj->json->obj (ql/wrap-inner-query
+                                                                       (query categories
+                                                                         (ql/aggregation :count))))
+                             :id                     card-id
+                             :display                "scalar"
+                             :visualization_settings {:global {:title nil}}
+                             :public_perms           0
+                             :created_at             $
+                             :database_id            (id)
+                             :table_id               (id :categories)
+                             :query_type             "query"})
+            :updated_at   $
+            :col          nil
+            :id           $
+            :card_id      card-id
             :dashboard_id dash-id
-            :created_at $
-            :sizeY 2
-            :row nil})])
-    (let [{card-id :id} (post-card card-name)
-          {dash-id :id} (create-dash dash-name)]
-      ((user->client :rasta) :post 200 (format "dashboard/%d/cards" dash-id) {:cardId card-id})
-      (->> ((user->client :rasta) :get 200 (format "dashboard/%d" dash-id))
-           :dashboard
-           :ordered_cards))))
+            :created_at   $
+            :sizeY        2
+            :row          nil})])
+      (let [{card-id :id} (post-card card-name)
+            {dash-id :id} (create-dash dash-name)]
+        ((user->client :rasta) :post 200 (format "dashboard/%d/cards" dash-id) {:cardId card-id})
+        (->> ((user->client :rasta) :get 200 (format "dashboard/%d" dash-id))
+             :dashboard
+             :ordered_cards))))
 
 ;; ## DELETE /api/dashboard/:id/cards
 (let [card-name (random-name)

test/metabase/api/dataset_test.clj

@@ -3,11 +3,11 @@
   (:require [expectations :refer :all]
             [korma.core :as k]
             [metabase.db :refer :all]
+            [metabase.driver.query-processor.expand :as ql]
             [metabase.models.query-execution :refer [QueryExecution]]
             [metabase.test.data :refer :all]
             [metabase.test.data.users :refer :all]
-            [metabase.test.util :refer [match-$ expect-eval-actual-first]]
-            [metabase.test.util.q :refer [Q-expand]]))
+            [metabase.test.util :refer [match-$ expect-eval-actual-first]]))
 
 ;;; ## POST /api/meta/dataset
 ;; Just a basic sanity check to make sure Query Processor endpoint is still working correctly.
@@ -21,7 +21,9 @@
        :status    "completed"
        :id        $
        :uuid      $})
-  ((user->client :rasta) :post 200 "dataset" (Q-expand aggregate count of checkins)))
+  ((user->client :rasta) :post 200 "dataset" (ql/wrap-inner-query
+                                               (query checkins
+                                                 (ql/aggregation :count)))))
 
 ;; Even if a query fails we still expect a 200 response from the api
 (expect-eval-actual-first

test/metabase/driver/mysql_test.clj

@@ -13,6 +13,6 @@
 
 (expect-with-engine :mysql
   [[1 nil]]
-  (data/dataset all-zero-dates
-    (ql/run-query
-      (ql/source-table (data/id :exciting-moments-in-history)))))
+  (-> (data/dataset metabase.driver.mysql-test/all-zero-dates
+        (data/run-query exciting-moments-in-history))
+      :data :rows))

test/metabase/driver/postgres_test.clj

@@ -52,9 +52,8 @@
 (expect-with-engine :postgres
   [{:name "id",      :base_type :IntegerField}
    {:name "user_id", :base_type :UUIDField}]
-  (->> (data/dataset with-uuid
-         (ql/run-query
-           (ql/source-table (data/id :users))))
+  (->> (data/dataset metabase.driver.postgres-test/with-uuid
+         (data/run-query users))
        :data
        :cols
        (mapv (u/rpartial select-keys [:name :base_type]))))
@@ -63,8 +62,7 @@
 ;; Check that we can filter by a UUID Field
 (expect-with-engine :postgres
   [[2 #uuid "4652b2e7-d940-4d55-a971-7e484566663e"]]
-  (-> (data/dataset with-uuid
-        (ql/run-query
-          (ql/source-table (data/id :users))
-          (ql/filter (ql/= (data/id :users :user_id) "4652b2e7-d940-4d55-a971-7e484566663e"))))
+  (-> (data/dataset metabase.driver.postgres-test/with-uuid
+        (data/run-query users
+          (ql/filter (ql/= $user_id "4652b2e7-d940-4d55-a971-7e484566663e"))))
       :data :rows))

test/metabase/test/data.clj

 (ns metabase.test.data
   "Code related to creating and deleting test databases + datasets."
-  (:require [clojure.tools.logging :as log]
+  (:require [clojure.string :as s]
+            [clojure.tools.logging :as log]
             (metabase [db :refer :all]
                       [driver :as driver])
+            [metabase.driver.query-processor.expand :as ql]
             (metabase.models [database :refer [Database]]
                              [field :refer [Field] :as field]
                              [table :refer [Table]])
@@ -46,11 +48,10 @@
 
 (defn do-with-dataset
   "Bind `Database` for DATASET as the current DB and execute F.
-   DATASET is a *symbol* that can be resolved in the current namespace or in `metabase.test.data.dataset-definitions`:
+   DATASET is an optionally namespace-qualified *symbol*. If not namespace-qualified, `metabase.test.data.dataset-definitions` is assumed.
 
-     (do-with-dataset 'some-local-db-def f)
      (do-with-dataset 'some-other-ns/some-db-def f)
-     (do-with-dataset 'sad-toucan-incidents)         ; metabase.test.data.dataset-definitions/sad-toucan-incidents"
+     (do-with-dataset 'sad-toucan-incidents)        ; metabase.test.data.dataset-definitions/sad-toucan-incidents"
   [dataset f]
   {:pre [(symbol? dataset)]}
   (let [dataset-var (or (resolve dataset)
@@ -63,7 +64,7 @@
 (defmacro dataset
   "Convenience wrapper for `do-with-dataset`.
    Bind `Database` for DATASET as the current DB and execute BODY.
-   DATASET is a unquoted symbol name of a dataset resolvable in the current namespace or in `metabase.test.data.dataset-definitions`.
+   DATASET is a unquoted symbol name of a dataset; if not namespace-qualified, `metabase.test.data.dataset-definitions` is assumed.
 
      (dataset sad-toucan-incidents
        ...)"
@@ -71,6 +72,49 @@
   [dataset & body]
   `(do-with-dataset '~dataset (fn [] ~@body)))
 
+
+(defn- $->id
+  "Convert symbols like `$field` to `id` fn calls. Input is split into separate args by splitting the token on `.`.
+   With no `.` delimiters, it is assumed we're referring to a Field belonging to TABLE-NAME, which is passed implicitly as the first arg.
+   With one or more `.` delimiters, no implicit TABLE-NAME arg is passed to `id`:
+
+    $venue_id  -> (id :sightings :venue_id) ; TABLE-NAME is implicit first arg
+    $cities.id -> (id :cities :id)          ; specify non-default Table"
+  [table-name body]
+  (clojure.walk/prewalk (fn [form]
+                          (if (and (symbol? form)
+                                   (= (first (name form)) \$))
+                            (let [form  (apply str (rest (name form)))
+                                  parts (s/split form #"\.")]
+                              (if (= (count parts) 1)
+                                `(id ~table-name ~(keyword (first parts)))
+                                `(id ~@(map keyword parts))))
+                            form))
+                        body))
+
+(defmacro query
+  "Build a query, expands symbols like `$field` into calls to `id`.
+   Internally, this wraps `metabase.driver.query-processor.expand/query` and includes a call to `source-table`.
+   See the dox for `$->id` for more information on how `$`-prefixed expansion behaves.
+
+     (query venues
+       (ql/filter (ql/= $id 1)))
+
+      -> (ql/query
+           (ql/source-table (id :venues))
+           (ql/filter (ql/= (id :venues :id) 1)))"
+  {:style/indent 1}
+  [table & forms]
+  `(ql/query (ql/source-table (id ~(keyword table)))
+             ~@(map (partial $->id (keyword table)) forms)))
+
+(defmacro run-query
+  "Like `query`, but runs the query as well."
+  {:style/indent 1}
+  [table & forms]
+  `(ql/run-query* (query ~table ~@forms)))
+
+
 (defn format-name [nm]
   (i/format-name *data-loader* (name nm)))
 

test/metabase/test/util.clj

 (ns metabase.test.util
   "Helper functions and macros for writing unit tests."
-  (:require [expectations :refer :all]
+  (:require [cheshire.core :as json]
+            [expectations :refer :all]
             [medley.core :as m]
             (metabase [db :refer :all]
                       [util :as u])))
@@ -121,3 +122,11 @@
        (def ~(vary-meta fn-name assoc :private true) (ns-resolve '~namespc '~fn-name))
        ~(when (seq more)
           `(resolve-private-fns ~namespc ~(first more) ~@(rest more)))))
+
+(defn obj->json->obj
+  "Convert an object to JSON and back again. This can be done to ensure something will match its serialized + deserialized form,
+   e.g. keywords that aren't map keys:
+
+     (obj->json->obj {:type :query}) -> {:type \"query\"}"
+  [obj]
+  (json/parse-string (json/generate-string obj) keyword))