diff --git a/src/metabase/automagic_dashboards/core.clj b/src/metabase/automagic_dashboards/core.clj
index 844cf3725b248d3a89019993566cc6f5ad863ac3..36859f386c7669266809aab93de6eaf1b65a9c67 100644
--- a/src/metabase/automagic_dashboards/core.clj
+++ b/src/metabase/automagic_dashboards/core.clj
@@ -430,12 +430,10 @@
(defn- build-query
([context bindings filters metrics dimensions limit order_by]
- (walk/postwalk
- (fn [subform]
- (if (rules/dimension-form? subform)
- (let [[_ identifier opts] subform]
- (->reference :mbql (-> identifier bindings (merge opts))))
- subform))
+ (qp.util/postwalk-pred
+ rules/dimension-form?
+ (fn [[_ identifier opts]]
+ (->reference :mbql (-> identifier bindings (merge opts))))
{:type :query
:database (-> context :root :database)
:query (cond-> {:source_table (if (->> context :source (instance? (type Table)))
diff --git a/src/metabase/query_processor/interface.clj b/src/metabase/query_processor/interface.clj
index 42359622cc39cda6f39904e94392f225dece7faa..c1215ac47bf0bc66fda1fe17c2cc4c1317d7bb5b 100644
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -177,11 +177,18 @@
[unit]
(contains? relative-datetime-value-units (keyword unit)))
+(def binning-strategies
+ "Valid binning strategies for a `BinnedField`"
+ #{:num-bins :bin-width :default})
+
;; TODO - maybe we should figure out some way to have the schema validate that the driver supports field literals,
;; like we do for some of the other clauses. Ideally we'd do that in a more generic way (perhaps in expand, we could
;; make the clauses specify required feature metadata and have that get checked automatically?)
-(s/defrecord FieldLiteral [field-name :- su/NonBlankString
- base-type :- su/FieldType]
+(s/defrecord FieldLiteral [field-name :- su/NonBlankString
+ base-type :- su/FieldType
+ binning-strategy :- (s/maybe (apply s/enum binning-strategies))
+ binning-param :- (s/maybe s/Num)
+ fingerprint :- (s/maybe i/Fingerprint)]
nil
:load-ns true
clojure.lang.Named
@@ -210,11 +217,7 @@
nil
:load-ns true)
-(def binning-strategies
- "Valid binning strategies for a `BinnedField`"
- #{:num-bins :bin-width :default})
-
-(s/defrecord BinnedField [field :- Field
+(s/defrecord BinnedField [field :- (s/cond-pre Field FieldLiteral)
strategy :- (apply s/enum binning-strategies)
num-bins :- s/Int
min-value :- s/Num
diff --git a/src/metabase/query_processor/middleware/binning.clj b/src/metabase/query_processor/middleware/binning.clj
index 1cde0589b6d6b03754300630ad61b024340dbdec..53c333bb23e80575af7357f2860499e1f9185a0f 100644
--- a/src/metabase/query_processor/middleware/binning.clj
+++ b/src/metabase/query_processor/middleware/binning.clj
@@ -3,7 +3,8 @@
[clojure.walk :as walk]
[metabase
[public-settings :as public-settings]
- [util :as u]])
+ [util :as u]]
+ [metabase.query-processor.util :as qputil])
(:import [metabase.query_processor.interface BetweenFilter BinnedField ComparisonFilter]))
(defn- update!
@@ -162,8 +163,6 @@
(fn [query]
(let [filter-field-map (filter->field-map (get-in query [:query :filter]))]
(qp
- (walk/postwalk (fn [node]
- (if (instance? BinnedField node)
- (update-binned-field node filter-field-map)
- node))
- query)))))
+ (qputil/postwalk-pred #(instance? BinnedField %)
+ #(update-binned-field % filter-field-map)
+ query)))))
diff --git a/src/metabase/query_processor/middleware/expand.clj b/src/metabase/query_processor/middleware/expand.clj
index 19ca6f57fd6e67e5d554cbb53c0d5a2bf82f7c51..3ce1dc965d512e9041d13758967010904dc327dd 100644
--- a/src/metabase/query_processor/middleware/expand.clj
+++ b/src/metabase/query_processor/middleware/expand.clj
@@ -225,7 +225,7 @@
;;; ## breakout & fields
-(s/defn ^:ql binning-strategy :- FieldPlaceholder
+(s/defn ^:ql binning-strategy :- (s/cond-pre FieldPlaceholder FieldLiteral)
"Reference to a `BinnedField`. This is just a `Field` reference with an associated `STRATEGY-NAME` and
`STRATEGY-PARAM`"
([f strategy-name & [strategy-param]]
diff --git a/src/metabase/query_processor/middleware/expand_macros.clj b/src/metabase/query_processor/middleware/expand_macros.clj
index b40b54d579a4dcb34a5d4bb90f3ba5c828630547..aa7f74ddc40ae06f9ea3da1177368d33c3b0ba19 100644
--- a/src/metabase/query_processor/middleware/expand_macros.clj
+++ b/src/metabase/query_processor/middleware/expand_macros.clj
@@ -105,12 +105,9 @@
(maybe-unnest-ag-clause ag-clause)))
(defn- expand-metrics-in-ag-clause [query-dict filter-clauses-atom]
- (walk/postwalk
- (fn [form]
- (if-not (metric? form)
- form
- (expand-metric form filter-clauses-atom)))
- query-dict))
+ (qputil/postwalk-pred metric?
+ #(expand-metric % filter-clauses-atom)
+ query-dict))
(defn merge-filter-clauses
"Merge filter clauses."
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index c8498e412692453ad9dd4a47bc915ac615be02df..997d146e13c6bcdf8929272a2954c5f6cb7cfbd1 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -24,7 +24,7 @@
(defn- card-id->source-query
"Return the source query info for Card with CARD-ID."
[card-id]
- (let [card (db/select-one ['Card :dataset_query :database_id] :id card-id)
+ (let [card (db/select-one ['Card :dataset_query :database_id :result_metadata] :id card-id)
card-query (:dataset_query card)]
(assoc (or (:query card-query)
(when-let [native (:native card-query)]
@@ -33,7 +33,8 @@
(throw (Exception. (str "Missing source query in Card " card-id))))
;; include database ID as well; we'll pass that up the chain so it eventually gets put in its spot in the
;; outer-query
- :database (:database card-query))))
+ :database (:database card-query)
+ :result_metadata (:result_metadata card))))
(defn- source-table-str->source-query
"Given a SOURCE-TABLE-STR like `card__100` return the appropriate source query."
@@ -41,7 +42,10 @@
(let [[_ card-id-str] (re-find #"^card__(\d+)$" source-table-str)]
(u/prog1 (card-id->source-query (Integer/parseInt card-id-str))
(when-not i/*disable-qp-logging*
- (log/info "\nFETCHED SOURCE QUERY FROM CARD" card-id-str ":\n" (u/pprint-to-str 'yellow <>))))))
+ (log/infof "\nFETCHED SOURCE QUERY FROM CARD %s:\n%s"
+ card-id-str
+ ;; No need to include result metadata here, it can be large and will clutter the logs
+ (u/pprint-to-str 'yellow (dissoc <> :result_metadata)))))))
(defn- expand-card-source-tables
"If `source-table` is a Card reference (a string like `card__100`) then replace that with appropriate
@@ -58,8 +62,9 @@
;; Add new `source-query` info in its place. Pass the database ID up the chain, removing it from the
;; source query
(assoc
- :source-query (dissoc source-query :database)
- :database (:database source-query)))))))
+ :source-query (dissoc source-query :database :result_metadata)
+ :database (:database source-query)
+ :result_metadata (:result_metadata source-query)))))))
(defn- fetch-source-query* [{inner-query :query, :as outer-query}]
(if-not inner-query
@@ -68,9 +73,11 @@
;; otherwise attempt to expand any source queries as needed
(let [expanded-inner-query (expand-card-source-tables inner-query)]
(merge outer-query
- {:query (dissoc expanded-inner-query :database)}
+ {:query (dissoc expanded-inner-query :database :result_metadata)}
(when-let [database (:database expanded-inner-query)]
- {:database database})))))
+ {:database database})
+ (when-let [result-metadata (:result_metadata expanded-inner-query)]
+ {:result_metadata result-metadata})))))
(defn fetch-source-query
"Middleware that assocs the `:source-query` for this query if it was specified using the shorthand `:source-table`
diff --git a/src/metabase/query_processor/middleware/resolve.clj b/src/metabase/query_processor/middleware/resolve.clj
index 78cf8066fce793386cc16e3020a90d3fcf2703cf..eacd0d3475bc7044f30c57c90ab9948198c202af 100644
--- a/src/metabase/query_processor/middleware/resolve.clj
+++ b/src/metabase/query_processor/middleware/resolve.clj
@@ -25,7 +25,7 @@
[db :as db]
[hydrate :refer [hydrate]]])
(:import java.util.TimeZone
- [metabase.query_processor.interface DateTimeField DateTimeValue ExpressionRef Field FieldPlaceholder
+ [metabase.query_processor.interface DateTimeField DateTimeValue ExpressionRef Field FieldLiteral FieldPlaceholder
RelativeDatetime RelativeDateTimeValue TimeField TimeValue Value ValuePlaceholder]))
;;; ---------------------------------------------------- UTIL FNS ----------------------------------------------------
@@ -161,7 +161,7 @@
;;; ----------------------------------------------- FIELD PLACEHOLDER ------------------------------------------------
-(defn- resolve-binned-field [{:keys [binning-strategy binning-param] :as field-ph} field]
+(defn- resolve-binned-field [binning-strategy binning-param field]
(let [binned-field (i/map->BinnedField {:field field
:strategy binning-strategy})]
(case binning-strategy
@@ -200,7 +200,7 @@
(i/map->TimeField {:field field})
binning-strategy
- (resolve-binned-field this field)
+ (resolve-binned-field binning-strategy binning-param field)
:else field)
;; If that fails just return ourselves as-is
@@ -399,6 +399,29 @@
(assoc-in <> [:query :join-tables] joined-tables)
(walk/postwalk #(resolve-table % fk-id+table-id->table) <>))))))
+(defn- resolve-field-literals
+ "When resolving a field, we connect a `field-id` with a `Field` in our metadata tables. This is a similar process
+ for `FieldLiteral`s, except we are attempting to connect a `FieldLiteral` with an associated entry in the
+ `result_metadata` attached to the query (typically from the `Card` of a nested query)."
+ [{:keys [result_metadata] :as expanded-query-dict}]
+ (let [name->fingerprint (zipmap (map :name result_metadata)
+ (map :fingerprint result_metadata))]
+ (qputil/postwalk-pred #(instance? FieldLiteral %)
+ (fn [{:keys [binning-strategy binning-param] :as node}]
+ (let [fingerprint (get name->fingerprint (:field-name node))
+ node-with-print (assoc node :fingerprint fingerprint)]
+ (cond
+ ;; We can't bin without min/max values found from a fingerprint
+ (and binning-strategy (not fingerprint))
+ (throw (Exception. "Binning not supported on a field literal with no fingerprint"))
+
+ (and fingerprint binning-strategy)
+ (resolve-binned-field binning-strategy binning-param node-with-print)
+
+ :else
+ node-with-print)))
+ expanded-query-dict)))
+
;;; ------------------------------------------------ PUBLIC INTERFACE ------------------------------------------------
@@ -408,6 +431,7 @@
(some-> expanded-query-dict
record-fk-field-ids
resolve-fields
+ resolve-field-literals
resolve-tables))
(defn resolve-middleware
diff --git a/src/metabase/query_processor/middleware/results_metadata.clj b/src/metabase/query_processor/middleware/results_metadata.clj
index 839a4ea8ebf7ce00b6a4d555f1702f82cf7add01..76110b33ed0f40ab94d7794cb72f176131bada3b 100644
--- a/src/metabase/query_processor/middleware/results_metadata.clj
+++ b/src/metabase/query_processor/middleware/results_metadata.clj
@@ -7,6 +7,7 @@
[clojure.tools.logging :as log]
[metabase.models.humanization :as humanization]
[metabase.query-processor.interface :as i]
+ [metabase.sync.interface :as si]
[metabase.util :as u]
[metabase.util
[encryption :as encryption]
@@ -29,7 +30,8 @@
(s/optional-key :description) (s/maybe su/NonBlankString)
:base_type su/FieldTypeKeywordOrString
(s/optional-key :special_type) (s/maybe su/FieldTypeKeywordOrString)
- (s/optional-key :unit) (s/maybe DateTimeUnitKeywordOrString)}]
+ (s/optional-key :unit) (s/maybe DateTimeUnitKeywordOrString)
+ (s/optional-key :fingerprint) (s/maybe si/Fingerprint)}]
"Valid array of results column metadata maps")
"value must be an array of valid results column metadata maps."))
@@ -44,7 +46,7 @@
;; if base-type isn't set put a default one in there. Similarly just use humanized value of `:name` for `:display_name` if one isn't set
{:base_type :type/*
:display_name (humanization/name->human-readable-name (name (:name col)))}
- (u/select-non-nil-keys col [:name :display_name :description :base_type :special_type :unit])
+ (u/select-non-nil-keys col [:name :display_name :description :base_type :special_type :unit :fingerprint])
;; since years are actually returned as text they can't be used for breakout purposes so don't advertise them as DateTime columns
(when (= (:unit col) :year)
{:base_type :type/Text
diff --git a/src/metabase/query_processor/util.clj b/src/metabase/query_processor/util.clj
index 5441fa30c01096ff1fed8fcd3795fafcc819d0a0..fedcc1671da0f148f10a0cab651226fbccd14e0d 100644
--- a/src/metabase/query_processor/util.clj
+++ b/src/metabase/query_processor/util.clj
@@ -4,7 +4,9 @@
[codecs :as codecs]
[hash :as hash]]
[cheshire.core :as json]
- [clojure.string :as str]
+ [clojure
+ [string :as str]
+ [walk :as walk]]
[metabase.util :as u]
[metabase.util.schema :as su]
[schema.core :as s]))
@@ -138,3 +140,14 @@
(when (string? source-table)
(when-let [[_ card-id-str] (re-matches #"^card__(\d+$)" source-table)]
(Integer/parseInt card-id-str)))))
+
+;;; ---------------------------------------- General Tree Manipulation Helpers ---------------------------------------
+
+(defn postwalk-pred
+ "Transform `form` by applying `f` to a node in the tree when that `pred` returns true for that node"
+ [pred f form]
+ (walk/postwalk (fn [node]
+ (if (pred node)
+ (f node)
+ node))
+ form))
diff --git a/test/metabase/api/table_test.clj b/test/metabase/api/table_test.clj
index 63682bd2c8924b92c77206b32ec5d0cffc24a81f..573b4bad625aa929e9bd3401fae35c71fc4fb5fe 100644
--- a/test/metabase/api/table_test.clj
+++ b/test/metabase/api/table_test.clj
@@ -667,3 +667,29 @@
(expect
#{:metrics :segments :linked-from :linking-to :tables}
(-> ((user->client :crowberto) :get 200 (format "table/%s/related" (data/id :venues))) keys set))
+
+;; Nested queries with a fingerprint should have dimension options for binning
+(datasets/expect-with-engines (qpt/non-timeseries-engines-with-feature :binning)
+ (repeat 2 (var-get #'table-api/coordinate-dimension-indexes))
+ (tt/with-temp Card [card {:database_id (data/id)
+ :dataset_query {:database (data/id)
+ :type :query
+ :query {:source-query {:source-table (data/id :venues)}}}}]
+ ;; run the Card which will populate its result_metadata column
+ ((user->client :crowberto) :post 200 (format "card/%d/query" (u/get-id card)))
+ (let [response ((user->client :crowberto) :get 200 (format "table/card__%d/query_metadata" (u/get-id card)))]
+ (map #(dimension-options-for-field response %)
+ ["latitude" "longitude"]))))
+
+;; Nested queries missing a fingerprint should not show binning-options
+(datasets/expect-with-engines (qpt/non-timeseries-engines-with-feature :binning)
+ [nil nil]
+ (tt/with-temp Card [card {:database_id (data/id)
+ :dataset_query {:database (data/id)
+ :type :query
+ :query {:source-query {:source-table (data/id :venues)}}}}]
+ ;; By default result_metadata will be nil (and no fingerprint). Just asking for query_metadata after the card was
+ ;; created but before it was ran should not allow binning
+ (let [response ((user->client :crowberto) :get 200 (format "table/card__%d/query_metadata" (u/get-id card)))]
+ (map #(dimension-options-for-field response %)
+ ["latitude" "longitude"]))))
diff --git a/test/metabase/query_processor/middleware/fetch_source_query_test.clj b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
index 0b9d4936ddb4710ab1a7eee87503386b201feafa..8905749e71f69e8256e2e17ebf16fdd815929e23 100644
--- a/test/metabase/query_processor/middleware/fetch_source_query_test.clj
+++ b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
@@ -76,21 +76,22 @@
:query {:source-table (str "card__" (u/get-id card))}})))
(expect
- (default-expanded-results
- {:source-query {:source-table {:schema "PUBLIC" :name "CHECKINS" :id (data/id :checkins)}, :join-tables nil}
- :filter {:filter-type :between,
- :field {:field-name "date", :base-type :type/Date},
- :min-val {:value (tcoerce/to-timestamp (du/str->date-time "2015-01-01"))
- :field {:field {:field-name "date", :base-type :type/Date}, :unit :default}},
- :max-val {:value (tcoerce/to-timestamp (du/str->date-time "2015-02-01"))
- :field {:field {:field-name "date", :base-type :type/Date}, :unit :default}}}})
+ (let [date-field-literal {:field-name "date", :base-type :type/Date, :binning-strategy nil, :binning-param nil, :fingerprint nil}]
+ (default-expanded-results
+ {:source-query {:source-table {:schema "PUBLIC" :name "CHECKINS" :id (data/id :checkins)}, :join-tables nil}
+ :filter {:filter-type :between,
+ :field date-field-literal
+ :min-val {:value (tcoerce/to-timestamp (du/str->date-time "2015-01-01"))
+ :field {:field date-field-literal, :unit :default}},
+ :max-val {:value (tcoerce/to-timestamp (du/str->date-time "2015-02-01"))
+ :field {:field date-field-literal, :unit :default}}}}))
(tt/with-temp Card [card {:dataset_query {:database (data/id)
:type :query
:query {:source-table (data/id :checkins)}}}]
(expand-and-scrub {:database database/virtual-id
:type :query
:query {:source-table (str "card__" (u/get-id card))
- :filter ["BETWEEN" ["field-id" ["field-literal" "date" "type/Date"]] "2015-01-01" "2015-02-01"]}})))
+ :filter ["BETWEEN" ["field-id" ["field-literal" "date" "type/Date"]] "2015-01-01" "2015-02-01"]}})))
;; make sure that nested nested queries work as expected
(expect
diff --git a/test/metabase/query_processor/middleware/results_metadata_test.clj b/test/metabase/query_processor/middleware/results_metadata_test.clj
index 301942ad7b933d765b69439bd70d4824425d4fc2..e681b7e31866bf3b2480945e0737ef5b4bea3dc3 100644
--- a/test/metabase/query_processor/middleware/results_metadata_test.clj
+++ b/test/metabase/query_processor/middleware/results_metadata_test.clj
@@ -91,7 +91,9 @@
[{:base_type "type/Text"
:display_name "Date"
:name "DATE"
- :unit nil}
+ :unit nil
+ :fingerprint {:global {:distinct-count 618}, :type {:type/DateTime {:earliest "2013-01-03T00:00:00.000Z"
+ :latest "2015-12-29T00:00:00.000Z"}}}}
{:base_type "type/Integer"
:display_name "count"
:name "count"
diff --git a/test/metabase/query_processor_test/breakout_test.clj b/test/metabase/query_processor_test/breakout_test.clj
index cdca9fa82415ed980cb3b9e5995c0838ca0075d5..a0a7e040ae6fd014fb0ec5ba11cb9dd5e8a0e2c7 100644
--- a/test/metabase/query_processor_test/breakout_test.clj
+++ b/test/metabase/query_processor_test/breakout_test.clj
@@ -5,9 +5,11 @@
[query-processor-test :refer :all]
[util :as u]]
[metabase.models
+ [card :refer [Card]]
[dimension :refer [Dimension]]
[field :refer [Field]]
[field-values :refer [FieldValues]]]
+ [metabase.query-processor :as qp]
[metabase.query-processor.middleware
[add-dimension-projections :as add-dim-projections]
[expand :as ql]]
@@ -17,7 +19,8 @@
[metabase.test.data
[dataset-definitions :as defs]
[datasets :as datasets]]
- [toucan.db :as db]))
+ [toucan.db :as db]
+ [toucan.util.test :as tt]))
;;; single column
(qp-expect-with-all-engines
@@ -245,3 +248,34 @@
(ql/aggregation (ql/count))
(ql/breakout (ql/binning-strategy $latitude :default)))
(select-keys [:status :class :error]))))
+
+(defn- field->result-metadata [field]
+ (select-keys field [:name :display_name :description :base_type :special_type :unit :fingerprint]))
+
+(defn- nested-venues-query [card-or-card-id]
+ {:database metabase.models.database/virtual-id
+ :type :query
+ :query {:source-table (str "card__" (u/get-id card-or-card-id))
+ :aggregation [:count]
+ :breakout [(ql/binning-strategy (ql/field-literal "LATITUDE" :type/Float) :num-bins 20)]}})
+
+;; Binning should be allowed on nested queries that have result metadata
+(datasets/expect-with-engines (non-timeseries-engines-with-feature :binning)
+ [[10.0 1] [32.0 4] [34.0 57] [36.0 29] [40.0 9]]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-query {:source-table (data/id :venues)}}}
+ :result_metadata (mapv field->result-metadata (db/select Field :table_id (data/id :venues)))}]
+ (-> (nested-venues-query card)
+ qp/process-query
+ rows)))
+
+;; Binning is not supported when there is no fingerprint to determine boundaries
+(datasets/expect-with-engines (non-timeseries-engines-with-feature :binning)
+ Exception
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-query {:source-table (data/id :venues)}}}}]
+ (-> (nested-venues-query card)
+ qp/process-query
+ rows)))