Fix pivots that specify an aggregation `:order-by` (#28545)

Fixes #22872

In #21839, all order-bys were removed from pivot queries because columns
in `ORDER BY` had to appear in `GROUP BY` clauses. Unfortunately that
removed legal order by columns (e.g. count) from being applied.

This now keeps aggregation columns in field defs to restore the sorting
behaviour.

src/metabase/query_processor/pivot.clj

@@ -2,7 +2,6 @@
   "Pivot table actions for the query processor"
   (:require
    [clojure.core.async :as a]
-   [medley.core :as m]
    [metabase.mbql.normalize :as mbql.normalize]
    [metabase.query-processor :as qp]
    [metabase.query-processor.context :as qp.context]
@@ -104,12 +103,19 @@
       (log/tracef "Added pivot-grouping expression to query\n%s" (u/pprint-to-str 'yellow query))
       query)))
 
-(defn- remove-existing-order-bys
-  "Remove existing `:order-by` clauses from the query. Since we're adding our own breakouts (i.e. `GROUP BY` and `ORDER
-  BY` clauses) to do the pivot table stuff, existing `:order-by` clauses probably won't work -- `ORDER BY` isn't
-  allowed for fields that don't appear in `GROUP BY`."
+(defn- remove-non-aggregation-order-bys
+  "Only keep existing aggregations in `:order-by` clauses from the query.
+   Since we're adding our own breakouts (i.e. `GROUP BY` and `ORDER BY` clauses)
+   to do the pivot table stuff, existing `:order-by` clauses probably won't work
+   -- `ORDER BY` isn't allowed for fields that don't appear in `GROUP BY`."
   [outer-query]
-  (m/dissoc-in outer-query [:query :order-by]))
+  (update
+    outer-query
+    :query
+    (fn [query]
+      (if-let [new-order-by (not-empty (filterv (comp #(= :aggregation %) first second) (:order-by query)))]
+        (assoc query :order-by new-order-by)
+        (dissoc query :order-by)))))
 
 (defn- generate-queries
   "Generate the additional queries to perform a generic pivot table"
@@ -121,7 +127,7 @@
                             new-breakouts (for [i breakout-indices]
                                             (nth all-breakouts i))]]
       (-> outer-query
-          remove-existing-order-bys
+          remove-non-aggregation-order-bys
           (add-grouping-field new-breakouts group-bitmask)))
     (catch Throwable e
       (throw (ex-info (tru "Error generating pivot queries")

test/metabase/query_processor/pivot_test.clj

@@ -321,3 +321,33 @@
                 [nil 1 200]]
                (mt/rows
                 (qp.pivot/run-pivot-query query))))))))
+
+(deftest pivot-with-order-by-metric-test
+  (testing "Pivot queries should allow ordering by aggregation (#22872)"
+    (mt/dataset sample-dataset
+      (let  [query (mt/mbql-query reviews
+                     {:breakout [$rating [:field (mt/id :reviews :created_at) {:temporal-unit :year}]]
+                      :aggregation [[:count]]
+                      :order-by [[:asc [:aggregation 0 nil]]]
+                      :filter [:between $created_at "2019-01-01" "2021-01-01"]})]
+        (mt/with-native-query-testing-context query
+          (is (= [[1 "2020-01-01T00:00:00Z" 0 5]
+                  [2 "2020-01-01T00:00:00Z" 0 13]
+                  [3 "2020-01-01T00:00:00Z" 0 14]
+                  [1 "2019-01-01T00:00:00Z" 0 15]
+                  [3 "2019-01-01T00:00:00Z" 0 29]
+                  [2 "2019-01-01T00:00:00Z" 0 35]
+                  [5 "2020-01-01T00:00:00Z" 0 45]
+                  [4 "2020-01-01T00:00:00Z" 0 78]
+                  [5 "2019-01-01T00:00:00Z" 0 137]
+                  [4 "2019-01-01T00:00:00Z" 0 236]
+                  [nil "2020-01-01T00:00:00Z" 1 155]
+                  [nil "2019-01-01T00:00:00Z" 1 452]
+                  [1 nil 2 20]
+                  [3 nil 2 43]
+                  [2 nil 2 48]
+                  [5 nil 2 182]
+                  [4 nil 2 314]
+                  [nil nil 3 607]]
+                 (mt/rows
+                   (qp.pivot/run-pivot-query query)))))))))