From dd0ebfd4556162df1d850b9113321670685fc0c9 Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Tue, 25 Sep 2018 18:46:40 -0700
Subject: [PATCH] QP refactor part 7 [WIP] [ci skip]

---
 src/metabase/mbql/schema.clj                  |  5 +++-
 .../query_processor/middleware/binning.clj    | 23 +++++++++++--------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/src/metabase/mbql/schema.clj b/src/metabase/mbql/schema.clj
index 2b6ebcb21ebd2..7194aa7729414 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))
@@ -74,10 +75,11 @@
 (def ^:private BinningStrategyName
   (s/enum :num-bins :bin-width :default))
 
+;; TODO - binning strategy is disallowed for `:default` and required for the others
 (defclause binning-strategy
   field                     (one-of field-id field-literal fk-> expression datetime-field)
   strategy-name             BinningStrategyName
-  strategy-param            (optional s/Num))
+  strategy-param            (optional (s/constrained s/Num (complement neg?) "strategy param must be >= 0.")))
 
 (def Field
   "Schema for anything that refers to a Field, from the common `[:field-id <id>]` to variants like `:datetime-field` or
@@ -245,6 +247,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))
 
diff --git a/src/metabase/query_processor/middleware/binning.clj b/src/metabase/query_processor/middleware/binning.clj
index 53c333bb23e80..c14481213a077 100644
--- a/src/metabase/query_processor/middleware/binning.clj
+++ b/src/metabase/query_processor/middleware/binning.clj
@@ -4,8 +4,8 @@
             [metabase
              [public-settings :as public-settings]
              [util :as u]]
-            [metabase.query-processor.util :as qputil])
-  (:import [metabase.query_processor.interface BetweenFilter BinnedField ComparisonFilter]))
+            [metabase.mbql.util :as mbql.u]
+            [metabase.query-processor.util :as qputil]))
 
 (defn- update!
   "Similar to `clojure.core/update` but works on transient maps"
@@ -13,9 +13,11 @@
   (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."
+  "Find any comparison or `:between` filter. AND...?" ; TODO
   [mbql-filter]
+  (for [filter-clause    (mbql.u/clause-instances #{:between :< :<= :> :>=} mbql-filter)
+        field-id-clauses (mbql.u/clause-instances #{:field-id} filter)])
+  (let [filters ])
   (let [acc (transient {})]
     (walk/prewalk
      (fn [x]
@@ -155,14 +157,15 @@
       ;; nice version.
       (or (nicer-breakout resolved-binned-field) resolved-binned-field))))
 
+(defn update-binning-strategy* [query]
+  (let [filter-field-map (filter->field-map (get-in query [:query :filter]))]
+    (qputil/postwalk-pred #(instance? BinnedField %)
+                          #(update-binned-field % filter-field-map)
+                          query)))
+
 (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*))