only add implicit order_by if there is no explicit order_by

src/metabase/driver/generic_sql/query_processor.clj

@@ -16,21 +16,26 @@
 (declare apply-form
          log-query)
 
+(def ^{:dynamic true, :private true} *query*
+  "Query dictionary that we're currently processing"
+  nil)
+
 ;; ## Public Functions
 
 (defn process
   "Convert QUERY into a korma `select` form."
   [{{:keys [source_table] :as query} :query}]
   (when-not (zero? source_table)
-    (let [forms (->> (map apply-form query)                    ; call `apply-form` for each clause and strip out nil results
-                     (filter identity)
-                     (mapcat (fn [form] (if (vector? form) form ; some `apply-form` implementations return a vector of multiple korma forms; if only one was
-                                           [form])))           ; returned wrap it in a vec so `mapcat` can build a flattened sequence of forms
-                     doall)]
-      (when (config/config-bool :mb-db-logging)
-        (log-query query forms))
-      `(let [entity# (table-id->korma-entity ~source_table)]
-         (select entity# ~@forms)))))
+    (binding [*query* query]
+      (let [forms (->> (map apply-form query)                    ; call `apply-form` for each clause and strip out nil results
+                       (filter identity)
+                       (mapcat (fn [form] (if (vector? form) form ; some `apply-form` implementations return a vector of multiple korma forms; if only one was
+                                             [form])))           ; returned wrap it in a vec so `mapcat` can build a flattened sequence of forms
+                       doall)]
+        (when (config/config-bool :mb-db-logging)
+          (log-query query forms))
+        `(let [entity# (table-id->korma-entity ~source_table)]
+           (select entity# ~@forms))))))
 
 
 (defn process-structured
@@ -92,10 +97,17 @@
   (match field-ids
     []    nil ; empty clause
     [nil] nil ; empty clause
-    _     (let [field-names (map field-id->kw field-ids)]
+    _     (let [field-names (map field-id->kw field-ids)
+                order-by-field-names (some->> (:order_by *query*) ; get set of names of all fields specified in `order_by`
+                                              (map first)
+                                              (map field-id->kw)
+                                              set)]
             `[(group  ~@field-names)
               (fields ~@field-names)
-              (order  ~@field-names :ASC)])))
+              ~@(->> field-names                                                    ; Add an implicit `order :ASC` clause for every field specified in `breakout`
+                     (filter (complement (partial contains? order-by-field-names))) ; that is *not* specified *explicitly* in `order_by`.
+                     (map (fn [field-name]
+                            `(order ~field-name :ASC))))])))
 
 ;; ### `:fields`
 ;; ex.

test/metabase/driver/generic_sql/query_processor_test.clj

@@ -332,7 +332,25 @@
                                   :order_by [[(field->id :checkins :user_id) "ascending"]]
                                   :limit nil}}))
 
-;; ## "BREAKOUT" - MULTIPLE COLUMNS
+;; ## "BREAKOUT" - MULTIPLE COLUMNS W/ IMPLICT "ORDER_BY"
+;; Fields should be implicitly ordered :ASC for all the fields in `breakout` that are not specified in `order_by`
+(expect {:status :completed,
+         :row_count 10,
+         :data {:rows [[1 1 1] [5 1 1] [7 1 1] [10 1 1] [13 1 1] [16 1 1] [26 1 1] [31 1 1] [35 1 1] [36 1 1]],
+                :columns ["VENUE_ID" "USER_ID" "count"],
+                :cols [{:special_type "fk", :base_type "IntegerField", :description nil, :name "VENUE_ID", :table_id (table->id :checkins), :id (field->id :checkins :venue_id)}
+                       {:special_type "fk", :base_type "IntegerField", :description nil, :name "USER_ID", :table_id (table->id :checkins), :id (field->id :checkins :user_id)}
+                       {:base_type "IntegerField", :special_type "number", :name "count", :id nil, :table_id nil, :description nil}]}}
+        (process-and-run {:type :query
+                          :database @db-id
+                          :query {:source_table (table->id :checkins)
+                                  :limit 10
+                                  :aggregation ["count"]
+                                  :breakout [(field->id :checkins :user_id)
+                                             (field->id :checkins :venue_id)]}}))
+
+;; ## "BREAKOUT" - MULTIPLE COLUMNS W/ EXPLICIT "ORDER_BY"
+;; `breakout` should not implicitly order by any fields specified in `order_by`
 (expect {:status :completed,
          :row_count 10,
          :data {:rows [[2 15 1] [3 15 1] [7 15 1] [14 15 1] [16 15 1] [18 15 1] [22 15 1] [23 15 2] [24 15 1] [27 15 1]],