Feature flag binning

src/metabase/api/table.clj

@@ -4,12 +4,13 @@
             [compojure.core :refer [GET PUT]]
             [medley.core :as m]
             [metabase
+             [driver :as driver]
              [sync-database :as sync-database]
              [util :as u]]
             [metabase.api.common :as api]
             [metabase.models
              [card :refer [Card]]
-             [database :as database]
+             [database :as database :refer [Database]]
              [field :refer [Field]]
              [interface :as mi]
              [table :as table :refer [Table]]]
@@ -140,29 +141,35 @@
 (def ^:private coordinate-dimension-indexes
   (create-dim-index-seq :type/Coordinate))
 
-(defn- assoc-dimension-options [resp]
-  (-> resp
-      (assoc :dimension_options dimension-options-for-response)
-      (update :fields (fn [fields]
-                        (for [{:keys [base_type special_type min_value max_value] :as field} fields]
-                          (assoc field
-                            :dimension_options
-                            (cond
+(defn- assoc-field-dimension-options [{:keys [base_type special_type min_value max_value] :as field}]
+  (assoc field
+    :dimension_options
+    (cond
 
-                              (isa? base_type :type/DateTime)
-                              datetime-dimension-indexes
+      (isa? base_type :type/DateTime)
+      datetime-dimension-indexes
 
-                              (and min_value max_value
-                                   (isa? special_type :type/Coordinate))
-                              coordinate-dimension-indexes
+      (and min_value max_value
+           (isa? special_type :type/Coordinate))
+      coordinate-dimension-indexes
 
-                              (and min_value max_value
-                                   (isa? base_type :type/Number)
-                                   (or (nil? special_type) (isa? special_type :type/Number)))
-                              numeric-dimension-indexes
+      (and min_value max_value
+           (isa? base_type :type/Number)
+           (or (nil? special_type) (isa? special_type :type/Number)))
+      numeric-dimension-indexes
 
-                              :else
-                              [])))))))
+      :else
+      [])))
+
+(defn- assoc-dimension-options [resp driver]
+  (if (and driver (contains? (driver/features driver) :binning))
+    (-> resp
+        (assoc :dimension_options dimension-options-for-response)
+        (update :fields #(mapv assoc-field-dimension-options %)))
+    (-> resp
+        (assoc :dimension_options [])
+        (update :fields (fn [fields]
+                          (mapv #(assoc % :dimension_options []) fields))))))
 
 (api/defendpoint GET "/:id/query_metadata"
   "Get metadata about a `Table` useful for running queries.
@@ -172,16 +179,18 @@
   will any of its corresponding values be returned. (This option is provided for use in the Admin Edit Metadata page)."
   [id include_sensitive_fields]
   {include_sensitive_fields (s/maybe su/BooleanString)}
-  (-> (api/read-check Table id)
-      (hydrate :db [:fields :target] :field_values :segments :metrics)
-      (m/dissoc-in [:db :details])
-      assoc-dimension-options
-      (update-in [:fields] (if (Boolean/parseBoolean include_sensitive_fields)
-                             ;; If someone passes include_sensitive_fields return hydrated :fields as-is
-                             identity
-                             ;; Otherwise filter out all :sensitive fields
-                             (partial filter (fn [{:keys [visibility_type]}]
-                                               (not= (keyword visibility_type) :sensitive)))))))
+  (let [table (api/read-check Table id)
+        driver (driver/engine->driver (db/select-one-field :engine Database :id (:db_id table)))]
+    (-> table
+        (hydrate :db [:fields :target] :field_values :segments :metrics)
+        (m/dissoc-in [:db :details])
+        (assoc-dimension-options driver)
+        (update-in [:fields] (if (Boolean/parseBoolean include_sensitive_fields)
+                               ;; If someone passes include_sensitive_fields return hydrated :fields as-is
+                               identity
+                               ;; Otherwise filter out all :sensitive fields
+                               (partial filter (fn [{:keys [visibility_type]}]
+                                                 (not= (keyword visibility_type) :sensitive))))))))
 
 (defn- card-result-metadata->virtual-fields
   "Return a sequence of 'virtual' fields metadata for the 'virtual' table for a Card in the Saved Questions 'virtual' database."

src/metabase/driver/generic_sql.clj

@@ -339,7 +339,8 @@
             :expressions
             :expression-aggregations
             :native-parameters
-            :nested-queries}
+            :nested-queries
+            :binning}
     (set-timezone-sql driver) (conj :set-timezone)))
 
 

test/metabase/api/table_test.clj

@@ -559,13 +559,17 @@
 
 ;; Lat/Long fields should use bin-width rather than num-bins
 (expect
-  #{"bin-width" "default"}
+  (if (binning-supported?)
+    #{"bin-width" "default"}
+    #{})
   (let [response ((user->client :rasta) :get 200 (format "table/%d/query_metadata" (id :venues)))]
     (extract-dimension-options response "LATITUDE")))
 
 ;; Number columns without a special type should use "num-bins"
 (expect
-  #{"num-bins" "default"}
+  (if (binning-supported?)
+    #{"num-bins" "default"}
+    #{})
   (let [{:keys [special_type]} (Field (id :venues :price))]
     (try
       (db/update! Field (id :venues :price) :special_type nil)

test/metabase/query_processor_test/breakout_test.clj

 (ns metabase.query-processor-test.breakout-test
   "Tests for the `:breakout` clause."
-  (:require [metabase.query-processor-test :refer :all]
-            [metabase.query-processor.expand :as ql]
+  (:require [metabase
+             [query-processor-test :refer :all]
+             [util :as u]]
             [metabase.models.field :refer [Field]]
-            [metabase.test.data :as data]
-            [metabase.test.util :as tu]
-            [metabase.util :as u]
+            [metabase.query-processor.expand :as ql]
+            [metabase.test
+             [data :as data]
+             [util :as tu]]
+            [metabase.test.data.datasets :as datasets]
             [toucan.db :as db]))
 
 ;; single column
@@ -74,21 +77,21 @@
        booleanize-native-form
        (format-rows-by [int int int])))
 
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
   [[10.1 1] [33.1 61] [37.7 29] [39.2 8] [40.8 1]]
   (format-rows-by [(partial u/round-to-decimals 1) int]
     (rows (data/run-query venues
             (ql/aggregation (ql/count))
             (ql/breakout (ql/binning-strategy $latitude :num-bins 20))))))
 
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
  [[10.1 1] [30.5 99]]
   (format-rows-by [(partial u/round-to-decimals 1) int]
     (rows (data/run-query venues
             (ql/aggregation (ql/count))
             (ql/breakout (ql/binning-strategy $latitude :num-bins 3))))))
 
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
   [[10.1 -165.4 1] [33.1 -119.7 61] [37.7 -124.2 29] [39.2 -78.5 8] [40.8 -78.5 1]]
   (format-rows-by [(partial u/round-to-decimals 1) (partial u/round-to-decimals 1) int]
     (rows (data/run-query venues
@@ -98,14 +101,14 @@
 
 ;; Currently defaults to 8 bins when the number of bins isn't
 ;; specified
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
  [[10.1 1] [30.1 90] [40.1 9]]
   (format-rows-by [(partial u/round-to-decimals 1) int]
     (rows (data/run-query venues
             (ql/aggregation (ql/count))
             (ql/breakout (ql/binning-strategy $latitude :default))))))
 
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
  [[10.1 1] [30.1 61] [35.1 29] [40.1 9]]
   (tu/with-temporary-setting-values [breakout-bin-width 5.0]
     (format-rows-by [(partial u/round-to-decimals 1) int]
@@ -114,7 +117,7 @@
               (ql/breakout (ql/binning-strategy $latitude :default)))))))
 
 ;; Testing bin-width
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
   [[10.1 1] [33.1 25] [34.1 36] [37.1 29] [40.1 9]]
   (format-rows-by [(partial u/round-to-decimals 1) int]
     (rows (data/run-query venues
@@ -122,14 +125,14 @@
             (ql/breakout (ql/binning-strategy $latitude :bin-width 1))))))
 
 ;; Testing bin-width using a float
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
    [[10.1 1] [32.6 61] [37.6 29] [40.1 9]]
   (format-rows-by [(partial u/round-to-decimals 1) int]
     (rows (data/run-query venues
             (ql/aggregation (ql/count))
             (ql/breakout (ql/binning-strategy $latitude :bin-width 2.5))))))
 
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
   [[33.0 4] [34.0 57]]
   (tu/with-temporary-setting-values [breakout-bin-width 1.0]
     (format-rows-by [(partial u/round-to-decimals 1) int]
@@ -140,7 +143,7 @@
               (ql/breakout (ql/binning-strategy $latitude :default)))))))
 
 ;;Validate binning info is returned with the binning-strategy
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
   (merge (venues-col :latitude)
          {:min_value 10.0646, :source :breakout,
           :max_value 40.7794, :binning_info {:binning_strategy "num-bins", :bin_width 10.0,
@@ -153,7 +156,7 @@
       first))
 
 ;;Validate binning info is returned with the binning-strategy
-(expect-with-non-timeseries-dbs
+(datasets/expect-with-engines (engines-that-support :binning)
   {:status :failed
    :class Exception
    :error (format "Unable to bin field '%s' with id '%s' without a min/max value"

test/metabase/test/data.clj

@@ -167,6 +167,11 @@
   []
   (contains? (driver/features *driver*) :foreign-keys))
 
+(defn binning-supported?
+  "Does the current engine support binning?"
+  []
+  (contains? (driver/features *driver*) :binning))
+
 (defn default-schema [] (i/default-schema *driver*))
 (defn id-field-type  [] (i/id-field-type *driver*))