From 12f178e857b52311a11f8a470780b30352550611 Mon Sep 17 00:00:00 2001
From: Cam Saul <cam@getluckybird.com>
Date: Thu, 28 May 2015 17:23:04 -0700
Subject: [PATCH] :smiling_imp: allow ordering by aggregate fields in generic
SQL QP.
---
.dir-locals.el | 4 +-
.../driver/generic_sql/query_processor.clj | 17 ++-
test/metabase/driver/query_processor_test.clj | 106 +++++++++++++++++-
3 files changed, 118 insertions(+), 9 deletions(-)
diff --git a/.dir-locals.el b/.dir-locals.el
index adf2f4927b2..430e49a10af 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -22,6 +22,8 @@
(expect-when-testing-against-dataset 1)
(expect-when-testing-mongo 1)
(expect-with-all-drivers 1)
+ (expect-with-dataset 1)
+ (expect-with-datasets 1)
(ins 1)
(let-400 1)
(let-404 1)
@@ -31,7 +33,7 @@
(macrolet 1)
(org-perms-case 1)
(pdoseq 1)
- (qp-expect-with-all-drivers 1)
+ (qp-expect-with-datasets 1)
(resolve-private-fns 1)
(symbol-macrolet 1)
(sync-in-context 2)
diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj
index 65a48e470c5..ed2ec072873 100644
--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -177,11 +177,20 @@
(->> order-by-pairs
(map (fn [pair] (when-not (vector? pair) (throw (Exception. "order_by clause must consists of pairs like [field_id \"ascending\"]"))) pair))
(mapv (fn [[field-id asc-desc]]
- {:pre [(integer? field-id)
+ {:pre [(or (integer? field-id)
+ (= field-id "$$aggregation"))
(string? asc-desc)]}
- `(order ~(field-id->kw field-id) ~(case asc-desc
- "ascending" :ASC
- "descending" :DESC)))))))
+ `(order ~(if (integer? field-id) (field-id->kw field-id)
+ (let [[ag] (:aggregation (:query qp/*query*))]
+ `(raw ~(case ag
+ "avg" "\"avg\"" ; based on the type of the aggregation
+ "count" "\"count\"" ; make sure we ask the DB to order by the
+ "distinct" "\"count\"" ; name of the aggregate field
+ "stddev" "\"stddev\""
+ "sum" "\"sum\""))))
+ ~(case asc-desc
+ "ascending" :ASC
+ "descending" :DESC)))))))
;; ### `:page`
;; ex.
diff --git a/test/metabase/driver/query_processor_test.clj b/test/metabase/driver/query_processor_test.clj
index d6b7205b8eb..00ada5fb23a 100644
--- a/test/metabase/driver/query_processor_test.clj
+++ b/test/metabase/driver/query_processor_test.clj
@@ -5,7 +5,7 @@
[metabase.driver :as driver]
[metabase.driver.query-processor :refer :all]
(metabase.models [table :refer [Table]])
- [metabase.test.data.datasets :as datasets :refer [*dataset* expect-with-all-datasets]]))
+ [metabase.test.data.datasets :as datasets :refer [*dataset*]]))
;; ## Dataset-Independent Data Fns
@@ -42,10 +42,26 @@
;; ### Helper Fns + Macros
+(defmacro qp-expect-with-datasets
+ "Slightly more succinct way of writing QP tests. Adds standard boilerplate to run QP tests against DATASETS."
+ [datasets {:keys [rows] :as data} query]
+ {:pre [(set? datasets)
+ (map? data)
+ (sequential? rows)
+ (map? query)]}
+ `(datasets/expect-with-datasets ~datasets
+ {:status :completed
+ :row_count ~(count rows)
+ :data ~data}
+ (driver/process-query
+ {:type :query
+ :database (db-id)
+ :query ~query})))
+
(defmacro qp-expect-with-all-datasets
- "Slightly more succinct way of writing QP tests. Adds standard boilerplate to actual/expected forms."
+ "Like `qp-expect-with-datasets`, but tests against *all* datasets."
[data query]
- `(expect-with-all-datasets
+ `(datasets/expect-with-all-datasets
{:status :completed
:row_count ~(count (:rows data))
:data ~data}
@@ -53,6 +69,7 @@
:database (db-id)
:query ~query})))
+
(defn ->columns
"Generate the vector that should go in the `columns` part of a QP result; done by calling `format-name` against each column name."
[& names]
@@ -516,7 +533,7 @@
;; ## EMPTY QUERY
;; Just don't barf
-(expect-with-all-datasets
+(datasets/expect-with-all-datasets
{:status :completed
:row_count 0
:data {:rows [], :columns [], :cols []}}
@@ -608,3 +625,84 @@
{:source_table (id :venues)
:breakout [(id :venues :price)]
:aggregation ["cum_sum" (id :venues :id)]})
+
+
+;;; ## order_by aggregate fields (SQL-only for the time being)
+
+;;; ### order_by aggregate ["count"]
+(qp-expect-with-datasets #{:generic-sql}
+ {:columns [(format-name "price")
+ "count"]
+ :rows [[4 6]
+ [3 13]
+ [1 22]
+ [2 59]]
+ :cols [(venue-col :price)
+ {:base_type :IntegerField, :special_type :number, :description nil, :table_id nil, :name "count", :id nil}]}
+ {:source_table (id :venues)
+ :aggregation ["count"]
+ :breakout [(id :venues :price)]
+ :order_by [["$$aggregation" "ascending"]]})
+
+
+;;; ### order_by aggregate ["sum" field-id]
+(qp-expect-with-datasets #{:generic-nsql}
+ {:columns [(format-name "price")
+ "sum"]
+ :rows [[2 (->sum-type 2855)]
+ [1 (->sum-type 1211)]
+ [3 (->sum-type 615)]
+ [4 (->sum-type 369)]]
+ :cols [(venue-col :price)
+ {:base_type (id-field-type), :special_type :id, :name "sum", :id nil, :table_id nil, :description nil}]}
+ {:source_table (id :venues)
+ :aggregation ["sum" (id :venues :id)]
+ :breakout [(id :venues :price)]
+ :order_by [["$$aggregation" "descending"]]})
+
+
+;;; ### order_by aggregate ["distinct" field-id]
+(qp-expect-with-datasets #{:generic-sql}
+ {:columns [(format-name "price")
+ "count"]
+ :rows [[4 6]
+ [3 13]
+ [1 22]
+ [2 59]]
+ :cols [(venue-col :price)
+ {:base_type :IntegerField, :special_type :id, :name "count", :id nil, :table_id nil, :description nil}]}
+ {:source_table (id :venues)
+ :aggregation ["distinct" (id :venues :id)]
+ :breakout [(id :venues :price)]
+ :order_by [["$$aggregation" "ascending"]]})
+
+
+;;; ### order_by aggregate ["avg" field-id]
+(qp-expect-with-datasets #{:generic-sql}
+ {:columns [(format-name "price")
+ "avg"]
+ :rows [[4 53]
+ [1 32]
+ [2 28]
+ [3 22]]
+ :cols [(venue-col :price)
+ {:base_type :IntegerField, :special_type :fk, :name "avg", :id nil, :table_id nil, :description nil}]}
+ {:source_table (id :venues)
+ :aggregation ["avg" (id :venues :category_id)]
+ :breakout [(id :venues :price)]
+ :order_by [["$$aggregation" "ascending"]]})
+
+;;; ### order_by aggregate ["stddev" field-id]
+(qp-expect-with-datasets #{:generic-sql}
+ {:columns [(format-name "price")
+ "stddev"]
+ :rows [[3 26.19160170741759]
+ [1 24.112111881665186]
+ [2 21.418692164795292]
+ [4 14.788509052639485]]
+ :cols [(venue-col :price)
+ {:base_type :IntegerField, :special_type :fk, :name "stddev", :id nil, :table_id nil, :description nil}]}
+ {:source_table (id :venues)
+ :aggregation ["stddev" (id :venues :category_id)]
+ :breakout [(id :venues :price)]
+ :order_by [["$$aggregation" "descending"]]})
--
GitLab