From 9e1827bbbf502dbcf99c3ca6be81453875ab05bc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cam=20Sau=CC=88l?= <cammsaul@gmail.com>
Date: Thu, 9 Jul 2015 13:03:51 -0700
Subject: [PATCH] Fix column ordering / test fix
---
.../driver/generic_sql/query_processor.clj | 59 ++++++++++++-------
src/metabase/driver/query_processor.clj | 59 ++++++++++---------
test/metabase/driver/query_processor_test.clj | 32 +++++-----
3 files changed, 86 insertions(+), 64 deletions(-)
diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj
index d7ab6f153f6..d3e704c4769 100644
--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -96,33 +96,50 @@
(defprotocol IGenericSQLFormattable
- (formatted [this]))
+ (formatted [this] [this include-as?]))
(extend-protocol IGenericSQLFormattable
Field
- (formatted [{:keys [table-name field-name base-type special-type]}]
- ;; TODO - add Table names
- (cond
- (contains? #{:DateField :DateTimeField} base-type) `(raw ~(format "CAST(\"%s\".\"%s\" AS DATE)" table-name field-name))
- (= special-type :timestamp_seconds) `(raw ~((:cast-timestamp-seconds-field-to-date-fn (:driver *query*)) table-name field-name))
- (= special-type :timestamp_milliseconds) `(raw ~((:cast-timestamp-milliseconds-field-to-date-fn (:driver *query*)) table-name field-name))
- :else (keyword (format "%s.%s" table-name field-name))))
+ (formatted
+ ([this]
+ (formatted this false))
+ ([{:keys [table-name field-name base-type special-type]} include-as?]
+ ;; TODO - add Table names
+ (cond
+ (contains? #{:DateField :DateTimeField} base-type) `(raw ~(str (format "CAST(\"%s\".\"%s\" AS DATE)" table-name field-name)
+ (when include-as?
+ (format " AS \"%s\"" field-name))))
+ (= special-type :timestamp_seconds) `(raw ~(str ((:cast-timestamp-seconds-field-to-date-fn (:driver *query*)) table-name field-name)
+ (when include-as?
+ (format " AS \"%s\"" field-name))))
+ (= special-type :timestamp_milliseconds) `(raw ~(str ((:cast-timestamp-milliseconds-field-to-date-fn (:driver *query*)) table-name field-name)
+ (when include-as?
+ (format " AS \"%s\"" field-name))))
+ :else (keyword (format "%s.%s" table-name field-name)))))
+
;; e.g. the ["aggregation" 0] fields we allow in order-by
OrderByAggregateField
- (formatted [_]
- (let [{:keys [aggregation-type]} (:aggregation (:query *query*))] ; determine the name of the aggregation field
- `(raw ~(case aggregation-type
- :avg "\"avg\""
- :count "\"count\""
- :distinct "\"count\""
- :stddev "\"stddev\""
- :sum "\"sum\""))))
+ (formatted
+ ([this]
+ (formatted this false))
+ ([_ _]
+ (let [{:keys [aggregation-type]} (:aggregation (:query *query*))] ; determine the name of the aggregation field
+ `(raw ~(case aggregation-type
+ :avg "\"avg\""
+ :count "\"count\""
+ :distinct "\"count\""
+ :stddev "\"stddev\""
+ :sum "\"sum\"")))))
+
Value
- (formatted [{:keys [value]}]
- (if-not (instance? java.util.Date value) value
- `(raw ~(format "CAST('%s' AS DATE)" (.toString ^java.util.Date value))))))
+ (formatted
+ ([this]
+ (formatted this false))
+ ([{:keys [value]} _]
+ (if-not (instance? java.util.Date value) value
+ `(raw ~(format "CAST('%s' AS DATE)" (.toString ^java.util.Date value)))))))
(defmethod apply-form :aggregation [[_ {:keys [aggregation-type field]}]]
@@ -148,11 +165,11 @@
;; Add fields form only for fields that weren't specified in :fields clause -- we don't want to include it twice, or korma will barf
(fields ~@(->> fields
(filter (partial (complement contains?) (set (:fields (:query *query*)))))
- (map formatted)))])
+ (map (u/rpartial formatted :include-as))))])
(defmethod apply-form :fields [[_ fields]]
- `(fields ~@(map formatted fields)))
+ `(fields ~@(map (u/rpartial formatted :include-as) fields)))
(defn- filter-subclause->predicate
diff --git a/src/metabase/driver/query_processor.clj b/src/metabase/driver/query_processor.clj
index cd3639b0294..f49e400b835 100644
--- a/src/metabase/driver/query_processor.clj
+++ b/src/metabase/driver/query_processor.clj
@@ -222,19 +222,19 @@
(defn- order-cols
"Construct a sequence of column keywords that should be used for pulling ordered rows from RESULTS.
FIELDS should be a sequence of all `Fields` for the `Table` associated with QUERY."
- [{{breakout-fields :breakout, fields-fields :fields, fields-is-implicit :fields-is-implicit} :query} results fields]
- {:post [(= (set %)
- (set (keys (first results))))]}
- (let [;; TODO - This function was written before the advent of the expanded query it is designed to work with Field IDs rather than expanded forms
- ;; Since this logic is delicate I've side-stepped the issue by converting the expanded Fields back to IDs for the time being.
- ;; We should carefully re-work this function to use expanded Fields so we don't need the complicated logic below to fetch their names
+ [{{breakout-fields :breakout, {ag-type :aggregation-type} :aggregation, fields-fields :fields, fields-is-implicit :fields-is-implicit} :query} results fields]
+ (let [;; Get all the column name keywords returned by the results
+ result-kws (set (keys (first results)))
+ valid-kw? (partial contains? result-kws)
+
breakout-ids (map :field-id breakout-fields)
- breakout-kws (for [field breakout-fields]
- (->> (rest (expand/qualified-name-components field)) ; TODO - this "qualified name for results" should be calculated in the Query expander
- (interpose ".")
- (apply str)
- keyword))
+ breakout-kws (->> (for [field breakout-fields]
+ (->> (rest (expand/qualified-name-components field)) ; TODO - this "qualified name for results" should be calculated in the Query expander
+ (interpose ".")
+ (apply str)
+ keyword))
+ (filter valid-kw?))
fields-ids (map :field-id fields-fields)
@@ -259,9 +259,6 @@
(filter (complement (partial contains? (set breakout-ids))))
distinct)
- ;; Get all the column name keywords returned by the results
- result-kws (set (keys (first results)))
-
;; Make a helper function that will take a sequence of Field IDs and convert them to corresponding column name keywords.
;; Don't include names that aren't part of RESULT-KWS: we fetch *all* the Fields for a Table regardless of the Query, so
;; there are likely some unused ones.
@@ -269,26 +266,34 @@
(some->> (map field-id->field field-ids)
(map :name)
(map keyword)
- (filter (partial contains? result-kws))))
+ (filter valid-kw?)))
;; Use fn above to get the keyword column names of other non-aggregation fields [#3 and #4]
non-breakout-kws (ids->kws non-breakout-ids)
- ;; Now get all the keyword column names specific to aggregation, such as :sum or :count [#2].
- ;; Just get all the items in RESULT-KWS that *aren't* part of BREAKOUT-KWS or NON-BREAKOUT-KWS
- ag-kws (->> result-kws
- ;; TODO - Currently, this will never be more than a single Field, since we only
- ;; support a single aggregation clause at this point. When we add support for
- ;; multiple aggregation clauses, we'll need to add some logic to make sure they're
- ;; being ordered correctly, e.g. the first aggregate column before the second, etc.
- (filter (complement (partial contains? (set (concat breakout-kws non-breakout-kws))))))]
- (assert (<= (count ag-kws) 1)
- (format "Invalid number of aggregate columns: %d" (count ag-kws)))
+ ;; Get the aggregate column if any
+ ag-kws (when (and ag-type
+ (not= ag-type :rows))
+ (let [ag (if (= ag-type :distinct) :count
+ ag-type)]
+ [ag]))
+
+ ;; Collect all other Fields
+ other-kws (->> result-kws
+ (filter (complement (partial contains? (set (concat breakout-kws non-breakout-kws ag-kws)))))
+ sort)] ; sort by name so results are deterministic
+
+ (when (seq other-kws)
+ (log/warn (u/format-color 'red "Warning: not 100%% sure how to order these columns: %s" (vec other-kws))))
;; Now combine the breakout [#1] + aggregate [#2] + "non-breakout" [#3 & #4] column name keywords into a single sequence
(when-not *disable-qp-logging*
- (log/debug (u/format-color 'magenta "Using this ordering: breakout: %s, ag: %s, other: %s" (vec breakout-kws) (vec ag-kws) (vec non-breakout-kws))))
- (concat breakout-kws ag-kws non-breakout-kws)))
+ (log/debug (u/format-color 'magenta "Using this ordering: breakout: %s, ag: %s, non-breakout: %s, other: %s"
+ (vec breakout-kws) (vec ag-kws) (vec non-breakout-kws) (vec other-kws))))
+ (let [ordered-kws (concat breakout-kws ag-kws non-breakout-kws other-kws)]
+ (assert (= (set ordered-kws) result-kws)
+ (format "Order-cols returned invalid results: expected %s, got %s" result-kws (set ordered-kws)))
+ ordered-kws)))
(defn- add-fields-extra-info
"Add `:extra_info` about `ForeignKeys` to `Fields` whose `special_type` is `:fk`."
diff --git a/test/metabase/driver/query_processor_test.clj b/test/metabase/driver/query_processor_test.clj
index 0aa87799d72..aee567b2021 100644
--- a/test/metabase/driver/query_processor_test.clj
+++ b/test/metabase/driver/query_processor_test.clj
@@ -912,22 +912,22 @@
;; (What he was doing when we saw him, sighting ID)
;; Check that we can include an FK field in the :fields clause
(datasets/expect-with-datasets #{:h2 :postgres}
- [["In the Park" 772]
- ["Working at a Pet Store" 894]
- ["At the Airport" 684]
- ["At a Restaurant" 199]
- ["Working as a Limo Driver" 33]
- ["At Starbucks" 902]
- ["On TV" 927]
- ["At a Restaurant" 996]
- ["Wearing a Biggie Shirt" 897]
- ["In the Expa Office" 499]]
- (->> (query-with-temp-db defs/tupac-sightings
- :source_table &sightings:id
- :fields [&sightings.id:id ["fk->" &sightings.category_id:id &categories.name:id]]
- :order_by [[&sightings.timestamp:id "descending"]]
- :limit 10)
- :data :rows))
+ [[772 "In the Park"]
+ [894 "Working at a Pet Store"]
+ [684 "At the Airport"]
+ [199 "At a Restaurant"]
+ [33 "Working as a Limo Driver"]
+ [902 "At Starbucks"]
+ [927 "On TV"]
+ [996 "At a Restaurant"]
+ [897 "Wearing a Biggie Shirt"]
+ [499 "In the Expa Office"]]
+ (Q run against tupac-sightings
+ return :data :rows
+ of sightings
+ fields id category_id->categories.name
+ order timestamp-
+ lim 10))
;; 1. Check that we can order by Foreign Keys
--
GitLab