Merge pull request metabase#2586 from metabase/default-to-day-bucketing-except-for-sorting

Default to `:day` bucketing everywhere except when sorting. 😓

Author: camsaul

src/metabase/query_processor/expand.clj

@@ -306,27 +306,37 @@
 
 ;;; ## order-by
 
-(s/defn ^:private ^:always-validate maybe-parse-order-by-subclause :- i/OrderBy [subclause]
-  (cond
-    (map? subclause)    subclause
-    (vector? subclause) (let [[f direction] subclause]
-                          (log/warn (u/format-color 'yellow (str "The syntax for order-by has changed in MBQL '98. [<field> :ascending/:descending] is deprecated. "
-                                                                 "Prefer [:asc/:desc <field>] instead.")))
-                          {:field (field f), :direction (normalize-token direction)})))
-
-(s/defn ^:ql ^:always-validate asc :- i/OrderBy
-  "order-by subclause. Specify that results should be returned in ascending order for Field or AgRef F.
+(s/defn ^:private ^:always-validate order-by-subclause :- i/OrderBy
+  [direction :- i/OrderByDirection, f]
+  ;; it's not particularly useful to sort datetime fields with the default `:day` bucketing,
+  ;; so specifiy `:default` bucketing to prevent the default of `:day` from being set during resolution.
+  ;; This won't affect fields that aren't `DateTimeFields`.
+  {:direction direction
+   :field     (let [f (field f)]
+                (if-not (instance? FieldPlaceholder f)
+                  f
+                  (update f :datetime-unit (fn [unit]
+                                             (core/or unit :default)))))})
+
+(def ^:ql ^{:arglists '([field])} asc
+  "`order-by` subclause. Specify that results should be returned in ascending order for Field or AgRef F.
 
      (order-by {} (asc 100))"
-  [f]
-  {:field (field f), :direction :ascending})
+  (partial order-by-subclause :ascending))
 
-(s/defn ^:ql ^:always-validate desc :- i/OrderBy
-  "order-by subclause. Specify that results should be returned in ascending order for Field or AgRef F.
+(def ^:ql ^{:arglists '([field])} desc
+  "`order-by` subclause. Specify that results should be returned in ascending order for Field or AgRef F.
 
      (order-by {} (desc 100))"
-  [f]
-  {:field (field f), :direction :descending})
+  (partial order-by-subclause :descending))
+
+(s/defn ^:private ^:always-validate maybe-parse-order-by-subclause :- i/OrderBy
+  [subclause]
+  (cond
+    (map? subclause)    subclause ; already parsed by `asc` or `desc`
+    (vector? subclause) (let [[f direction] subclause]
+                          (log/warn (u/format-color 'yellow "The syntax for order-by has changed in MBQL '98. [<field> :ascending/:descending] is deprecated. Prefer [:asc/:desc <field>] instead."))
+                          (order-by-subclause (normalize-token direction) f))))
 
 (defn ^:ql order-by
   "Specify how ordering should be done for this query.

src/metabase/query_processor/interface.clj

@@ -300,11 +300,14 @@
   (s/named (s/cond-pre SimpleFilterClause NotFilter CompoundFilter)
            "Valid filter clause"))
 
+(def OrderByDirection
+  "Schema for the direction in an `OrderBy` subclause."
+  (s/named (s/enum :ascending :descending) "Valid order-by direction"))
 
 (def OrderBy
   "Schema for top-level `order-by` clause in an MBQL query."
   (s/named {:field     AnyField
-            :direction (s/named (s/enum :ascending :descending) "Valid order-by direction")}
+            :direction OrderByDirection}
            "Valid order-by subclause"))
 
 

src/metabase/query_processor/resolve.clj

@@ -97,13 +97,12 @@
   (if-let [{:keys [base-type special-type], :as field} (some-> (field-id->fields field-id)
                                                                map->Field)]
     ;; try to resolve the Field with the ones available in field-id->fields
-    (let [datetime-field? (or datetime-unit
-                              (contains? #{:DateField :DateTimeField} base-type)
+    (let [datetime-field? (or (contains? #{:DateField :DateTimeField} base-type)
                               (contains? #{:timestamp_seconds :timestamp_milliseconds} special-type))]
       (if-not datetime-field?
         field
         (map->DateTimeField {:field field
-                             :unit  (or datetime-unit :default)})))
+                             :unit  (or datetime-unit :day)}))) ; default to `:day` if a unit wasn't specified
     ;; If that fails just return ourselves as-is
     this))
 

test/metabase/query_processor_test.clj

@@ -485,7 +485,7 @@
      :cols    [(aggregate-col :count)]}
   (format-rows-by [int] (run-query checkins
                           (ql/aggregation (ql/count))
-                          (ql/filter (ql/between (ql/datetime-field $date :day) "2015-04-01" "2015-05-01")))))
+                          (ql/filter (ql/between $date "2015-04-01" "2015-05-01")))))
 
 ;; ### FILTER -- "OR", "<=", "="
 (expect-with-non-timeseries-dbs
@@ -859,8 +859,8 @@
     9)
   (count (rows (dataset sad-toucan-incidents
                  (run-query incidents
-                   (ql/filter (ql/and (ql/> (ql/datetime-field $timestamp :day) "2015-06-01")
-                                      (ql/< (ql/datetime-field $timestamp :day) "2015-06-03")))
+                   (ql/filter (ql/and (ql/> $timestamp "2015-06-01")
+                                      (ql/< $timestamp "2015-06-03")))
                    (ql/order-by (ql/asc $timestamp)))))))
 
 (expect-with-non-timeseries-dbs
@@ -905,7 +905,7 @@
   (->> (dataset sad-toucan-incidents
          (run-query incidents
            (ql/aggregation (ql/count))
-           (ql/breakout (ql/datetime-field $timestamp :day))
+           (ql/breakout $timestamp)
            (ql/limit 10)))
        rows (format-rows-by [identity int])))
 

test/metabase/timeseries_query_processor_test.clj

@@ -390,7 +390,7 @@
              ["2013-01-23+0000" 1]]}
   (data (data/run-query checkins
           (ql/aggregation (ql/count))
-          (ql/breakout (ql/datetime-field $timestamp :day))
+          (ql/breakout $timestamp)
           (ql/limit 5))))
 
 ;;; date bucketing - minute