ORDER DAT SHIT THE RIGHT WAY KTHANX <3

src/metabase/driver/generic_sql/query_processor/annotate.clj

@@ -25,46 +25,10 @@
             :columns column-names
             :cols (get-column-info query column-names)}}))
 
-(defn- -order-columns
-  "Don't use this directly; use `order-columns`.
-
-   This broken out for testability -- it doesn't depend on data from the DB."
-  [fields breakout-field-ids castified-field-names]
-  ;; Basically we want to convert both BREAKOUT-FIELD-IDS and CASTIFIED-FIELD-NAMES to maps like:
-  ;;   {:name      "updated_at"
-  ;;    :id        224
-  ;;    :castified (keyword "CAST(updated_at AS DATE)")
-  ;;    :position  21}
-  ;; Then we can order things appropriately and return the castified names.
-  (let [uncastified->castified (zipmap (map #(uncastify (name %)) castified-field-names) castified-field-names)
-        fields                 (map #(assoc % :castified (uncastified->castified (:name %)))
-                                    fields)
-        id->field              (zipmap (map :id fields) fields)
-        castified->field       (zipmap (map :castified fields) fields)
-        breakout-fields        (->> breakout-field-ids
-                                    (map id->field))
-        other-fields           (->> castified-field-names
-                                    (map (fn [castified-name]
-                                           (or (castified->field castified-name)
-                                               {:castified castified-name             ; for aggregate fields like 'count' create a fake map
-                                                :position 0})))                       ; with position 0 so it is returned ahead of the other fields
-                                    (filter #(not (contains? (set breakout-field-ids)
-                                                             (:id %))))
-                                    (sort-by :position))]
-    (->> (concat breakout-fields other-fields)
-         (map :castified))))
-
 (defn- order-columns
-  "Return CASTIFIED-FIELD-NAMES in the order we'd like to display them in the output.
-   They should be ordered as follows:
-
-   1.  All breakout fields, in the same order as BREAKOUT-FIELD-IDS
-   2.  Any aggregate fields like `count`
-   3.  All other columns in the same order as `Field.position`."
-  [{{source-table :source_table breakout-field-ids :breakout} :query} castified-field-names]
-  (-order-columns (sel :many :fields [Field :id :name :position] :table_id source-table)
-                  (filter identity breakout-field-ids)                                   ; handle empty breakout clauses like [nil]
-                  castified-field-names))
+  [query castified-field-names]
+  (binding [qp/*uncastify-fn* uncastify]
+    (qp/order-columns query castified-field-names)))
 
 (defn- get-column-names
   "Get an ordered seqences of column names for the results.
@@ -84,6 +48,7 @@
                           (filter #(not (contains? (set fields-clause-fields) %)))
                           (order-columns query))]
     (->> (concat fields-clause-fields other-fields)                                   ; Return a combined vector. Convert them to strs, otherwise korma
+         (filter identity)                                                            ; remove any nils -- don't want a NullPointerException
          (map name))))                                                                ; will qualify them like `"METABASE_FIELD"."FOLLOWERS_COUNT"
 
 (defn- uncastify

src/metabase/driver/mongo/query_processor.clj

@@ -148,7 +148,7 @@
 (defn annotate-results [{:keys [source_table] :as query} results]
   {:pre [(integer? source_table)]}
   (let [field-name->field (sel :many :field->obj [Field :name] :table_id source_table)
-        column-keys       (keys (first results))
+        column-keys       (qp/order-columns {:query query} (keys (first results)))
         column-names      (map name column-keys)]
     {:row_count (count results)
      :status :completed

src/metabase/driver/query_processor.clj

@@ -70,10 +70,10 @@
 (defn get-column-info
   "Get extra information about result columns. This is done by looking up matching `Fields` for the `Table` in QUERY or looking up
    information about special columns such as `count` via `get-special-column-info`."
-  [query column-names]
-  {:pre [(:query query)]}
-  (let [table-id (get-in query [:query :source_table])
-        columns (->> (sel :many [Field :id :table_id :name :description :base_type :special_type] ; lookup columns with matching names for this Table
+  [{{table-id :source_table} :query, :as query} column-names]
+  {:pre [(integer? table-id)
+         (every? string? column-names)]}
+  (let [columns (->> (sel :many [Field :id :table_id :name :description :base_type :special_type] ; lookup columns with matching names for this Table
                           :table_id table-id :name [in (set column-names)])
                      (map (fn [{:keys [name] :as column}]                                         ; build map of column-name -> column
                             {name (-> (select-keys column [:id :table_id :name :description :base_type :special_type])
@@ -95,10 +95,67 @@
           :id nil
           :table_id nil
           :description nil}
-         (let [aggregation-type  (keyword column-name)                               ; For aggregations of a specific Field (e.g. `sum`)
+         (let [aggregation-type   (keyword column-name)                              ; For aggregations of a specific Field (e.g. `sum`)
                field-aggregation? (contains? #{:avg :stddev :sum} aggregation-type)] ; lookup the field we're aggregating and return its
            (if field-aggregation? (sel :one :fields [Field :base_type :special_type] ; type info. (The type info of the aggregate result
                                        :id (-> query :query :aggregation second))    ; will be the same.)
                (case aggregation-type                                                ; Otherwise for general aggregations such as `count`
                  :count {:base_type :IntegerField                                    ; just return hardcoded type info
                          :special_type :number})))))
+
+(def ^:dynamic *uncastify-fn* identity) ; default is no-op
+
+;; TODO - since this was moved over from generic SQL some of its functionality should be reworked. And dox updated.
+;; (Since castification is basically SQL-specific it would make sense to handle castification / decastification separately)
+;; Fix this when it's not a Friday
+
+(defn -order-columns
+  "Don't use this directly; use `order-columns`.
+
+   This broken out for testability -- it doesn't depend on data from the DB."
+  [fields breakout-field-ids field-field-ids castified-field-names]
+  ;; Basically we want to convert both BREAKOUT-FIELD-IDS and CASTIFIED-FIELD-NAMES to maps like:
+  ;;   {:name      "updated_at"
+  ;;    :id        224
+  ;;    :castified (keyword "CAST(updated_at AS DATE)")
+  ;;    :position  21}
+  ;; Then we can order things appropriately and return the castified names.
+  (let [uncastified->castified (zipmap (map #(*uncastify-fn* (name %)) castified-field-names) castified-field-names)
+        fields                 (map #(assoc % :castified (uncastified->castified (:name %)))
+                                    fields)
+        id->field              (zipmap (map :id fields) fields)
+        castified->field       (zipmap (map :castified fields) fields)
+        breakout-fields        (->> breakout-field-ids
+                                    (map id->field))
+        field-fields           (->> field-field-ids
+                                    (map id->field))
+        other-fields           (->> castified-field-names
+                                    (map (fn [castified-name]
+                                           (or (castified->field castified-name)
+                                               {:castified castified-name             ; for aggregate fields like 'count' create a fake map
+                                                :position 0})))                       ; with position 0 so it is returned ahead of the other fields
+                                    (filter #(not (or (contains? (set breakout-field-ids)
+                                                                 (:id %))
+                                                      (contains? (set field-field-ids)
+                                                                 (:id %)))))
+                                    (sort-by :position))]
+    (->> (concat breakout-fields field-fields other-fields)
+         (map :castified))))
+
+(defn order-columns
+  "Return CASTIFIED-FIELD-NAMES in the order we'd like to display them in the output.
+   They should be ordered as follows:
+
+   1.  All breakout fields, in the same order as BREAKOUT-FIELD-IDS
+   2.  Any aggregate fields like `count`
+   3.  Fields included in the `fields` clause
+   4.  All other columns in the same order as `Field.position`."
+  [{{source-table :source_table, breakout-field-ids :breakout, field-field-ids :fields} :query} castified-field-names]
+  (try
+    (-order-columns (sel :many :fields [Field :id :name :position] :table_id source-table)
+                    breakout-field-ids
+                    field-field-ids
+                    castified-field-names)
+    (catch Exception e
+      (.printStackTrace e)
+      (println (.getMessage e)))))

test/metabase/driver/generic_sql/query_processor/annotate_test.clj

 (ns metabase.driver.generic-sql.query-processor.annotate-test
   (:require [expectations :refer :all]
             [metabase.driver.generic-sql.query-processor.annotate :refer :all]
+            [metabase.driver.query-processor :as qp]
             [metabase.test.util :refer [resolve-private-fns]]))
 
-(resolve-private-fns metabase.driver.generic-sql.query-processor.annotate -order-columns)
+(resolve-private-fns metabase.driver.generic-sql.query-processor.annotate uncastify)
+
+(defn -order-columns [& args]
+  (binding [qp/*uncastify-fn* uncastify]
+    (apply qp/-order-columns args)))
 
 ;; ## TESTS FOR -ORDER-COLUMNS
 
@@ -42,7 +47,7 @@
          :description
          :external_url
          (keyword "CAST(updated_at AS DATE)")]
-  (-order-columns mock-fields [] mock-castified-field-names))
+  (-order-columns mock-fields [] []  mock-castified-field-names))
 
 ;; Check that breakout fields are returned first, in order, before other fields
 (expect [:description
@@ -55,7 +60,7 @@
          (keyword "CAST(created_at AS DATE)")
          :external_url
          (keyword "CAST(updated_at AS DATE)")]
-  (-order-columns mock-fields [375 433] mock-castified-field-names))
+  (-order-columns mock-fields [375 433] [] mock-castified-field-names))
 
 ;; Check that aggregate fields are returned ahead of other fields
 (expect [:allows_suggestions
@@ -69,4 +74,4 @@
          (keyword "CAST(created_at AS DATE)")
          :external_url
          (keyword "CAST(updated_at AS DATE)")]
-  (-order-columns mock-fields [433 375] (concat [:count] mock-castified-field-names)))
+  (-order-columns mock-fields [433 375] []  (concat [:count] mock-castified-field-names)))

test/metabase/driver/generic_sql/query_processor_test.clj

@@ -82,30 +82,9 @@
                                  :limit 10
                                  :order_by [[(field->id :venues :id) "ascending"]]}}))
 
-;; ## "ORDER_BY" CLAUSE
-;; Test that we can tell the Query Processor to return results ordered by multiple fields
-(expect {:status :completed,
-         :row_count 10,
-         :data {:rows [[1 12 375] [1 9 139] [1 1 72] [2 15 129] [2 12 471] [2 11 325] [2 9 590] [2 9 833] [2 8 380] [2 5 719]],
-                :columns ["VENUE_ID" "USER_ID" "ID"],
-                :cols [{:extra_info {:target_table_id (table->id :venues)} :special_type :fk, :base_type :IntegerField, :description nil, :name "VENUE_ID", :table_id (table->id :checkins), :id (field->id :checkins :venue_id)}
-                       {:extra_info {:target_table_id (table->id :users)} :special_type :fk, :base_type :IntegerField, :description nil, :name "USER_ID", :table_id (table->id :checkins), :id (field->id :checkins :user_id)}
-                       {:extra_info {} :special_type :id, :base_type :BigIntegerField, :description nil, :name "ID", :table_id (table->id :checkins), :id (field->id :checkins :id)}]}}
-  (driver/process-query {:type :query
-                         :database @db-id
-                         :query {:source_table (table->id :checkins)
-                                 :aggregation ["rows"]
-                                 :limit 10
-                                 :fields [(field->id :checkins :venue_id)
-                                          (field->id :checkins :user_id)
-                                          (field->id :checkins :id)]
-                                 :order_by [[(field->id :checkins :venue_id) "ascending"]
-                                            [(field->id :checkins :user_id) "descending"]
-                                            [(field->id :checkins :id) "ascending"]]}}))
 
 ;; ## "FILTER" CLAUSE
 
-
 ;; ### FILTER -- "AND", ">", ">="
 (expect {:status :completed,
          :row_count 5,

test/metabase/driver/query_processor_test.clj

@@ -190,6 +190,45 @@
     :table_id (->table-id :venues)
     :id (->field-id :venues :name)}])
 
+(defn ->columns [& names]
+  (mapv (partial format-name *driver-data*)
+        names))
+
+(defn categories-col [col]
+  (case col
+    :id   {:extra_info {} :special_type :id, :base_type (id-field-type *driver-data*), :description nil, :name (format-name *driver-data* "id")
+           :table_id (->table-id :categories), :id (->field-id :categories :id)}
+    :name {:extra_info {} :special_type nil, :base_type :TextField, :description nil, :name (format-name *driver-data* "name")
+           :table_id (->table-id :categories), :id (->field-id :categories :name)}))
+
+(defn checkins-col [col]
+  (case col
+    :id       {:extra_info {}
+               :special_type :id
+               :base_type (id-field-type *driver-data*)
+               :description nil
+               :name (format-name *driver-data* "id")
+               :table_id (->table-id :checkins)
+               :id (->field-id :checkins :id)}
+    :venue_id {:extra_info (if (fks-supported? *driver-data*) {:target_table_id (->table-id :venues)}
+                               {})
+               :special_type (when (fks-supported? *driver-data*)
+                               :fk)
+               :base_type :IntegerField
+               :description nil
+               :name (format-name *driver-data* "venue_id")
+               :table_id (->table-id :checkins)
+               :id (->field-id :checkins :venue_id)}
+    :user_id  {:extra_info (if (fks-supported? *driver-data*) {:target_table_id (->table-id :users)}
+                               {})
+               :special_type (if (fks-supported? *driver-data*) :fk
+                                 :category)
+               :base_type :IntegerField
+               :description nil
+               :name (format-name *driver-data* "user_id")
+               :table_id (->table-id :checkins)
+               :id (->field-id :checkins :user_id)}))
+
 ;; ### "COUNT" AGGREGATION
 (qp-expect-with-all-drivers
     {:rows [[100]]
@@ -272,7 +311,7 @@
             [3 "Artisan"]
             [4 "Asian"]
             [5 "BBQ"]]
-     :columns [(format-name *driver-data* "id"), (format-name *driver-data* "name")]
+     :columns (->columns "id" "name")
      :cols [{:extra_info {} :special_type :id, :base_type (id-field-type *driver-data*), :description nil, :name (format-name *driver-data* "id")
              :table_id (->table-id :categories), :id (->field-id :categories :id)}
             {:extra_info {} :special_type nil, :base_type :TextField, :description nil, :name (format-name *driver-data* "name")
@@ -290,13 +329,30 @@
             [8 "Beer Garden"]
             [9 "Breakfast / Brunch"]
             [10 "Brewery"]]
-     :columns [(format-name *driver-data* "id"), (format-name *driver-data* "name")]
-     :cols [{:extra_info {} :special_type :id, :base_type (id-field-type *driver-data*), :description nil, :name (format-name *driver-data* "id")
-             :table_id (->table-id :categories), :id (->field-id :categories :id)}
-            {:extra_info {} :special_type nil, :base_type :TextField, :description nil, :name (format-name *driver-data* "name")
-             :table_id (->table-id :categories), :id (->field-id :categories :name)}]}
+     :columns (->columns "id" "name")
+     :cols [(categories-col :id)
+            (categories-col :name)]}
   {:source_table (->table-id :categories)
    :aggregation ["rows"]
    :page {:items 5
           :page 2}
    :order_by [[(->field-id :categories :name) "ascending"]]})
+
+
+;; ## "ORDER_BY" CLAUSE
+;; Test that we can tell the Query Processor to return results ordered by multiple fields
+(qp-expect-with-all-drivers
+    {:rows [[1 12 375] [1 9 139] [1 1 72] [2 15 129] [2 12 471] [2 11 325] [2 9 590] [2 9 833] [2 8 380] [2 5 719]],
+     :columns (->columns "venue_id" "user_id" "id")
+     :cols [(checkins-col :venue_id)
+            (checkins-col :user_id)
+            (checkins-col :id)]}
+  {:source_table (->table-id :checkins)
+   :aggregation ["rows"]
+   :limit 10
+   :fields [(->field-id :checkins :venue_id)
+            (->field-id :checkins :user_id)
+            (->field-id :checkins :id)]
+   :order_by [[(->field-id :checkins :venue_id) "ascending"]
+              [(->field-id :checkins :user_id) "descending"]
+              [(->field-id :checkins :id) "ascending"]]})