diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj
index d048a8597efaf6bfc9a927dbd0c7bb0f87d655c0..cfa8b84ed2ab69b0ff501e3b39b203bed08e17ad 100644
--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -113,7 +113,7 @@
(->honeysql driver field))
(defmethod ->honeysql [Object BinnedField]
- [driver {:keys [bin-width min-value max-value field]}]
+ [driver {:keys [bin-width min-value max-value field] :as binned-field}]
(let [honeysql-field-form (->honeysql driver field)]
;;
;; Equation is | (value - min) |
diff --git a/src/metabase/mbql/normalize.clj b/src/metabase/mbql/normalize.clj
index 8b5364c37d1a359dd6de83d623e7e1594a7ce766..b8af46de4651ccf353cdbc0ff4b1964286ed8da5 100644
--- a/src/metabase/mbql/normalize.clj
+++ b/src/metabase/mbql/normalize.clj
@@ -197,20 +197,27 @@
(defn- normalize-source-query [{native? :native, :as source-query}]
(normalize-tokens source-query [(if native? :native :query)]))
+(defn- normalize-source-metadata [metadata]
+ (-> metadata
+ (update :base_type keyword)
+ (update :special_type keyword)
+ (update :fingerprint walk/keywordize-keys )))
+
(def ^:private path->special-token-normalization-fn
"Map of special functions that should be used to perform token normalization for a given path. For example, the
`:expressions` key in an MBQL query should preserve the case of the expression names; this custom behavior is
defined below."
- {:type mbql.u/normalize-token
+ {:type mbql.u/normalize-token
;; don't normalize native queries
- :native {:query identity
- :template-tags normalize-template-tags}
- :query {:aggregation normalize-ag-clause-tokens
- :expressions normalize-expressions-tokens
- :order-by normalize-order-by-tokens
- :source-query normalize-source-query}
- :parameters {::sequence normalize-query-parameter}
- :context #(some-> % mbql.u/normalize-token)})
+ :native {:query identity
+ :template-tags normalize-template-tags}
+ :query {:aggregation normalize-ag-clause-tokens
+ :expressions normalize-expressions-tokens
+ :order-by normalize-order-by-tokens
+ :source-query normalize-source-query}
+ :parameters {::sequence normalize-query-parameter}
+ :context #(some-> % mbql.u/normalize-token)
+ :source-metadata {::sequence normalize-source-metadata}})
(defn normalize-tokens
"Recursively normalize tokens in `x`.
diff --git a/src/metabase/mbql/schema.clj b/src/metabase/mbql/schema.clj
index 2b6ebcb21ebd2691cf6225d248c5c60ff666b348..de7306ffb38a88d250b30df39c786081bc617c04 100644
--- a/src/metabase/mbql/schema.clj
+++ b/src/metabase/mbql/schema.clj
@@ -31,6 +31,7 @@
"Schema for an MBQL datetime string literal, in ISO-8601 format."
(s/constrained su/NonBlankString du/date-string? "datetime-literal"))
+;; TODO - `unit` is not allowed if `n` is `current`
(defclause relative-datetime
n (s/cond-pre (s/eq :current) s/Int)
unit (optional RelativeDatetimeUnit))
@@ -71,13 +72,30 @@
unit DatetimeFieldUnit)
;; binning strategy can wrap any of the above clauses, but again, not another binning strategy clause
-(def ^:private BinningStrategyName
+(def BinningStrategyName
+ "Schema for a valid value for the `strategy-name` param of a `binning-strategy` clause."
(s/enum :num-bins :bin-width :default))
+(def BinnableField
+ "Schema for any sort of field clause that can be wrapped by a `binning-strategy` clause."
+ (one-of field-id fk-> datetime-field field-literal))
+
+(def ResolvedBinningStrategyOptions
+ "Schema for map of options tacked on to the end of `binning-strategy` clauses by the `binning` middleware."
+ {:num-bins su/IntGreaterThanZero
+ :bin-width (s/constrained s/Num (complement neg?) "bin width must be >= 0.")
+ :min-value s/Num
+ :max-value s/Num})
+
+;; TODO - binning strategy param is disallowed for `:default` and required for the others. For `num-bins` it must also
+;; be an integer.
(defclause binning-strategy
- field (one-of field-id field-literal fk-> expression datetime-field)
- strategy-name BinningStrategyName
- strategy-param (optional s/Num))
+ field BinnableField
+ strategy-name BinningStrategyName
+ strategy-param (optional (s/maybe (s/constrained s/Num (complement neg?) "strategy param must be >= 0.")))
+ ;; These are added in automatically by the `binning` middleware. Don't add them yourself, as they're just be
+ ;; replaced.
+ resolved-options (optional ResolvedBinningStrategyOptions))
(def Field
"Schema for anything that refers to a Field, from the common `[:field-id <id>]` to variants like `:datetime-field` or
@@ -245,6 +263,7 @@
"Schema for things that make sense in a filter like `>` or `<`, i.e. things that can be sorted."
(s/cond-pre
s/Num
+ s/Str
DatetimeLiteral
FieldOrRelativeDatetime))
@@ -450,6 +469,17 @@
;; when we get a chance
(s/optional-key :query-type) (s/enum "MBQL" "native")})
+(def SourceQueryMetadata
+ "Schema for the expected keys in metadata about source query columns if it is passed in to the query."
+ ;; TODO - there is a very similar schema in `metabase.sync.analyze.query-results`; see if we can merge them
+ {:name su/NonBlankString
+ :display_name su/NonBlankString
+ :base_type su/FieldType
+ (s/optional-key :special_type) (s/maybe su/FieldType)
+ ;; you'll need to provide this in order to use BINNING
+ (s/optional-key :fingerprint) (s/maybe su/Map)
+ s/Any s/Any})
+
;;; --------------------------------------------- Metabase [Outer] Query ---------------------------------------------
@@ -458,30 +488,36 @@
`Card.dataset_query`."
(s/constrained
;; TODO - move database/virtual-id into this namespace so we don't have to use the magic number here
- {:database (s/cond-pre (s/eq -1337) su/IntGreaterThanZero)
+ {:database (s/cond-pre (s/eq -1337) su/IntGreaterThanZero)
;; Type of query. `:query` = MBQL; `:native` = native. TODO - consider normalizing `:query` to `:mbql`
- :type (s/enum :query :native)
- (s/optional-key :native) NativeQuery
- (s/optional-key :query) MBQLQuery
- (s/optional-key :parameters) [Parameter]
+ :type (s/enum :query :native)
+ (s/optional-key :native) NativeQuery
+ (s/optional-key :query) MBQLQuery
+ (s/optional-key :parameters) [Parameter]
;;
;; OPTIONS
;;
;; These keys are used to tweak behavior of the Query Processor.
;; TODO - can we combine these all into a single `:options` map?
;;
- (s/optional-key :settings) (s/maybe Settings)
- (s/optional-key :constraints) (s/maybe Constraints)
- (s/optional-key :middleware) (s/maybe MiddlewareOptions)
+ (s/optional-key :settings) (s/maybe Settings)
+ (s/optional-key :constraints) (s/maybe Constraints)
+ (s/optional-key :middleware) (s/maybe MiddlewareOptions)
;;
;; INFO
;;
;; Used when recording info about this run in the QueryExecution log; things like context query was ran in and
;; User who ran it
- (s/optional-key :info) (s/maybe Info)
+ (s/optional-key :info) (s/maybe Info)
+ ;; Info about the columns of the source query. Added in automatically by middleware. This metadata is primarily
+ ;; used to let power things like binning when used with Field Literals instead of normal Fields
+ (s/optional-key :source-metadata) (s/maybe [SourceQueryMetadata])
+ #_:fk-field-ids
+ #_:table-ids
+ ;;
;; Other various keys get stuck in the query dictionary at some point or another by various pieces of QP
;; middleware to record bits of state. Everyone else can ignore them.
- s/Keyword s/Any}
+ s/Keyword s/Any}
(fn [{native :native, mbql :query, query-type :type}]
(case query-type
:native (core/and native (core/not mbql))
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 5af54c71b64aa402c87ff43db761fa47ea0ae012..963bb87b65e643de0ca868840fd06272d2d31d81 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -35,6 +35,7 @@
[permissions :as perms]
[resolve :as resolve]
[resolve-driver :as resolve-driver]
+ [resolve-fields :as resolve-fields]
[results-metadata :as results-metadata]
[source-table :as source-table]
[store :as store]
@@ -101,9 +102,10 @@
cumulative-ags/handle-cumulative-aggregations
results-metadata/record-and-return-metadata!
format-rows/format-rows
- binning/update-binning-strategy
resolve/resolve-middleware
expand/expand-middleware ; ▲▲▲ QUERY EXPANSION POINT ▲▲▲ All functions *above* will see EXPANDED query during PRE-PROCESSING
+ binning/update-binning-strategy
+ resolve-fields/resolve-fields
add-dim/add-remapping
implicit-clauses/add-implicit-clauses
bucket-datetime/auto-bucket-datetime-breakouts
diff --git a/src/metabase/query_processor/interface.clj b/src/metabase/query_processor/interface.clj
index cb8e54032e5be84c081b7a98a57e68eafd0b62a5..95d410dddd43629742b6ecc19857c7a3e925ac3e 100644
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -205,6 +205,7 @@
base-type :- su/FieldType
binning-strategy :- (s/maybe (apply s/enum binning-strategies))
binning-param :- (s/maybe s/Num)
+ binning-opts :- s/Any
fingerprint :- (s/maybe i/Fingerprint)]
nil
:load-ns true
@@ -275,6 +276,7 @@
remapped-to :- (s/maybe s/Str)
field-display-name :- (s/maybe s/Str)
binning-strategy :- (s/maybe (apply s/enum binning-strategies))
+ binning-opts :- s/Any
binning-param :- (s/maybe s/Num)]
nil
:load-ns true)
diff --git a/src/metabase/query_processor/middleware/annotate_and_sort.clj b/src/metabase/query_processor/middleware/annotate_and_sort.clj
index 2c408596225625556b6028c7fdff93b1737696fe..2c63eccfa7b922c5d9153a3a8ce97cebd5b118ef 100644
--- a/src/metabase/query_processor/middleware/annotate_and_sort.clj
+++ b/src/metabase/query_processor/middleware/annotate_and_sort.clj
@@ -1,24 +1,17 @@
(ns metabase.query-processor.middleware.annotate-and-sort
"Middleware for annotating (adding type information to) the results of a query and sorting the columns in the results."
- ;; TODO - `annotate` and `sort` are technically two seperate steps. We should decouple everything so we get a namespace
- ;; structure looking more like:
- ;; `metabase.query-processor.middleware.sort`
- ;; `metabase.query-processor.middleware.annotate`
- ;; `metabase.query-processor.middleware.mbql`
- ;; `metabase.query-processor.middleware.sql`
(:require [metabase.driver :as driver]
[metabase.models.humanization :as humanization]
- [metabase.query-processor
- [annotate :as annotate]
- [util :as qputil]]))
+ [metabase.query-processor.annotate :as annotate]))
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | NATIVE QUERY ANNOTATION |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | NATIVE QUERY ANNOTATION |
+;;; +----------------------------------------------------------------------------------------------------------------+
(defn- infer-column-types
- "Infer the types of columns by looking at the first value for each in the results, and add the relevant information in `:cols`.
- This is used for native queries, which don't have the type information from the original `Field` objects used in the query, which is added to the results by `annotate`."
+ "Infer the types of columns by looking at the first value for each in the results, and add the relevant information in
+ `:cols`. This is used for native queries, which don't have the type information from the original `Field` objects
+ used in the query, which is added to the results by `annotate`."
[{:keys [columns rows], :as results}]
(assoc results
:columns (mapv name columns)
@@ -30,17 +23,16 @@
(nth row i)))
:type/*)}))))
-
-;;; +------------------------------------------------------------------------------------------------------------------------+
-;;; | GENERAL MIDDLEWARE |
-;;; +------------------------------------------------------------------------------------------------------------------------+
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | GENERAL MIDDLEWARE |
+;;; +----------------------------------------------------------------------------------------------------------------+
(defn annotate-and-sort
"Middleware for adding type information to columns returned by running a query, and sorting the columns in the results."
[qp]
- (fn [query]
+ (fn [{query-type :type, :as query}]
(let [results (qp query)]
- (-> (if-not (or (qputil/mbql-query? query)
+ (-> (if-not (or (= query-type :query)
(:annotate? results))
(infer-column-types results)
(annotate/annotate-and-sort query results))
diff --git a/src/metabase/query_processor/middleware/binning.clj b/src/metabase/query_processor/middleware/binning.clj
index 53c333bb23e80575af7357f2860499e1f9185a0f..7f98875b2cdbeb35b0d2f28d75ce77c487272d41 100644
--- a/src/metabase/query_processor/middleware/binning.clj
+++ b/src/metabase/query_processor/middleware/binning.clj
@@ -1,68 +1,91 @@
(ns metabase.query-processor.middleware.binning
+ "Middleware that handles `binning-strategy` Field clauses. This adds a `resolved-options` map to every
+ `binning-strategy` clause that contains the information query processors will need in order to perform binning."
(:require [clojure.math.numeric-tower :refer [ceil expt floor]]
- [clojure.walk :as walk]
[metabase
[public-settings :as public-settings]
[util :as u]]
- [metabase.query-processor.util :as qputil])
- (:import [metabase.query_processor.interface BetweenFilter BinnedField ComparisonFilter]))
-
-(defn- update!
- "Similar to `clojure.core/update` but works on transient maps"
- [^clojure.lang.ITransientAssociative coll k f]
- (assoc! coll k (f (get coll k))))
-
-(defn- filter->field-map
- "A bit of a stateful hack using clojure.walk/prewalk to find any comparison or between filter. This should be replaced
- by a zipper for a more functional/composable approach to this problem."
- [mbql-filter]
- (let [acc (transient {})]
- (walk/prewalk
- (fn [x]
- (when (or (instance? BetweenFilter x)
- (and (instance? ComparisonFilter x)
- (contains? #{:< :> :<= :>=} (:filter-type x))))
- (update! acc (get-in x [:field :field-id]) #(if (seq %)
- (conj % x)
- [x])))
- x)
- mbql-filter)
- (persistent! acc)))
-
-(defn calculate-bin-width
+ [metabase.mbql
+ [schema :as mbql.s]
+ [util :as mbql.u]]
+ [metabase.query-processor.store :as qp.store]
+ [metabase.util
+ [i18n :refer [tru]]
+ [schema :as su]]
+ [schema.core :as s]))
+
+;;; ----------------------------------------------- Extracting Bounds ------------------------------------------------
+
+(def ^:private FieldID->Filters {su/IntGreaterThanZero [mbql.s/Filter]})
+
+(s/defn ^:private filter->field-map :- FieldID->Filters
+ "Find any comparison or `:between` filter and return a map of referenced Field ID -> all the clauses the reference
+ it."
+ [filter-clause :- (s/maybe mbql.s/Filter)]
+ (reduce
+ (partial merge-with concat)
+ {}
+ (for [filter-clause (mbql.u/clause-instances #{:between :< :<= :> :>=} filter-clause)
+ [_ field-id] (mbql.u/clause-instances #{:field-id} filter-clause)]
+ {field-id [filter-clause]})))
+
+(s/defn ^:private extract-bounds :- {:min-value s/Num, :max-value s/Num}
+ "Given query criteria, find a min/max value for the binning strategy using the greatest user specified min value and
+ the smallest user specified max value. When a user specified min or max is not found, use the global min/max for the
+ given field."
+ [field-id :- (s/maybe su/IntGreaterThanZero), fingerprint :- (s/maybe su/Map), field-id->filters :- FieldID->Filters]
+ (let [{global-min :min, global-max :max} (get-in fingerprint [:type :type/Number])
+ filter-clauses (get field-id->filters field-id)
+ ;; [:between <field> <min> <max>] or [:< <field> <x>]
+ user-maxes (for [[clause-name :as filter-clause] filter-clauses
+ :when (#{:< :<= :between} clause-name)]
+ (last filter-clause))
+ user-mins (for [[clause-name :as filter-clause] filter-clauses
+ :when (#{:> :>= :between} clause-name)]
+ (if (= :between clause-name)
+ (nth filter-clause 2)
+ (last filter-clause)))
+ min-value (or (when (seq user-mins)
+ (apply max user-mins))
+ global-min)
+ max-value (or (when (seq user-maxes)
+ (apply min user-maxes))
+ global-max)]
+ (when-not (and min-value max-value)
+ (throw (Exception. (str (tru "Unable to bin Field without a min/max value")))))
+ {:min-value min-value, :max-value max-value}))
+
+
+;;; ------------------------------------------ Calculating resolved options ------------------------------------------
+
+(s/defn ^:private calculate-bin-width :- s/Num
"Calculate bin width required to cover interval [`min-value`, `max-value`] with `num-bins`."
- [min-value max-value num-bins]
+ [min-value :- s/Num, max-value :- s/Num, num-bins :- su/IntGreaterThanZero]
(u/round-to-decimals 5 (/ (- max-value min-value)
num-bins)))
-(defn calculate-num-bins
+(s/defn ^:private calculate-num-bins :- su/IntGreaterThanZero
"Calculate number of bins of width `bin-width` required to cover interval [`min-value`, `max-value`]."
- [min-value max-value bin-width]
+ [min-value :- s/Num, max-value :- s/Num, bin-width :- (s/constrained s/Num (complement neg?) "number >= 0")]
(long (Math/ceil (/ (- max-value min-value)
- bin-width))))
+ bin-width))))
-(defn- extract-bounds
- "Given query criteria, find a min/max value for the binning strategy using the greatest user specified min value and
- the smallest user specified max value. When a user specified min or max is not found, use the global min/max for the
- given field."
- [{:keys [field-id fingerprint]} field-filter-map]
- (let [{global-min :min, global-max :max} (get-in fingerprint [:type :type/Number])
- user-maxes (for [{:keys [filter-type] :as query-filter} (get field-filter-map field-id)
- :when (contains? #{:< :<= :between} filter-type)]
- (if (= :between filter-type)
- (get-in query-filter [:max-val :value])
- (get-in query-filter [:value :value])))
- user-mins (for [{:keys [filter-type] :as query-filter} (get field-filter-map field-id)
- :when (contains? #{:> :>= :between} filter-type)]
- (if (= :between filter-type)
- (get-in query-filter [:min-val :value])
- (get-in query-filter [:value :value])))]
- [(or (when (seq user-mins)
- (apply max user-mins))
- global-min)
- (or (when (seq user-maxes)
- (apply min user-maxes))
- global-max)]))
+(s/defn ^:private resolve-default-strategy :- [(s/one (s/enum :bin-width :num-bins) "strategy")
+ (s/one {:bin-width s/Num, :num-bins su/IntGreaterThanZero} "opts")]
+ "Determine the approprate strategy & options to use when `:default` strategy was specified."
+ [metadata :- {:special_type su/FieldType, s/Any s/Any}, min-value :- s/Num, max-value :- s/Num]
+ (if (isa? (:special_type metadata) :type/Coordinate)
+ (let [bin-width (public-settings/breakout-bin-width)]
+ [:bin-width
+ {:bin-width bin-width
+ :num-bins (calculate-num-bins min-value max-value bin-width)}])
+ (let [num-bins (public-settings/breakout-bins-num)]
+ [:num-bins
+ {:num-bins num-bins
+ :bin-width (calculate-bin-width min-value max-value num-bins)}])))
+
+
+;;; ------------------------------------- Humanized binning with nicer-breakout --------------------------------------
(defn- ceil-to
[precision x]
@@ -76,8 +99,8 @@
(def ^:private ^:const pleasing-numbers [1 1.25 2 2.5 3 5 7.5 10])
-(defn- nicer-bin-width
- [min-value max-value num-bins]
+(s/defn ^:private nicer-bin-width
+ [min-value :- s/Num, max-value :- s/Num, num-bins :- su/IntGreaterThanZero]
(let [min-bin-width (calculate-bin-width min-value max-value num-bins)
scale (expt 10 (u/order-of-magnitude min-bin-width))]
(->> pleasing-numbers
@@ -100,69 +123,75 @@
(drop-while (partial apply not=))
ffirst)))
-(def ^{:arglists '([binned-field])} nicer-breakout
+(s/defn ^:private nicer-breakout* :- mbql.s/ResolvedBinningStrategyOptions
"Humanize binning: extend interval to start and end on a \"nice\" number and, when number of bins is fixed, have a
\"nice\" step (bin width)."
- (fixed-point
- (fn
- [{:keys [min-value max-value bin-width num-bins strategy] :as binned-field}]
- (let [bin-width (if (= strategy :num-bins)
- (nicer-bin-width min-value max-value num-bins)
- bin-width)
- [min-value max-value] (nicer-bounds min-value max-value bin-width)]
- (-> binned-field
- (assoc :min-value min-value
- :max-value max-value
- :num-bins (if (= strategy :num-bins)
- num-bins
- (calculate-num-bins min-value max-value bin-width))
- :bin-width bin-width))))))
-
-(defn- resolve-default-strategy [{:keys [strategy field]} min-value max-value]
- (if (isa? (:special-type field) :type/Coordinate)
- (let [bin-width (public-settings/breakout-bin-width)]
- {:strategy :bin-width
- :bin-width bin-width
- :num-bins (calculate-num-bins min-value max-value bin-width)})
- (let [num-bins (public-settings/breakout-bins-num)]
- {:strategy :num-bins
- :num-bins num-bins
- :bin-width (calculate-bin-width min-value max-value num-bins)})))
-
-(defn- update-binned-field
- "Given a field, resolve the binning strategy (either provided or found if default is specified) and calculate the
- number of bins and bin width for this file. `filter-field-map` contains related criteria that could narrow the
- domain for the field."
- [{:keys [field num-bins strategy bin-width] :as binned-field} filter-field-map]
- (let [[min-value max-value] (extract-bounds field filter-field-map)]
- (when-not (and min-value max-value)
- (throw (Exception. (format "Unable to bin field '%s' with id '%s' without a min/max value"
- (get-in binned-field [:field :field-name])
- (get-in binned-field [:field :field-id])))))
- (let [resolved-binned-field (merge binned-field
- {:min-value min-value :max-value max-value}
- (case strategy
-
- :num-bins
- {:bin-width (calculate-bin-width min-value max-value num-bins)}
-
- :bin-width
- {:num-bins (calculate-num-bins min-value max-value bin-width)}
-
- :default
- (resolve-default-strategy binned-field min-value max-value)))]
- ;; Bail out and use unmodifed version if we can't converge on a
- ;; nice version.
- (or (nicer-breakout resolved-binned-field) resolved-binned-field))))
+ [strategy :- mbql.s/BinningStrategyName
+ {:keys [min-value max-value bin-width num-bins]} :- mbql.s/ResolvedBinningStrategyOptions]
+ (let [bin-width (if (= strategy :num-bins)
+ (nicer-bin-width min-value max-value num-bins)
+ bin-width)
+ [min-value max-value] (nicer-bounds min-value max-value bin-width)]
+ {:min-value min-value
+ :max-value max-value
+ :num-bins (if (= strategy :num-bins)
+ num-bins
+ (calculate-num-bins min-value max-value bin-width))
+ :bin-width bin-width}))
+
+(s/defn ^:private nicer-breakout :- (s/maybe mbql.s/ResolvedBinningStrategyOptions)
+ [strategy :- mbql.s/BinningStrategyName, opts :- mbql.s/ResolvedBinningStrategyOptions]
+ (let [f (partial nicer-breakout* strategy)]
+ ((fixed-point f) opts)))
+
+
+;;; -------------------------------------------- Adding resolved options ---------------------------------------------
+
+(defn- resolve-options [strategy strategy-param metadata min-value max-value]
+ (case strategy
+ :num-bins
+ [:num-bins
+ {:num-bins strategy-param
+ :bin-width (calculate-bin-width min-value max-value strategy-param)}]
+
+ :bin-width
+ [:bin-width
+ {:bin-width strategy-param
+ :num-bins (calculate-num-bins min-value max-value strategy-param)}]
+
+ :default
+ (resolve-default-strategy metadata min-value max-value)))
+
+(s/defn ^:private update-binned-field :- mbql.s/binning-strategy
+ "Given a `binning-strategy` clause, resolve the binning strategy (either provided or found if default is specified)
+ and calculate the number of bins and bin width for this field. `field-id->filters` contains related criteria that
+ could narrow the domain for the field. This info is saved as part of each `binning-strategy` clause."
+ [query, field-id->filters :- FieldID->Filters, [_ field-clause strategy strategy-param] :- mbql.s/binning-strategy]
+ (let [field-id-or-name (mbql.u/field-clause->id-or-literal field-clause)
+ metadata (if (integer? field-id-or-name)
+ (qp.store/field field-id-or-name)
+ (some (fn [metadata]
+ (when (= (:name metadata) field-id-or-name)
+ metadata))
+ (:source-metadata query)))
+ {:keys [min-value max-value]
+ :as min-max} (extract-bounds (when (integer? field-id-or-name) field-id-or-name)
+ (:fingerprint metadata)
+ field-id->filters)
+ [new-strategy resolved-options] (resolve-options strategy strategy-param metadata min-value max-value)
+ resolved-options (merge min-max resolved-options)]
+ ;; Bail out and use unmodifed version if we can't converge on a nice version.
+ [:binning-strategy field-clause new-strategy strategy-param (or (nicer-breakout new-strategy resolved-options)
+ resolved-options)]))
+
+
+(defn- update-binning-strategy* [query]
+ (let [field-id->filters (filter->field-map (get-in query [:query :filter]))]
+ (mbql.u/replace-clauses-in query [:query] :binning-strategy (partial update-binned-field query field-id->filters))))
(defn update-binning-strategy
"When a binned field is found, it might need to be updated if a relevant query criteria affects the min/max value of
the binned field. This middleware looks for that criteria, then updates the related min/max values and calculates
the bin-width based on the criteria values (or global min/max information)."
[qp]
- (fn [query]
- (let [filter-field-map (filter->field-map (get-in query [:query :filter]))]
- (qp
- (qputil/postwalk-pred #(instance? BinnedField %)
- #(update-binned-field % filter-field-map)
- query)))))
+ (comp qp update-binning-strategy*))
diff --git a/src/metabase/query_processor/middleware/expand.clj b/src/metabase/query_processor/middleware/expand.clj
index 91222d3d45d565ec7cfc7da5b34e633c26ed32f8..b7ba2be89ebc99d7c222d2fc8a09e8ff2e6ba64c 100644
--- a/src/metabase/query_processor/middleware/expand.clj
+++ b/src/metabase/query_processor/middleware/expand.clj
@@ -161,12 +161,12 @@
(s/defn ^:deprecated ^:private ag-with-field :- i/Aggregation [ag-type f]
(i/map->AggregationWithField {:aggregation-type ag-type, :field (field-or-expression f)}))
-(def ^:ql ^{:arglists '([f])} avg "Aggregation clause. Return the average value of F." (partial ag-with-field :avg))
-(def ^:ql ^{:arglists '([f])} distinct "Aggregation clause. Return the number of distinct values of F." (partial ag-with-field :distinct))
-(def ^:ql ^{:arglists '([f])} sum "Aggregation clause. Return the sum of the values of F." (partial ag-with-field :sum))
-(def ^:ql ^{:arglists '([f])} cum-sum "Aggregation clause. Return the cumulative sum of the values of F." (partial ag-with-field :cumulative-sum))
-(def ^:ql ^{:arglists '([f])} min "Aggregation clause. Return the minimum value of F." (partial ag-with-field :min))
-(def ^:ql ^{:arglists '([f])} max "Aggregation clause. Return the maximum value of F." (partial ag-with-field :max))
+(def ^:ql ^:deprecated ^{:arglists '([f])} avg "Aggregation clause. Return the average value of F." (partial ag-with-field :avg))
+(def ^:ql ^:deprecated ^{:arglists '([f])} distinct "Aggregation clause. Return the number of distinct values of F." (partial ag-with-field :distinct))
+(def ^:ql ^:deprecated ^{:arglists '([f])} sum "Aggregation clause. Return the sum of the values of F." (partial ag-with-field :sum))
+(def ^:ql ^:deprecated ^{:arglists '([f])} cum-sum "Aggregation clause. Return the cumulative sum of the values of F." (partial ag-with-field :cumulative-sum))
+(def ^:ql ^:deprecated ^{:arglists '([f])} min "Aggregation clause. Return the minimum value of F." (partial ag-with-field :min))
+(def ^:ql ^:deprecated ^{:arglists '([f])} max "Aggregation clause. Return the maximum value of F." (partial ag-with-field :max))
(defn ^:deprecated ^:ql stddev
"Aggregation clause. Return the standard deviation of values of F.
@@ -219,17 +219,17 @@
(s/defn ^:deprecated ^: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]]
+ ([f strategy-name & [strategy-param resolved-options]]
(let [strategy (qputil/normalize-token strategy-name)
field (field f)]
- (assoc field :binning-strategy strategy, :binning-param strategy-param))))
+ (assoc field :binning-strategy strategy, :binning-param strategy-param, :binning-opts resolved-options))))
(defn- ^:deprecated fields-list-clause
([_ query] query)
([k query & fields] (assoc query k (mapv field fields))))
-(def ^:ql ^{:arglists '([query & fields])} breakout "Specify which fields to breakout by." (partial fields-list-clause :breakout))
-(def ^:ql ^{:arglists '([query & fields])} fields "Specify which fields to return." (partial fields-list-clause :fields))
+(def ^:ql ^:deprecated ^{:arglists '([query & fields])} breakout "Specify which fields to breakout by." (partial fields-list-clause :breakout))
+(def ^:ql ^:deprecated ^{:arglists '([query & fields])} fields "Specify which fields to return." (partial fields-list-clause :fields))
;;; ## filter
@@ -241,8 +241,8 @@
([compound-type, subclause :- i/Filter, & more :- [i/Filter]]
(i/map->CompoundFilter {:compound-type compound-type, :subclauses (vec (cons subclause more))})))
-(def ^:ql ^{:arglists '([& subclauses])} and "Filter subclause. Return results that satisfy *all* SUBCLAUSES." (partial compound-filter :and))
-(def ^:ql ^{:arglists '([& subclauses])} or "Filter subclause. Return results that satisfy *any* of the SUBCLAUSES." (partial compound-filter :or))
+(def ^:ql ^:deprecated ^{:arglists '([& subclauses])} and "Filter subclause. Return results that satisfy *all* SUBCLAUSES." (partial compound-filter :and))
+(def ^:ql ^:deprecated ^{:arglists '([& subclauses])} or "Filter subclause. Return results that satisfy *any* of the SUBCLAUSES." (partial compound-filter :or))
(s/defn ^:deprecated ^:private equality-filter :- i/Filter
([filter-type _ f v]
@@ -251,7 +251,7 @@
(apply compound-fn (for [v (cons v more)]
(equality-filter filter-type compound-fn f v)))))
-(def ^:ql ^{:arglists '([f v & more])} =
+(def ^:ql ^:deprecated ^{:arglists '([f v & more])} =
"Filter subclause. With a single value, return results where F == V. With two or more values, return results where F
matches *any* of the values (i.e.`IN`)
@@ -259,7 +259,7 @@
(= f v1 v2) ; same as (or (= f v1) (= f v2))"
(partial equality-filter := or))
-(def ^:ql ^{:arglists '([f v & more])} !=
+(def ^:ql ^:deprecated ^{:arglists '([f v & more])} !=
"Filter subclause. With a single value, return results where F != V. With two or more values, return results where F
does not match *any* of the values (i.e. `NOT IN`)
@@ -273,10 +273,10 @@
(s/defn ^:deprecated ^:private comparison-filter :- ComparisonFilter [filter-type f v]
(i/map->ComparisonFilter {:filter-type filter-type, :field (field f), :value (value f v)}))
-(def ^:ql ^{:arglists '([f v])} < "Filter subclause. Return results where F is less than V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :<))
-(def ^:ql ^{:arglists '([f v])} <= "Filter subclause. Return results where F is less than or equal to V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :<=))
-(def ^:ql ^{:arglists '([f v])} > "Filter subclause. Return results where F is greater than V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :>))
-(def ^:ql ^{:arglists '([f v])} >= "Filter subclause. Return results where F is greater than or equal to V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :>=))
+(def ^:ql ^:deprecated ^{:arglists '([f v])} < "Filter subclause. Return results where F is less than V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :<))
+(def ^:ql ^:deprecated ^{:arglists '([f v])} <= "Filter subclause. Return results where F is less than or equal to V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :<=))
+(def ^:ql ^:deprecated ^{:arglists '([f v])} > "Filter subclause. Return results where F is greater than V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :>))
+(def ^:ql ^:deprecated ^{:arglists '([f v])} >= "Filter subclause. Return results where F is greater than or equal to V. V must be orderable, i.e. a number or datetime." (partial comparison-filter :>=))
(s/defn ^:deprecated ^:ql between :- BetweenFilter
"Filter subclause. Return results where F is between MIN and MAX. MIN and MAX must be orderable, i.e. numbers or datetimes.
@@ -309,17 +309,17 @@
:value (value f s)
:case-sensitive? (get options-map :case-sensitive true)})))
-(def ^:ql ^{:arglists '([f s] [f s options-map])} starts-with
+(def ^:ql ^:deprecated ^{:arglists '([f s] [f s options-map])} starts-with
"Filter subclause. Return results where F starts with the string S. By default, is case-sensitive, but you may pass an
`options-map` with `{:case-sensitive false}` for case-insensitive searches."
(partial string-filter :starts-with))
-(def ^:ql ^{:arglists '([f s] [f s options-map])} contains
+(def ^:ql ^:deprecated ^{:arglists '([f s] [f s options-map])} contains
"Filter subclause. Return results where F contains the string S. By default, is case-sensitive, but you may pass an
`options-map` with `{:case-sensitive false}` for case-insensitive searches."
(partial string-filter :contains))
-(def ^:ql ^{:arglists '([f s] [f s options-map])} ends-with
+(def ^:ql ^:deprecated ^{:arglists '([f s] [f s options-map])} ends-with
"Filter subclause. Return results where F ends with with the string S. By default, is case-sensitive, but you may pass
an `options-map` with `{:case-sensitive false}` for case-insensitive searches."
(partial string-filter :ends-with))
@@ -353,7 +353,7 @@
(> field max-val)))
(i/strict-map->NotFilter {:compound-type :not, :subclause clause})))))
-(def ^:ql ^{:arglists '([f s]), :added "0.15.0"} does-not-contain
+(def ^:ql ^:deprecated ^{:arglists '([f s]), :added "0.15.0"} does-not-contain
"Filter subclause. Return results where F does not start with the string S."
(comp not contains))
@@ -421,13 +421,13 @@
(update f :datetime-unit (fn [unit]
(core/or unit :default)))))})
-(def ^:ql ^{:arglists '([field])} asc
+(def ^:ql ^:deprecated ^{:arglists '([field])} asc
"`order-by` subclause. Specify that results should be returned in ascending order for Field or AgRef F.
(order-by {} (asc 100))"
(partial order-by-subclause :ascending))
-(def ^:ql ^{:arglists '([field])} desc
+(def ^:ql ^:deprecated ^{:arglists '([field])} desc
"`order-by` subclause. Specify that results should be returned in ascending order for Field or AgRef F.
(order-by {} (desc 100))"
@@ -503,10 +503,10 @@
(float arg) ; convert args to floats so things like 5 / 10 -> 0.5 instead of 0
arg)))}))
-(def ^:ql ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} + "Arithmetic addition function." (partial expression-fn :+))
-(def ^:ql ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} - "Arithmetic subtraction function." (partial expression-fn :-))
-(def ^:ql ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} * "Arithmetic multiplication function." (partial expression-fn :*))
-(def ^:ql ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} / "Arithmetic division function." (partial expression-fn :/))
+(def ^:ql ^:deprecated ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} + "Arithmetic addition function." (partial expression-fn :+))
+(def ^:ql ^:deprecated ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} - "Arithmetic subtraction function." (partial expression-fn :-))
+(def ^:ql ^:deprecated ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} * "Arithmetic multiplication function." (partial expression-fn :*))
+(def ^:ql ^:deprecated ^{:arglists '([rvalue1 rvalue2 & more]), :added "0.17.0"} / "Arithmetic division function." (partial expression-fn :/))
;;; Metric & Segment handlers
diff --git a/src/metabase/query_processor/middleware/fetch_source_query.clj b/src/metabase/query_processor/middleware/fetch_source_query.clj
index 54f551ab999dde626240c747c56670ffa966d0c4..8009900b56eb0b2eedc1ad0808e23eaf25a2358b 100644
--- a/src/metabase/query_processor/middleware/fetch_source_query.clj
+++ b/src/metabase/query_processor/middleware/fetch_source_query.clj
@@ -5,9 +5,10 @@
[metabase.mbql.schema :as mbql.s]
[metabase.query-processor.interface :as i]
[metabase.util :as u]
- [metabase.util.i18n :refer [trs]]
+ [metabase.util.i18n :refer [trs tru]]
[schema.core :as s]
- [toucan.db :as db]))
+ [toucan.db :as db]
+ [metabase.mbql.normalize :as normalize]))
(defn- trim-query
"Native queries can have trailing SQL comments. This works when executed directly, but when we use the query in a
@@ -32,22 +33,22 @@
{:native (trim-query card-id (:query native))}
(when (seq (:template-tags native))
{:template-tags (:template-tags native)})))
- (throw (Exception. (str "Missing source query in Card " card-id))))
+ (throw (Exception. (str (tru "Missing source query in Card {0}" 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)
- :result_metadata (:result_metadata card))))
+ :source-metadata (normalize/normalize-fragment [:source-metadata] (:result_metadata card)))))
(defn- source-table-str->source-query
- "Given a SOURCE-TABLE-STR like `card__100` return the appropriate source query."
+ "Given a `source-table-str` like `card__100` return the appropriate source query."
[source-table-str]
(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/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)))))))
+ (log/info (trs "Fetched source query from Card {0}:" card-id-str)
+ "\n"
+ ;; 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
@@ -63,9 +64,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 :result_metadata)
+ :source-query (dissoc source-query :database :source-metadata)
:database (:database source-query)
- :result_metadata (:result_metadata source-query))))))
+ :source-metadata (:source-metadata source-query))))))
(s/defn ^:private fetch-source-query* :- mbql.s/Query
[{inner-query :query, :as outer-query} :- mbql.s/Query]
@@ -75,11 +76,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 :result_metadata)}
+ {:query (dissoc expanded-inner-query :database :source-metadata)}
(when-let [database (:database expanded-inner-query)]
{:database database})
- (when-let [result-metadata (:result_metadata expanded-inner-query)]
- {:result_metadata result-metadata})))))
+ (when-let [source-metadata (:source-metadata expanded-inner-query)]
+ {:source-metadata source-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 8035dda23f20f72c75283553f017ad7fb6db7bfb..994a5d65a3165d4f5b7a4b0951d9d4127dac8e2e 100644
--- a/src/metabase/query_processor/middleware/resolve.clj
+++ b/src/metabase/query_processor/middleware/resolve.clj
@@ -1,4 +1,4 @@
-(ns metabase.query-processor.middleware.resolve
+(ns ^:deprecated metabase.query-processor.middleware.resolve
"Resolve references to `Fields`, `Tables`, and `Databases` in an expanded query dictionary. During the `expand`
phase of the Query Processor, forms like `[:field-id 10]` are replaced with placeholder objects of types like
`FieldPlaceholder` or similar. During this phase, we'll take those placeholder objects and fetch information from
@@ -163,21 +163,10 @@
;;; ----------------------------------------------- FIELD PLACEHOLDER ------------------------------------------------
-(defn- resolve-binned-field [binning-strategy binning-param field]
- (let [binned-field (i/map->BinnedField {:field field
- :strategy binning-strategy})]
- (case binning-strategy
- :num-bins
- (assoc binned-field :num-bins binning-param)
-
- :bin-width
- (assoc binned-field :bin-width binning-param)
-
- :default
- binned-field
-
- :else
- (throw (Exception. (format "Unregonized binning strategy '%s'" binning-strategy))))))
+(defn- resolve-binned-field [binning-strategy binning-param binning-opts field]
+ (i/map->BinnedField (merge {:field field
+ :strategy binning-strategy}
+ binning-opts)))
(defn- merge-non-nils
"Like `clojure.core/merge` but only merges non-nil values"
@@ -186,7 +175,7 @@
(defn- field-ph-resolve-field
"Attempt to resolve the `Field` for a `FieldPlaceholder`. Return a resolved `Field` or `DateTimeField`."
- [{:keys [field-id datetime-unit binning-strategy binning-param], :as this} field-id->field]
+ [{:keys [field-id datetime-unit binning-strategy binning-param binning-opts], :as this} field-id->field]
(if-let [{:keys [base-type special-type], :as field} (some-> (field-id->field field-id)
convert-db-field
(merge-non-nils (select-keys this [:fk-field-id :remapped-from :remapped-to :field-display-name])))]
@@ -202,7 +191,7 @@
(i/map->TimeField {:field field})
binning-strategy
- (resolve-binned-field binning-strategy binning-param field)
+ (resolve-binned-field binning-strategy binning-param binning-opts field)
:else field)
;; If that fails just return ourselves as-is
@@ -441,12 +430,12 @@
(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))]
+ `source-metadata` attached to the query (typically from the `Card` of a nested query)."
+ [{:keys [source-metadata] :as expanded-query-dict}]
+ (let [name->fingerprint (zipmap (map :name source-metadata)
+ (map :fingerprint source-metadata))]
(qputil/postwalk-pred #(instance? FieldLiteral %)
- (fn [{:keys [binning-strategy binning-param] :as node}]
+ (fn [{:keys [binning-strategy binning-param binning-opts] :as node}]
(let [fingerprint (get name->fingerprint (:field-name node))
node-with-print (assoc node :fingerprint fingerprint)]
(cond
@@ -455,7 +444,7 @@
(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)
+ (resolve-binned-field binning-strategy binning-param binning-opts node-with-print)
:else
node-with-print)))
@@ -464,7 +453,7 @@
;;; ------------------------------------------------ PUBLIC INTERFACE ------------------------------------------------
-(defn resolve-fields-if-needed
+(defn ^:deprecated resolve-fields-if-needed
"Resolves any unresolved fields found in `fields`. Will just return resolved fields with no changes."
[fields]
(let [fields-to-resolve (map unresolved-field-id fields)]
@@ -473,7 +462,7 @@
(map #(resolve-field % field-id->field) fields)
fields)))
-(s/defn resolve :- su/Map
+(s/defn ^:deprecated resolve :- su/Map
"Resolve placeholders by fetching `Fields`, `Databases`, and `Tables` that are referred to in EXPANDED-QUERY-DICT."
[expanded-query-dict :- su/Map]
(some-> expanded-query-dict
@@ -482,7 +471,7 @@
resolve-field-literals
resolve-tables))
-(defn resolve-middleware
+(defn ^:deprecated resolve-middleware
"Wraps the `resolve` function in a query-processor middleware"
[qp]
(fn [{database-id :database, query-type :type, :as query}]
diff --git a/src/metabase/query_processor/middleware/resolve_fields.clj b/src/metabase/query_processor/middleware/resolve_fields.clj
new file mode 100644
index 0000000000000000000000000000000000000000..129c39e4a5b030db5fc78050f8b9d1415ef84642
--- /dev/null
+++ b/src/metabase/query_processor/middleware/resolve_fields.clj
@@ -0,0 +1,21 @@
+(ns metabase.query-processor.middleware.resolve-fields
+ "Middleware that resolves the Fields referenced by a query."
+ (:require [metabase.mbql.util :as mbql.u]
+ [metabase.models.field :refer [Field]]
+ [metabase.query-processor.store :as qp.store]
+ [metabase.util :as u]
+ [toucan.db :as db]))
+
+(defn- resolve-fields* [{mbql-inner-query :query, :as query}]
+ (u/prog1 query
+ (when-let [field-ids (seq (map second (mbql.u/clause-instances :field-id mbql-inner-query)))]
+ ;; Just fetch the entire object for right now. We can pare this down at a later date
+ ;; TODO - figure out which Fields we need and only fetch those
+ (doseq [field (db/select Field :id [:in (set field-ids)])]
+ (qp.store/store-field! field)))))
+
+(defn resolve-fields
+ "Fetch the Fields referenced by `:field-id` clauses in a query and store them in the Query Processor Store for the
+ duration of the Query Execution."
+ [qp]
+ (comp qp resolve-fields*))
diff --git a/src/metabase/query_processor/middleware/source_table.clj b/src/metabase/query_processor/middleware/source_table.clj
index dfd77808c5fe0fb58f691810f97ea3402465bc99..76908209eaa62c76fb75a6b0433ea8649fa53aca 100644
--- a/src/metabase/query_processor/middleware/source_table.clj
+++ b/src/metabase/query_processor/middleware/source_table.clj
@@ -8,7 +8,8 @@
(defn- resolve-source-table [query]
(when-let [source-table-id (mbql.u/query->source-table-id query)]
(let [source-table (or (db/select-one [Table :schema :name :id], :id source-table-id)
- (throw (Exception. (str (trs "Cannot run query: could not find source table {0}." source-table-id)))))]
+ (throw (Exception. (str (trs "Cannot run query: could not find source table {0}."
+ source-table-id)))))]
(qp.store/store-table! source-table)))
query)
diff --git a/src/metabase/query_processor/store.clj b/src/metabase/query_processor/store.clj
index 2899160499510d4684129c79c2a086f815025f01..758db871040b44110d5d18b8012dc6c070fbb715 100644
--- a/src/metabase/query_processor/store.clj
+++ b/src/metabase/query_processor/store.clj
@@ -12,12 +12,12 @@
Of course, it would be entirely possible to call `(Field 10)` every time you needed information about that Field,
but fetching all Fields in a single pass and storing them for reuse is dramatically more efficient than fetching
those Fields potentially dozens of times in a single query execution."
- (:require [schema.core :as s]
- [metabase.models
+ (:require [metabase.models
[field :refer [Field]]
[table :refer [Table]]]
[metabase.util :as u]
- [metabase.util.schema :as su]))
+ [metabase.util.schema :as su]
+ [schema.core :as s]))
;;; ---------------------------------------------- Setting up the Store ----------------------------------------------
@@ -41,8 +41,6 @@
;; TODO - DATABASE ??
(def ^:private TableInstanceWithRequiredStoreKeys
- "Schema for Tables stored in the QP store (must be a `TableInstance` with at least the `:id`, `:schema`, and `:name`
- keys)."
(s/both
(class Table)
{:id su/IntGreaterThanZero ; TODO - what's the point of storing ID if it's already the key?
@@ -50,6 +48,18 @@
:name su/NonBlankString
s/Any s/Any}))
+(def ^:private FieldInstanceWithRequiredStorekeys
+ (s/both
+ (class Field)
+ {:id su/IntGreaterThanZero
+ :name su/NonBlankString
+ :display_name su/NonBlankString
+ :description (s/maybe s/Str)
+ :base_type su/FieldType
+ :special_type (s/maybe su/FieldType)
+ :fingerprint (s/maybe su/Map)
+ s/Any s/Any}))
+
;;; ------------------------------------------ Saving objects in the Store -------------------------------------------
@@ -62,15 +72,9 @@
(s/defn store-field!
"Store a `field` in the QP Store for the duration of the current query execution. Throws an Exception if Field is
invalid or doesn't have all required keys."
- [field :- (class Field)]
+ [field :- FieldInstanceWithRequiredStorekeys]
(swap! *store* assoc-in [:fields (u/get-id field)] field))
-(s/defn store-field-literal!
- "Like `store-field!`, but designed for storing Fields referred to by `:field-literal` clauses, i.e. by name instead of
- by ID."
- [field-literal :- su/NonBlankString, field :- (class Field)]
- (swap! *store* assoc-in [:field-literals field-literal] field))
-
;;; ---------------------------------------- Fetching objects from the Store -----------------------------------------
@@ -79,12 +83,7 @@
[table-id :- su/IntGreaterThanZero]
(get-in @*store* [:tables table-id]))
-(s/defn field :- (class Field)
+(s/defn field :- FieldInstanceWithRequiredStorekeys
"Fetch Field with `field-id` from the QP Store. Throws an Exception if valid item is not returned."
[field-id :- su/IntGreaterThanZero]
(get-in @*store* [:fields field-id]))
-
-(s/defn field-liteal :- (class Field)
- "Fetch Field named by `field-literal` from the QP Store. Throws an Exception if valid item is not returned."
- [field-literal :- su/NonBlankString]
- (get-in @*store* [:field-literals field-literal]))
diff --git a/test/metabase/mbql/normalize_test.clj b/test/metabase/mbql/normalize_test.clj
index 6b7a9cacc958b6247f8d05210096173242cc6c23..9a648a1b41c45fca1aca688fc41d24ef3eac1033 100644
--- a/test/metabase/mbql/normalize_test.clj
+++ b/test/metabase/mbql/normalize_test.clj
@@ -748,3 +748,27 @@
:query {:source-table 1}
:parameters [{:type "id", :target ["dimension" ["fk->" 3265 4575]], :value ["field-id"]}
{:type "date/all-options", :target ["dimension" ["field-id" 3270]], :value "thismonth"}]}))
+
+;; make sure source-metadata gets normalized the way we'd expect (just the type names in this case)
+(expect
+ {:source-metadata
+ [{:name "name"
+ :display_name "Name"
+ :base_type :type/Text
+ :special_type :type/Name
+ :fingerprint {:global {:distinct-count 100}
+ :type {:type/Text {:percent-json 0.0
+ :percent-url 0.0
+ :percent-email 0.0
+ :average-length 15.63}}}}]}
+ (normalize/normalize
+ {:source-metadata [{:name "name"
+ :display_name "Name"
+ :description nil
+ :base_type "type/Text"
+ :special_type "type/Name"
+ :fingerprint {"global" {"distinct-count" 100}
+ "type" {"type/Text" {"percent-json" 0.0
+ "percent-url" 0.0
+ "percent-email" 0.0
+ "average-length" 15.63}}}}]}))
diff --git a/test/metabase/query_processor/middleware/binning_test.clj b/test/metabase/query_processor/middleware/binning_test.clj
index 9447b83a6a6676eb404b0564bf269dfacd1a07f4..0b10ef19668ef6d293056c1397eda3e09af8270f 100644
--- a/test/metabase/query_processor/middleware/binning_test.clj
+++ b/test/metabase/query_processor/middleware/binning_test.clj
@@ -1,25 +1,28 @@
(ns metabase.query-processor.middleware.binning-test
(:require [expectations :refer [expect]]
- [metabase.query-processor.middleware
- [binning :as binning]
- [expand :as ql]]))
+ [metabase.models.field :as field :refer [Field]]
+ [metabase.query-processor.middleware.binning :as binning]
+ [metabase.query-processor.store :as qp.store]
+ [metabase.test.data :as data]
+ [metabase.util :as u]
+ [toucan.util.test :as tt]))
(expect
{}
- (#'binning/filter->field-map (ql/and
- (ql/= (ql/field-id 1) 10)
- (ql/= (ql/field-id 2) 10))))
+ (#'binning/filter->field-map [:and
+ [:= [:field-id 1] 10]
+ [:= [:field-id 2] 10]]))
(expect
- {1 [(ql/< (ql/field-id 1) 10) (ql/> (ql/field-id 1) 1)]
- 2 [(ql/> (ql/field-id 2) 20) (ql/< (ql/field-id 2) 10)]
- 3 [(ql/between (ql/field-id 3) 5 10)]}
- (#'binning/filter->field-map (ql/and
- (ql/< (ql/field-id 1) 10)
- (ql/> (ql/field-id 1) 1)
- (ql/> (ql/field-id 2) 20)
- (ql/< (ql/field-id 2) 10)
- (ql/between (ql/field-id 3) 5 10))))
+ {1 [[:< [:field-id 1] 10] [:> [:field-id 1] 1]]
+ 2 [[:> [:field-id 2] 20] [:< [:field-id 2] 10]]
+ 3 [[:between [:field-id 3] 5 10]]}
+ (#'binning/filter->field-map [:and
+ [:< [:field-id 1] 10]
+ [:> [:field-id 1] 1]
+ [:> [:field-id 2] 20]
+ [:< [:field-id 2] 10]
+ [:between [:field-id 3] 5 10]]))
(expect
[[1.0 1.0 1.0]
@@ -34,48 +37,95 @@
[(#'binning/nicer-bin-width 27 135 8)
(#'binning/nicer-bin-width -0.0002 10000.34 8)])
-(def ^:private test-min-max-field
- {:field-id 1 :fingerprint {:type {:type/Number {:min 100 :max 1000}}}})
+(def ^:private test-min-max-fingerprint
+ {:type {:type/Number {:min 100 :max 1000}}})
(expect
- [1 10]
- (#'binning/extract-bounds test-min-max-field
- {1 [(ql/> (ql/field-id 1) 1) (ql/< (ql/field-id 1) 10)]}))
+ {:min-value 1, :max-value 10}
+ (#'binning/extract-bounds 1
+ test-min-max-fingerprint
+ {1 [[:> [:field-id 1] 1] [:< [:field-id 1] 10]]}))
(expect
- [1 10]
- (#'binning/extract-bounds test-min-max-field
- {1 [(ql/between (ql/field-id 1) 1 10)]}))
+ {:min-value 1, :max-value 10}
+ (#'binning/extract-bounds 1
+ test-min-max-fingerprint
+ {1 [[:between [:field-id 1] 1 10]]}))
(expect
- [100 1000]
- (#'binning/extract-bounds test-min-max-field
+ {:min-value 100, :max-value 1000}
+ (#'binning/extract-bounds 1
+ test-min-max-fingerprint
{}))
(expect
- [500 1000]
- (#'binning/extract-bounds test-min-max-field
- {1 [(ql/> (ql/field-id 1) 500)]}))
+ {:min-value 500, :max-value 1000}
+ (#'binning/extract-bounds 1
+ test-min-max-fingerprint
+ {1 [[:> [:field-id 1] 500]]}))
(expect
- [100 500]
- (#'binning/extract-bounds test-min-max-field
- {1 [(ql/< (ql/field-id 1) 500)]}))
+ {:min-value 100, :max-value 500}
+ (#'binning/extract-bounds 1
+ test-min-max-fingerprint
+ {1 [[:< [:field-id 1] 500]]}))
(expect
- [600 700]
- (#'binning/extract-bounds test-min-max-field
- {1 [(ql/> (ql/field-id 1) 200)
- (ql/< (ql/field-id 1) 800)
- (ql/between (ql/field-id 1) 600 700)]}))
+ {:min-value 600, :max-value 700}
+ (#'binning/extract-bounds 1
+ test-min-max-fingerprint
+ {1 [[:> [:field-id 1] 200]
+ [:< [:field-id 1] 800]
+ [:between [:field-id 1] 600 700]]}))
(expect
- [[ 0.0 1000.0 125.0 8]
- [200N 1600N 200 8]
- [ 0.0 1200.0 200 8]
- [ 0.0 1005.0 15.0 67]]
- (for [breakout [{:field-id 1, :min-value 100, :max-value 1000, :strategy :num-bins, :num-bins 8}
- {:field-id 1, :min-value 200, :max-value 1600, :strategy :num-bins, :num-bins 8}
- {:field-id 1, :min-value 9, :max-value 1002, :strategy :num-bins, :num-bins 8}
- {:field-id 1, :min-value 9, :max-value 1002, :strategy :bin-width, :bin-width 15.0}]]
- ((juxt :min-value :max-value :bin-width :num-bins) (binning/nicer-breakout breakout))))
+ [{:min-value 0.0, :max-value 1000.0, :num-bins 8, :bin-width 125.0}
+ {:min-value 200N, :max-value 1600N, :num-bins 8, :bin-width 200}
+ {:min-value 0.0, :max-value 1200.0, :num-bins 8, :bin-width 200}
+ {:min-value 0.0, :max-value 1005.0, :num-bins 67, :bin-width 15.0}]
+ (for [[strategy opts] [[:num-bins {:min-value 100, :max-value 1000, :num-bins 8, :bin-width 0}]
+ [:num-bins {:min-value 200, :max-value 1600, :num-bins 8, :bin-width 0}]
+ [:num-bins {:min-value 9, :max-value 1002, :num-bins 8, :bin-width 0}]
+ [:bin-width {:min-value 9, :max-value 1002, :num-bins 1, :bin-width 15.0}]]]
+ (#'binning/nicer-breakout strategy opts)))
+
+;; does `resolve-default-strategy` work the way we'd expect?
+(expect
+ [:num-bins {:num-bins 8, :bin-width 28.28321}]
+ (#'binning/resolve-default-strategy {:special_type :type/Income} 12.061602936923117 238.32732001721533))
+
+;; does `nicer-breakout` make things NICER?
+(expect
+ {:min-value 0.0, :max-value 240.0, :num-bins 8, :bin-width 30}
+ (#'binning/nicer-breakout :num-bins {:min-value 12.061602936923117
+ :max-value 238.32732001721533
+ :bin-width 28.28321
+ :num-bins 8}))
+
+;; Try an end-to-end test of the middleware
+(tt/expect-with-temp [Field [field (field/map->FieldInstance
+ {:database_type "DOUBLE"
+ :table_id (data/id :checkins)
+ :special_type :type/Income
+ :name "TOTAL"
+ :display_name "Total"
+ :fingerprint {:global {:distinct-count 10000}
+ :type {:type/Number {:min 12.061602936923117
+ :max 238.32732001721533
+ :avg 82.96014815230829}}}
+ :base_type :type/Float})]]
+ {:query {:source-table (data/id :checkins)
+ :breakout [[:binning-strategy
+ [:field-id (u/get-id field)]
+ :num-bins
+ nil
+ {:min-value 0.0, :max-value 240.0, :num-bins 8, :bin-width 30}]]}
+ :type :query
+ :database (data/id)}
+ (qp.store/with-store
+ (qp.store/store-field! field)
+ ((binning/update-binning-strategy identity)
+ {:query {:source-table (data/id :checkins)
+ :breakout [[:binning-strategy [:field-id (u/get-id field)] :default]]}
+ :type :query
+ :database (data/id)})))
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 be66bbe7a5afa73d0c927170f0d7543c21d6623c..dd550ef05c6166dbf21e792c6f9e5d5193d20824 100644
--- a/test/metabase/query_processor/middleware/fetch_source_query_test.clj
+++ b/test/metabase/query_processor/middleware/fetch_source_query_test.clj
@@ -75,7 +75,12 @@
:query {:source-table (str "card__" (u/get-id card))}})))
(expect
- (let [date-field-literal {:field-name "date", :base-type :type/Date, :binning-strategy nil, :binning-param nil, :fingerprint nil}]
+ (let [date-field-literal {:field-name "date"
+ :base-type :type/Date
+ :binning-strategy nil
+ :binning-param nil
+ :binning-opts nil
+ :fingerprint nil}]
(default-expanded-results
{:source-query {:source-table (data/id :checkins)
:join-tables nil}
diff --git a/test/metabase/query_processor_test/breakout_test.clj b/test/metabase/query_processor_test/breakout_test.clj
index cd7fd767ae9bd9d9ceb0af728e932466a9872dc7..fd027c8646af935e5a1d118b6a6a98b95ed9a48f 100644
--- a/test/metabase/query_processor_test/breakout_test.clj
+++ b/test/metabase/query_processor_test/breakout_test.clj
@@ -14,6 +14,7 @@
[metabase.test
[data :as data]
[util :as tu]]
+ [metabase.util.i18n :refer [tru]]
[metabase.test.data
[dataset-definitions :as defs]
[datasets :as datasets]]
@@ -217,8 +218,8 @@
:num_bins 4, :min_value 10.0
:max_value 50.0})
(-> (data/run-mbql-query venues
- {:aggregation [[:count]]
- :breakout [[:binning-strategy $latitude :default]]})
+ {:aggregation [[:count]]
+ :breakout [[:binning-strategy $latitude :default]]})
tu/round-fingerprint-cols
(get-in [:data :cols])
first))
@@ -229,8 +230,8 @@
:num_bins 5, :min_value 7.5,
:max_value 45.0})
(-> (data/run-mbql-query venues
- {:aggregation [[:count]]
- :breakout [[:binning-strategy $latitude :num-bins 5]]})
+ {:aggregation [[:count]]
+ :breakout [[:binning-strategy $latitude :num-bins 5]]})
tu/round-fingerprint-cols
(get-in [:data :cols])
first))
@@ -239,13 +240,11 @@
(datasets/expect-with-engines (non-timeseries-engines-with-feature :binning)
{:status :failed
:class Exception
- :error (format "Unable to bin field '%s' with id '%s' without a min/max value"
- (:name (Field (data/id :venues :latitude)))
- (data/id :venues :latitude))}
+ :error "Unable to bin Field without a min/max value"}
(tu/with-temp-vals-in-db Field (data/id :venues :latitude) {:fingerprint {:type {:type/Number {:min nil, :max nil}}}}
(-> (data/run-mbql-query venues
- {:aggregation [[:count]]
- :breakout [[:binning-strategy $latitude :default]]})
+ {:aggregation [[:count]]
+ :breakout [[:binning-strategy $latitude :default]]})
(select-keys [:status :class :error]))))
(defn- field->result-metadata [field]
@@ -253,10 +252,10 @@
(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 [[:binning-strategy [:field-literal (data/format-name :latitude) :type/Float] :num-bins 20]]}})
+ :type :query
+ :query {:source-table (str "card__" (u/get-id card-or-card-id))
+ :aggregation [:count]
+ :breakout [[:binning-strategy [:field-literal (data/format-name :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 :nested-queries)