Fix order by field refs (#44740)

* Fix order by field refs

* Update test to handle all drivers

* Untangle fix-order-by-field-refs

src/metabase/query_processor/middleware/add_implicit_clauses.clj

@@ -117,14 +117,49 @@
 ;;; |                                        Add Implicit Breakout Order Bys                                         |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
+(defn- fix-order-by-field-refs
+  "This function transforms top level integer field refs in order by to corresponding string field refs from breakout
+  if present.
+
+  ## Context
+  In current situation, ie. model as a source, then aggregation and breakout, and finally order by a breakout field,
+  [[metabase.lib.order-by/orderable-columns]] returns field ref with integer id, while reference to same field, but
+  with string id is present in breakout. Then, [[add-implicit-breakout-order-by]] adds the string ref to order by.
+
+  Resulting query would contain both references, while integral is transformed differently -- it contains no casting.
+  As that is not part of group by, the query would fail.
+
+  Reference: https://github.com/metabase/metabase/issues/44653."
+  [{:keys [breakout order-by] :as inner-query}]
+  (if (or (empty? breakout) (empty? order-by))
+    inner-query
+    (let [name->breakout (into {}
+                               (keep (fn [[tag id-or-name :as clause]]
+                                       (when (and (= :field tag)
+                                                  (string? id-or-name))
+                                         [id-or-name clause])))
+                               breakout)
+          ref->maybe-field-name (fn [[tag id-or-name]]
+                                  (when (and (= :field tag)
+                                             (integer? id-or-name))
+                                    ((some-fn :lib/desired-column-alias :name)
+                                     (lib.metadata/field (qp.store/metadata-provider)
+                                                         id-or-name))))
+          maybe-convert-order-by-ref (fn [[dir ref :as order-by-elm]]
+                                       (if-some [breakout (-> ref ref->maybe-field-name name->breakout)]
+                                         [dir breakout]
+                                         order-by-elm))]
+      (update inner-query :order-by (partial mapv maybe-convert-order-by-ref)))))
+
 (mu/defn ^:private add-implicit-breakout-order-by :- mbql.s/MBQLQuery
   "Fields specified in `breakout` should add an implicit ascending `order-by` subclause *unless* that Field is already
   *explicitly* referenced in `order-by`."
-  [{breakouts :breakout, :as inner-query} :- mbql.s/MBQLQuery]
+  [inner-query :- mbql.s/MBQLQuery]
   ;; Add a new [:asc <breakout-field>] clause for each breakout. The cool thing is `add-order-by-clause` will
   ;; automatically ignore new ones that are reference Fields already in the order-by clause
-  (reduce mbql.u/add-order-by-clause inner-query (for [breakout breakouts]
-                                                   [:asc breakout])))
+  (let [{breakouts :breakout, :as inner-query} (fix-order-by-field-refs inner-query)]
+    (reduce mbql.u/add-order-by-clause inner-query (for [breakout breakouts]
+                                                     [:asc breakout]))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

test/metabase/query_processor/middleware/add_implicit_clauses_test.clj

@@ -3,6 +3,7 @@
    [clojure.test :refer :all]
    [metabase.legacy-mbql.util :as mbql.u]
    [metabase.lib.metadata :as lib.metadata]
+   [metabase.lib.metadata.jvm :as lib.metadata.jvm]
    [metabase.lib.test-metadata :as meta]
    [metabase.lib.test-util :as lib.tu]
    [metabase.lib.test-util.macros :as lib.tu.macros]
@@ -12,7 +13,8 @@
    [metabase.query-processor.preprocess :as qp.preprocess]
    [metabase.query-processor.store :as qp.store]
    [metabase.query-processor.test-util :as qp.test-util]
-   [metabase.test :as mt]))
+   [metabase.test :as mt]
+   [toucan2.tools.with-temp :as t2.with-temp]))
 
 (deftest ^:parallel ordering-test
   (testing "check we fetch Fields in the right order"
@@ -37,58 +39,63 @@
 
 (deftest ^:parallel add-order-bys-for-breakouts-test
   (testing "we should add order-bys for breakout clauses"
-    (is (= {:source-table 1
-            :breakout     [[:field 1 nil]]
-            :order-by     [[:asc [:field 1 nil]]]}
-           (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
-            {:source-table 1
-             :breakout     [[:field 1 nil]]})))))
+    (mt/with-metadata-provider meta/metadata-provider
+      (is (= {:source-table 1
+              :breakout     [[:field 1 nil]]
+              :order-by     [[:asc [:field 1 nil]]]}
+             (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
+              {:source-table 1
+               :breakout     [[:field 1 nil]]}))))))
 
 (deftest ^:parallel add-order-bys-for-breakouts-test-2
   (testing "we should add order-bys for breakout clauses"
     (testing "Add Field to existing order-by"
-      (is (= {:source-table 1
-              :breakout     [[:field 2 nil]]
-              :order-by     [[:asc [:field 1 nil]]
-                             [:asc [:field 2 nil]]]}
-             (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
-              {:source-table 1
-               :breakout     [[:field 2 nil]]
-               :order-by     [[:asc [:field 1 nil]]]}))))))
+      (mt/with-metadata-provider meta/metadata-provider
+        (is (= {:source-table 1
+                :breakout     [[:field 2 nil]]
+                :order-by     [[:asc [:field 1 nil]]
+                               [:asc [:field 2 nil]]]}
+               (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
+                {:source-table 1
+                 :breakout     [[:field 2 nil]]
+                 :order-by     [[:asc [:field 1 nil]]]})))))))
 
 (deftest ^:parallel add-order-bys-for-breakouts-test-3
   (testing "we should add order-bys for breakout clauses"
     (testing "...but not if the Field is already in an order-by"
-      (is (= {:source-table 1
-              :breakout     [[:field 1 nil]]
-              :order-by     [[:asc [:field 1 nil]]]}
-             (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
-              {:source-table 1
-               :breakout     [[:field 1 nil]]
-               :order-by     [[:asc [:field 1 nil]]]}))))))
+      (mt/with-metadata-provider meta/metadata-provider
+        (is (= {:source-table 1
+                :breakout     [[:field 1 nil]]
+                :order-by     [[:asc [:field 1 nil]]]}
+               (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
+                {:source-table 1
+                 :breakout     [[:field 1 nil]]
+                 :order-by     [[:asc [:field 1 nil]]]})))))))
 
 (deftest ^:parallel add-order-bys-for-breakouts-test-4
   (testing "we should add order-bys for breakout clauses"
     (testing "...but not if the Field is already in an order-by"
-      (is (= {:source-table 1
-              :breakout     [[:field 1 nil]]
-              :order-by     [[:desc [:field 1 nil]]]}
-             (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
-              {:source-table 1
-               :breakout     [[:field 1 nil]]
-               :order-by     [[:desc [:field 1 nil]]]}))))))
+      (mt/with-metadata-provider meta/metadata-provider
+        (is (= {:source-table 1
+                :breakout     [[:field 1 nil]]
+                :order-by     [[:desc [:field 1 nil]]]}
+               (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
+                {:source-table 1
+                 :breakout     [[:field 1 nil]]
+                 :order-by     [[:desc [:field 1 nil]]]})))))))
 
 (deftest ^:parallel add-order-bys-for-breakouts-test-5
   (testing "we should add order-bys for breakout clauses"
     (testing "...but not if the Field is already in an order-by"
       (testing "With a datetime-field"
-        (is (= {:source-table 1
-                :breakout     [[:field 1 {:temporal-unit :day}]]
-                :order-by     [[:asc [:field 1 {:temporal-unit :day}]]]}
-               (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
-                {:source-table 1
-                 :breakout     [[:field 1 {:temporal-unit :day}]]
-                 :order-by     [[:asc [:field 1 {:temporal-unit :day}]]]})))))))
+        (mt/with-metadata-provider meta/metadata-provider
+          (is (= {:source-table 1
+                  :breakout     [[:field 1 {:temporal-unit :day}]]
+                  :order-by     [[:asc [:field 1 {:temporal-unit :day}]]]}
+                 (#'qp.add-implicit-clauses/add-implicit-breakout-order-by
+                  {:source-table 1
+                   :breakout     [[:field 1 {:temporal-unit :day}]]
+                   :order-by     [[:asc [:field 1 {:temporal-unit :day}]]]}))))))))
 
 (defn- add-implicit-fields [inner-query]
   (if (qp.store/initialized?)
@@ -307,3 +314,23 @@
                                        :condition    [:= $category-id &cat.*categories.id]}]
                        :order-by     [[:asc $name]]
                        :limit        3}))))))))
+
+(deftest ^:synchronized model-breakout-sort-querying-test
+  (mt/test-drivers
+   (mt/normal-drivers)
+   (testing "Query with sort, breakout and _model as a source_ works correctly (#44653)."
+     (t2.with-temp/with-temp [:model/Card {card-id :id} {:type :model
+                                                         :dataset_query (mt/mbql-query orders)}]
+       (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+             field-id (mt/id :products :created_at)
+             {:keys [base-type name]} (lib.metadata/field mp field-id)]
+         (is (= [1 19 37 64 79]
+                (->> (mt/run-mbql-query
+                      nil
+                      {:source-table (str "card__" card-id)
+                       :aggregation  [[:count]]
+                       :breakout     [[:field  name {:base-type base-type :temporal-unit :month}]]
+                       :order-by     [[:asc [:field field-id {:base-type base-type :temporal-unit :month}]]]
+                       :limit        5})
+                     mt/rows
+                     (mapv (comp int second))))))))))