From 6a2868841eae4c151ca55aedd30922bb38d7de81 Mon Sep 17 00:00:00 2001
From: Ryan Senior <ryan@metabase.com>
Date: Tue, 13 Jun 2017 19:29:30 -0500
Subject: [PATCH] First cut at constraing min/max from query criteria
---
.../driver/generic_sql/query_processor.clj | 2 +-
src/metabase/query_processor.clj | 2 +
src/metabase/query_processor/interface.clj | 7 +-
.../query_processor/middleware/binning.clj | 67 +++++++++++++++++++
src/metabase/query_processor/resolve.clj | 8 +--
.../query_processor_test/breakout_test.clj | 10 +++
6 files changed, 86 insertions(+), 10 deletions(-)
create mode 100644 src/metabase/query_processor/middleware/binning.clj
diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj
index e89476b054e..a9d8a0d4833 100644
--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -89,7 +89,7 @@
(sql/date (driver) unit (formatted field)))
BinnedField
- (formatted [{{:keys [min-value max-value] :as field} :field bin-width :bin-width}]
+ (formatted [{:keys [bin-width min-value max-value field]}]
(let [formatted-field (formatted field)]
(apply hsql/call :case
(mapcat (fn [bin-floor]
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 9f486d28aa3..1594d6b26f2 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -12,6 +12,7 @@
[add-row-count-and-status :as row-count-and-status]
[add-settings :as add-settings]
[annotate-and-sort :as annotate-and-sort]
+ [binning :as binning]
[cache :as cache]
[catch-exceptions :as catch-exceptions]
[cumulative-aggregations :as cumulative-ags]
@@ -85,6 +86,7 @@
cumulative-ags/handle-cumulative-aggregations
implicit-clauses/add-implicit-clauses
format-rows/format-rows
+ binning/update-binning-strategy
expand-resolve/expand-resolve ; ▲▲▲ QUERY EXPANSION POINT ▲▲▲ All functions *above* will see EXPANDED query during PRE-PROCESSING
row-count-and-status/add-row-count-and-status ; ▼▼▼ RESULTS WRAPPING POINT ▼▼▼ All functions *below* will see results WRAPPED in `:data` during POST-PROCESSING
parameters/substitute-parameters
diff --git a/src/metabase/query_processor/interface.clj b/src/metabase/query_processor/interface.clj
index c9c9599c730..10e8793c8e8 100644
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -141,8 +141,11 @@
clojure.lang.Named
(getName [_] (name field)))
-(s/defrecord BinnedField [field :- Field
- num-bins :- s/Int]
+(s/defrecord BinnedField [field :- Field
+ num-bins :- s/Int
+ min-value :- s/Num
+ max-value :- s/Num
+ bin-width :- s/Num]
clojure.lang.Named
(getName [_] (name field)))
diff --git a/src/metabase/query_processor/middleware/binning.clj b/src/metabase/query_processor/middleware/binning.clj
new file mode 100644
index 00000000000..2879947fae7
--- /dev/null
+++ b/src/metabase/query_processor/middleware/binning.clj
@@ -0,0 +1,67 @@
+(ns metabase.query-processor.middleware.binning
+ (:require [clojure.walk :as walk]
+ [metabase.util :as u])
+ (:import [metabase.query_processor.interface BinnedField ComparisonFilter BetweenFilter]))
+
+(defn- update! [^clojure.lang.ITransientAssociative coll k f]
+ (assoc! coll k (f (get coll k))))
+
+(defn- filter->field-map [mbql-filter]
+ (let [acc (transient {})]
+ (clojure.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 [min-value max-value num-bins]
+ (u/round-to-decimals 5 (/ (- max-value min-value)
+ num-bins)))
+
+(defn extract-bounds [{field-id :field-id, global-min :min-value, global-max :max-value} field-filter-map]
+ ;; Assuming only one for now
+ (let [bound-1 (first (for [{:keys [value filter-type]} (get field-filter-map field-id)
+ :when (or (= :> filter-type)
+ (= :>= filter-type))]
+ (:value value) ))
+ bound-2 (first (for [{:keys [value filter-type]} (get field-filter-map field-id)
+ :when (or (= :< filter-type)
+ (= :<= filter-type))]
+ (:value value)))
+ comparison-bounds (when (and bound-1 bound-2)
+ (if (> bound-1 bound-2)
+ [bound-2 bound-1]
+ [bound-1 bound-2]))
+ ;;Assuming either >/< or between
+ between-bounds (first (for [{:keys [filter-type min-value max-value]} (get field-filter-map field-id)
+ :when (= :between filter-type)]
+ [min-value max-value]))]
+ (or (seq comparison-bounds)
+ (seq between-bounds)
+ [global-min global-max])) )
+
+(defn update-bin-width [breakouts filter-field-map]
+ (mapv (fn [{:keys [field num-bins] :as breakout}]
+ (if-let [[min-value max-value] (and (instance? BinnedField breakout)
+ (extract-bounds field filter-field-map))]
+ (assoc breakout
+ :min-value min-value
+ :max-value max-value
+ :bin-width (calculate-bin-width min-value max-value num-bins))
+ breakout))
+ breakouts))
+
+(defn update-binning-strategy
+ [qp]
+ (fn [query]
+ (let [binned-breakouts (filter #(instance? BinnedField %) (get-in query [:query :breakout]))]
+ (if (seq binned-breakouts)
+ (qp (update-in query [:query :breakout] update-bin-width (filter->field-map (get-in query [:query :filter]))))
+ (qp query)))))
diff --git a/src/metabase/query_processor/resolve.clj b/src/metabase/query_processor/resolve.clj
index 458d7d93ea6..7bfaa3b565d 100644
--- a/src/metabase/query_processor/resolve.clj
+++ b/src/metabase/query_processor/resolve.clj
@@ -100,11 +100,6 @@
;;; ## ------------------------------------------------------------ FIELD PLACEHOLDER ------------------------------------------------------------
-(defn- calculate-bin-width [field num-bins]
- (u/round-to-decimals 5 (/ (- (:max-value field)
- (:min-value field))
- num-bins)))
-
(defn- field-ph-resolve-field [{:keys [field-id datetime-unit fk-field-id binning-strategy], :as this} field-id->field]
(if-let [{:keys [base-type special-type], :as field} (some-> (field-id->field field-id)
i/map->Field
@@ -116,8 +111,7 @@
:unit (or datetime-unit :day)}) ; default to `:day` if a unit wasn't specified
binning-strategy
(i/map->BinnedField {:field field
- :num-bins binning-strategy
- :bin-width (calculate-bin-width field binning-strategy)})
+ :num-bins binning-strategy})
:else field)
;; If that fails just return ourselves as-is
this))
diff --git a/test/metabase/query_processor_test/breakout_test.clj b/test/metabase/query_processor_test/breakout_test.clj
index 5e3089a5a8b..90dcc0ab719 100644
--- a/test/metabase/query_processor_test/breakout_test.clj
+++ b/test/metabase/query_processor_test/breakout_test.clj
@@ -111,6 +111,16 @@
(ql/aggregation (ql/count))
(ql/breakout (ql/binning-strategy $latitude :default)))))))
+(expect-with-non-timeseries-dbs
+ [[33.0 4] [34.0 57]]
+ (tu/with-temporary-setting-values [breakout-bins-num 15]
+ (format-rows-by [(partial u/round-to-decimals 1) int]
+ (rows (data/run-query venues
+ (ql/aggregation (ql/count))
+ (ql/filter (ql/and (ql/< $latitude 35)
+ (ql/> $latitude 20)))
+ (ql/breakout (ql/binning-strategy $latitude :default)))))))
+
;;Validate binning info is returned with the binning-strategy
(expect-with-non-timeseries-dbs
(merge (venues-col :latitude)
--
GitLab