Merge pull request #341 from metabase/qp_better_ordering

QP orders all results based on value of Field.position

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

@@ -25,19 +25,46 @@
             :columns column-names
             :cols (get-column-info query column-names)}}))
 
- (defn order-columns
-   [{{source-table :source_table breakout-field-ids :breakout} :query} field-names]
-   (let [uncastified->field-name (->> field-names
-                                      (map (fn [field-name]
-                                             {(uncastify (name field-name)) field-name}))
-                                      (into {}))
-         field-id->uncastified (sel :many :id->field [Field :name] :name [in (set (keys uncastified->field-name))])
-         breakout-field-names (->> breakout-field-ids
-                                   (filter identity)
-                                   (map field-id->uncastified)
-                                   (map uncastified->field-name))]
-     (concat breakout-field-names (filter #(not (contains? (set breakout-field-names) %))
-                                          field-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))
 
 (defn- get-column-names
   "Get an ordered seqences of column names for the results.

src/metabase/models/field.clj

@@ -116,8 +116,11 @@
   field)
 
 (defmethod post-update Field [_ {:keys [id] :as field}]
-  (future
-    (create-field-values-if-needed (sel :one Field :id id))))
+  ;; if base_type or special_type were affected then we should asynchronously create corresponding FieldValues objects if need be
+  (when (or (contains? field :base_type)
+            (contains? field :special_type))
+    (future
+      (create-field-values-if-needed (sel :one [Field :id :table_id :base_type :special_type] :id id)))))
 
 (defmethod pre-cascade-delete Field [_ {:keys [id]}]
   (cascade-delete ForeignKey (where (or (= :origin_id id)

src/metabase/models/field_values.clj

@@ -30,7 +30,9 @@
 (defn field-should-have-field-values?
   "Should this `Field` be backed by a corresponding `FieldValues` object?"
   {:arglists '([field])}
-  [{:keys [base_type special_type]}]
+  [{:keys [base_type special_type] :as field}]
+  {:pre [(contains? field :base_type)
+         (contains? field :special_type)]}
   (or (contains? #{:category :city :state :country} (keyword special_type))
       (= (keyword base_type) :BooleanField)))
 

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.test.util :refer [resolve-private-fns]]))
+
+(resolve-private-fns metabase.driver.generic-sql.query-processor.annotate -order-columns)
+
+;; ## TESTS FOR -ORDER-COLUMNS
+
+(def ^:const ^:private mock-fields
+  [{:position 0, :name "id", :id 224}
+   {:position 1, :name "name", :id 341}
+   {:position 2, :name "author_name", :id 453}
+   {:position 3, :name "allows_suggestions", :id 433}
+   {:position 4, :name "author_url", :id 455}
+   {:position 5, :name "content_creator_id", :id 446}
+   {:position 6, :name "created_at", :id 263}
+   {:position 7, :name "description", :id 375}
+   {:position 8, :name "external_url", :id 424}
+   {:position 9, :name "updated_at", :id 284}])
+
+(def ^:const ^:private mock-castified-field-names
+  [:allows_suggestions
+   :description
+   :author_url
+   :name
+   (keyword "CAST(updated_at AS DATE)")
+   :id
+   :author_name
+   :external_url
+   :content_creator_id
+   (keyword "CAST(created_at AS DATE)")])
+
+;; Check that `Field.order` is respected. No breakout fields
+(expect [:id
+         :name
+         :author_name
+         :allows_suggestions
+         :author_url
+         :content_creator_id
+         (keyword "CAST(created_at AS DATE)")
+         :description
+         :external_url
+         (keyword "CAST(updated_at AS DATE)")]
+  (-order-columns mock-fields [] mock-castified-field-names))
+
+;; Check that breakout fields are returned first, in order, before other fields
+(expect [:description
+         :allows_suggestions
+         :id
+         :name
+         :author_name
+         :author_url
+         :content_creator_id
+         (keyword "CAST(created_at AS DATE)")
+         :external_url
+         (keyword "CAST(updated_at AS DATE)")]
+  (-order-columns mock-fields [375 433] mock-castified-field-names))
+
+;; Check that aggregate fields are returned ahead of other fields
+(expect [:allows_suggestions
+         :description
+         :count
+         :id
+         :name
+         :author_name
+         :author_url
+         :content_creator_id
+         (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)))

test/metabase/driver/generic_sql/sync_test.clj

@@ -6,14 +6,8 @@
                                          [util :refer [korma-entity]])
             (metabase.models [field :refer [Field]]
                              [table :refer [Table]])
-            [metabase.test-data :refer :all]))
-
-
-;; TODO  - This is fairly useful, should probably move to metabase.test.util
-(defmacro resolve-private-fns [namespc fn-name & more]
-  `(do (def ~fn-name (ns-resolve '~namespc '~fn-name))
-       ~(when (seq more)
-          `(resolve-private-fns ~namespc ~(first more) ~@(rest more)))))
+            [metabase.test-data :refer :all]
+            [metabase.test.util :refer [resolve-private-fns]]))
 
 (resolve-private-fns metabase.driver.generic-sql.sync field-avg-length field-percent-urls)
 

test/metabase/test/util.clj

@@ -103,3 +103,17 @@
                           (throw e#)))]
        (delete-fn#)
        result#)))
+
+
+;; ## resolve-private-fns
+
+(defmacro resolve-private-fns
+  "Have your cake and eat it too. This Macro adds private functions from another namespace to the current namespace so we can test them.
+
+    (resolve-private-fns metabase.driver.generic-sql.sync
+      field-avg-length field-percent-urls)"
+  {:arglists '([namespace-symb & fn-symbs])}
+  [namespc fn-name & more]
+  `(do (def ~fn-name (ns-resolve '~namespc '~fn-name))
+       ~(when (seq more)
+          `(resolve-private-fns ~namespc ~(first more) ~@(rest more)))))