Add support for sorting by an externally remapped column

When an MBQL query has an order by for a column that has been
remapped via FK, it will swap the order by for the original field for
the remapped field. Previously this would order by the original source
column and apply that to the remapping which would be unexpected.

src/metabase/query_processor/middleware/add_dimension_projections.clj

@@ -73,21 +73,42 @@
        :new-column (create-remapped-col remap-to remap-from)
        :dimension-type remap-type})))
 
+(defn- create-remap-col-pairs
+  "Return pairs of field id and the new remapped column that the field
+  should be remapped to. This is a list of pairs as we want to
+  preserve order"
+  [fields]
+  (for [{{:keys [field-id human-readable-field-id dimension-type dimension-name]} :dimensions,
+         field-name :field-name, source-field-id :field-id} fields
+        :when (= :external dimension-type)]
+    [source-field-id (create-fk-remap-col field-id
+                                          human-readable-field-id
+                                          field-name
+                                          dimension-name)]))
+
+(defn- update-remapped-order-by
+  "Order by clauses that include an external remapped column should be
+  replace that original column in the order by with the newly remapped
+  column. This should order by the text of the remapped column vs. the
+  id of the source column before the remapping"
+  [remap-cols-by-id order-by-seq]
+  (mapv (fn [{{:keys [field-id]} :field :as order-by-clause}]
+          (if-let [remapped-col (get remap-cols-by-id field-id)]
+            (assoc order-by-clause :field remapped-col)
+            order-by-clause))
+        order-by-seq))
+
 (defn- add-fk-remaps
   "Function that will include FK references needed for external
   remappings. This will then flow through to the resolver to get the
   new tables included in the join."
   [query]
-  (update-in query [:query :fields]
-             (fn [fields]
-               (concat fields
-                       (for [{{:keys [field-id human-readable-field-id dimension-type dimension-name]} :dimensions,
-                              :keys [field-name]} fields
-                             :when (= :external dimension-type)]
-                         (create-fk-remap-col field-id
-                                              human-readable-field-id
-                                              field-name
-                                              dimension-name))))))
+  (let [remap-col-pairs (create-remap-col-pairs (get-in query [:query :fields]))]
+    (if (seq remap-col-pairs)
+      (-> query
+          (update-in [:query :order-by] #(update-remapped-order-by (into {} remap-col-pairs) %))
+          (update-in [:query :fields] concat (map second remap-col-pairs)))
+      query)))
 
 (defn- remap-results
   "Munges results for remapping after the query has been executed. For

test/metabase/query_processor/middleware/add_dimension_projections_test.clj

@@ -107,7 +107,8 @@
 
 (def ^:private example-query
   {:query
-   {:fields
+   {:order-by []
+    :fields
     (mapv #(merge field-defaults %)
           [{:description "A unique internal identifier for the review. Should not be used externally.",
             :base-type :type/BigInteger,
@@ -155,6 +156,24 @@
                                             :field-display-name "Product"}))
   (add-fk-remaps example-query))
 
+(expect
+  (-> example-query
+      (assoc-in [:query :order-by] [{:direction :ascending
+                                     :field (i/map->FieldPlaceholder {:fk-field-id 32
+                                                                      :field-id 27
+                                                                      :remapped-from "PRODUCT_ID"
+                                                                      :remapped-to nil
+                                                                      :field-display-name "Product"})}])
+      (update-in [:query :fields]
+                 conj (i/map->FieldPlaceholder {:fk-field-id 32
+                                                :field-id 27
+                                                :remapped-from "PRODUCT_ID"
+                                                :remapped-to nil
+                                                :field-display-name "Product"})))
+  (-> example-query
+      (assoc-in [:query :order-by] [{:direction :ascending :field {:field-id 32}}])
+      add-fk-remaps))
+
 (def ^:private external-remapped-result
   (-> example-resultset
       (update :cols conj {:description "The name of the product as it should be displayed to customers.",

test/metabase/query_processor_test/breakout_test.clj

@@ -111,3 +111,23 @@
            (ql/limit 5))
          booleanize-native-form
          (format-rows-by [int int str]))))
+
+(expect-with-non-timeseries-dbs
+  [["Wine Bar" "Thai" "Thai" "Thai" "Thai" "Steakhouse" "Steakhouse" "Steakhouse" "Steakhouse" "Southern"]
+   ["American" "American" "American" "American" "American" "American" "American" "American" "Artisan" "Artisan"]]
+  (data/with-data
+    (fn []
+      [(db/insert! Dimension {:field_id (data/id :venues :category_id)
+                              :name "Foo"
+                              :type :external
+                              :human_readable_field_id (data/id :categories :name)})])
+    [(->> (data/run-query venues
+             (ql/order-by (ql/desc $category_id))
+             (ql/limit 10))
+           rows
+           (map last))
+     (->> (data/run-query venues
+             (ql/order-by (ql/asc $category_id))
+             (ql/limit 10))
+           rows
+           (map last))]))