Merge pull request #440 from metabase/qp_order_by_aggregate_field

 allow ordering by aggregate fields in Generic SQL QP.

.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)

resources/frontend_client/app/query_builder/gui_query_editor.react.js

@@ -175,6 +175,12 @@ var GuiQueryEditor = React.createClass({
                     this.props.query.query.breakout[0] !== null);
     },
 
+    canSortByAggregateField: function() {
+        var SORTABLE_AGGREGATION_TYPES = new Set(["avg", "count", "distinct", "stddev", "sum"]);
+
+        return this.hasValidBreakout() && SORTABLE_AGGREGATION_TYPES.has(this.props.query.query.aggregation[0]);
+    },
+
     addDimension: function() {
         var query = this.props.query;
         query.query.breakout.push(null);
@@ -352,6 +358,13 @@ var GuiQueryEditor = React.createClass({
                 }
             }.bind(this));
 
+            if (this.canSortByAggregateField()) {
+                breakoutFieldList.push({
+                    id: ["aggregation",  0],
+                    name: this.props.query.query.aggregation[0] // e.g. "sum"
+                });
+            }
+
             return breakoutFieldList;
         } else {
             return [];

src/metabase/driver/generic_sql/query_processor.clj

@@ -176,12 +176,20 @@
   (when-not (empty? order-by-pairs)
     (->> 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)
-                        (string? asc-desc)]}
-                 `(order ~(field-id->kw field-id) ~(case asc-desc
-                                                     "ascending" :ASC
-                                                     "descending" :DESC)))))))
+         (mapv (fn [[field asc-desc]]
+                 {:pre [(string? asc-desc)]}
+                 `(order ~(match [field]
+                            [field-id :guard integer?] (field-id->kw field-id)
+                            [["aggregation" 0]]        (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.

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" 0] "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" 0] "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 :number, :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" 0] "ascending"]]})
+
+
+;;; ### order_by aggregate ["avg" field-id]
+(qp-expect-with-datasets #{:generic-sql}
+  {:columns [(format-name "price")
+             "avg"]
+   :rows [[3 22]
+          [2 28]
+          [1 32]
+          [4 53]]
+   :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" 0] "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" 0] "descending"]]})