diff --git a/src/metabase/query_processor/pivot.clj b/src/metabase/query_processor/pivot.clj index 5012656dbab410b5667f30df3657ae202f0618fe..4992056a364115fdb35682708346a00d85baa019 100644 --- a/src/metabase/query_processor/pivot.clj +++ b/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") diff --git a/test/metabase/query_processor/pivot_test.clj b/test/metabase/query_processor/pivot_test.clj index d78445cfc155b770cf2cfa4bb3494c409e66a186..22fc61bdb45c3bcd9c9017245ef7d913858806b5 100644 --- a/test/metabase/query_processor/pivot_test.clj +++ b/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)))))))))