update the way we build our dataset query results output so that we order the...

update the way we build our dataset query results output so that we order the columns/values in a way that ensures dimensions are returned predictably before other columns like aggregations.

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

@@ -28,19 +28,29 @@
   "Get an ordered seqences of column names for the results.
    If a `fields` clause was specified in the Query Dict, we want to return results in the same order."
   [query results]
-  (let [field-ids (-> query :query :fields)
-        fields-clause-fields (when-not (or (empty? field-ids)
-                                           (= field-ids [nil]))
-                               (let [field-id->name (->> (sel :many [Field :id :name]
-                                                              :id [in field-ids])     ; Fetch names of fields from `fields` clause
-                                                         (map (fn [{:keys [id name]}]  ; build map of field-id -> field-name
-                                                                {id (keyword name)}))
-                                                         (into {}))]
-                                 (map field-id->name field-ids)))                     ; now get names in same order as the IDs
+  (let [ids->names (fn [field-ids]
+                     (when-not (or (empty? field-ids)
+                                 (= field-ids [nil]))
+                       (let [field-id->name (->> (sel :many [Field :id :name :base_type]
+                                                   :id [in field-ids])     ; Fetch names of fields from `fields` clause
+                                              (map (fn [{:keys [id name base_type]}]  ; build map of field-id -> field-name
+                                                     ;; this feels hacky, but we can't reuse `(castify)` because we don't want a korma form here :(
+                                                     (if (contains? #{:DateField :DateTimeField} base_type)
+                                                       {id (keyword (format "CAST(%s AS DATE)" name))}
+                                                       {id (keyword name)})))
+                                              (into {}))]
+                         (map field-id->name field-ids))))                  ; now get names in same order as the IDs
+        field-ids (-> query :query :fields)
+        fields-clause-fields (ids->names field-ids)
+        ;; note that we are protecting against the possibility that a dimension already exists in :fields
+        dimension-ids (filter #(not (contains? (set field-ids) %)) (-> query :query :breakout))
+        dimensions-clause-fields (ids->names dimension-ids)
         other-fields (->> (first results)
                           keys                                                        ; Get the names of any other fields that were returned (i.e., `sum`)
-                          (filter #(not (contains? (set fields-clause-fields) %))))]
-    (->> (concat fields-clause-fields other-fields)                                   ; Return a combined vector. Convert them to strs, otherwise korma
+                          (filter #(not (contains? (clojure.set/union
+                                                     (set fields-clause-fields)
+                                                     (set dimensions-clause-fields)) %))))]
+    (->> (concat fields-clause-fields dimensions-clause-fields other-fields)                                   ; Return a combined vector. Convert them to strs, otherwise korma
          (map name))))                                                                ; will qualify them like `"METABASE_FIELD"."FOLLOWERS_COUNT"
 
 (defn- uncastify

test/metabase/driver/generic_sql/query_processor_test.clj

@@ -353,10 +353,10 @@
 ;; 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 [{: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)}
+         :data {:rows [[1 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]],
+                :columns ["USER_ID" "VENUE_ID" "count"],
+                :cols [{: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 {: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)}
                        {:base_type :IntegerField, :special_type :number, :name "count", :id nil, :table_id nil, :description nil}]}}
         (process-and-run {:type :query
                           :database @db-id
@@ -370,10 +370,10 @@
 ;; `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]],
-                :columns ["VENUE_ID" "USER_ID" "count"],
-                :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)}
+         :data {:rows [[15 2 1] [15 3 1] [15 7 1] [15 14 1] [15 16 1] [15 18 1] [15 22 1] [15 23 2] [15 24 1] [15 27 1]],
+                :columns ["USER_ID" "VENUE_ID" "count"],
+                :cols [{: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 {: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)}
                        {:base_type :IntegerField, :special_type :number, :name "count", :id nil, :table_id nil, :description nil}]}}
         (process-and-run {:type :query
                           :database @db-id
@@ -420,9 +420,9 @@
 (expect {:status :completed
          :row_count 15
          :data {:rows [4 12 13 22 34 44 57 72 78 85 90 104 115 118 120]
-                :columns ["ID" "CAST(LAST_LOGIN AS DATE)"]
-                :cols [{:extra_info {} :special_type :id, :base_type :IntegerField, :description nil, :name "ID", :table_id (table->id :users), :id (field->id :users :id)}
-                       {:extra_info {} :special_type nil, :base_type :DateTimeField, :description nil, :name "LAST_LOGIN", :table_id (table->id :users), :id (field->id :users :last_login)}]}}
+                :columns ["CAST(LAST_LOGIN AS DATE)" "ID"]
+                :cols [{:extra_info {} :special_type nil, :base_type :DateTimeField, :description nil, :name "LAST_LOGIN", :table_id (table->id :users), :id (field->id :users :last_login)}
+                       {:extra_info {} :special_type :id, :base_type :IntegerField, :description nil, :name "ID", :table_id (table->id :users), :id (field->id :users :id)}]}}
   (-> (process-and-run {:type :query
                         :database @db-id
                         :query {:limit nil
@@ -432,7 +432,7 @@
                                 :aggregation ["cum_sum" (field->id :users :id)]}})
       ;; Rows come back like `[value timestamp]` but it is hard to compare timestamps directly since the values that come back are casted
       ;; to Dates and the exact value depends on the locale of the machine running the tests. So just drop the timestamps from the results.
-      (update-in [:data :rows] (partial map first))))
+      (update-in [:data :rows] (partial map last))))
 
 
 ;; # ERROR RESPONSES