Merge pull request #8579 from metabase/move-add-implicit-clauses-to-before-expansion

Query Processor Refactor Part 5

src/metabase/automagic_dashboards/comparison.clj

@@ -9,9 +9,7 @@
                            encode-base64-json metric-name source-name]]
              [filters :as filters]
              [populate :as populate]]
-            [metabase.mbql
-             [normalize :as normalize]
-             [util :as mbql.u]]
+            [metabase.mbql.normalize :as normalize]
             [metabase.models.table :refer [Table]]
             [metabase.query-processor.util :as qp.util]
             [puppetlabs.i18n.core :as i18n :refer [tru]]))
@@ -43,14 +41,22 @@
 (def ^:private ^{:arglists '([card])} display-type
   (comp qp.util/normalize-token :display))
 
+(defn- add-filter-clauses
+  "Add `new-filter-clauses` to a query. There is actually an `mbql.u/add-filter-clause` function we should be using
+  instead, but that validates its input and output, and the queries that come in here aren't always valid (for
+  example, missing `:database`). If we can, it would be nice to use that instead of reinventing the wheel here."
+  [{{existing-filter-clause :filter} :query, :as query}, new-filter-clauses]
+  (let [clauses           (filter identity (cons existing-filter-clause new-filter-clauses))
+        new-filter-clause (when (seq clauses)
+                            (normalize/normalize-fragment [:query :filter] (cons :and clauses)))]
+    (cond-> query
+      (seq new-filter-clause) (assoc-in [:query :filter] new-filter-clause))))
+
 (defn- inject-filter
   "Inject filter clause into card."
   [{:keys [query-filter cell-query] :as root} card]
   (-> card
-      (update :dataset_query #(reduce mbql.u/add-filter-clause
-                                      %
-                                      (map (partial normalize/normalize-fragment [:query :filter])
-                                           [query-filter cell-query])))
+      (update :dataset_query #(add-filter-clauses % [query-filter cell-query]))
       (update :series (partial map (partial inject-filter root)))))
 
 (defn- multiseries?

src/metabase/driver/googleanalytics/query_processor.clj

 (ns metabase.driver.googleanalytics.query-processor
   "The Query Processor is responsible for translating the Metabase Query Language into Google Analytics request format.
   See https://developers.google.com/analytics/devguides/reporting/core/v3"
-  (:require [clojure.string :as s]
+  (:require [clojure.string :as str]
             [clojure.tools.reader.edn :as edn]
             [medley.core :as m]
             [metabase.query-processor
@@ -51,8 +51,8 @@
   (into {} (for [c chars-to-escape]
              {c (str "\\" c)})))
 
-(def ^:private ^{:arglists '([s])} escape-for-regex         (u/rpartial s/escape (char-escape-map ".\\+*?[^]$(){}=!<>|:-")))
-(def ^:private ^{:arglists '([s])} escape-for-filter-clause (u/rpartial s/escape (char-escape-map ",;\\")))
+(def ^:private ^{:arglists '([s])} escape-for-regex         (u/rpartial str/escape (char-escape-map ".\\+*?[^]$(){}=!<>|:-")))
+(def ^:private ^{:arglists '([s])} escape-for-filter-clause (u/rpartial str/escape (char-escape-map ",;\\")))
 
 (defn- ga-filter ^String [& parts]
   (escape-for-filter-clause (apply str parts)))
@@ -90,10 +90,10 @@
 (defn- handle-breakout [{breakout-clause :breakout}]
   {:dimensions (if-not breakout-clause
                  ""
-                 (s/join "," (for [breakout-field breakout-clause]
-                               (if (instance? DateTimeField breakout-field)
-                                 (unit->ga-dimension (:unit breakout-field))
-                                 (->rvalue breakout-field)))))})
+                 (str/join "," (for [breakout-field breakout-clause]
+                                 (if (instance? DateTimeField breakout-field)
+                                   (unit->ga-dimension (:unit breakout-field))
+                                   (->rvalue breakout-field)))))})
 
 
 ;;; ### filter
@@ -125,15 +125,15 @@
 
 (defn- parse-filter-clause:filters ^String [{:keys [compound-type subclause subclauses], :as clause}]
   (case compound-type
-    :and (s/join ";" (remove nil? (map parse-filter-clause:filters subclauses)))
-    :or  (s/join "," (remove nil? (map parse-filter-clause:filters subclauses)))
+    :and (str/join ";" (remove nil? (map parse-filter-clause:filters subclauses)))
+    :or  (str/join "," (remove nil? (map parse-filter-clause:filters subclauses)))
     :not (parse-filter-subclause:filters subclause :negate)
     nil  (parse-filter-subclause:filters clause)))
 
 (defn- handle-filter:filters [{filter-clause :filter}]
   (when filter-clause
     (let [filter (parse-filter-clause:filters filter-clause)]
-      (when-not (s/blank? filter)
+      (when-not (str/blank? filter)
         {:filters filter}))))
 
 (defn- parse-filter-subclause:interval [{:keys [filter-type field value], :as filter} & [negate?]]
@@ -173,7 +173,7 @@
 
 (defn- handle-order-by [{:keys [order-by], :as query}]
   (when order-by
-    {:sort (s/join "," (for [{:keys [field direction]} order-by]
+    {:sort (str/join "," (for [{:keys [field direction]} order-by]
                          (str (case direction
                                 :ascending  ""
                                 :descending "-")
@@ -205,7 +205,7 @@
    :mbql? true})
 
 (defn- parse-number [s]
-  (edn/read-string (s/replace s #"^0+(.+)$" "$1")))
+  (edn/read-string (str/replace s #"^0+(.+)$" "$1")))
 
 (def ^:private ga-dimension->date-format-fn
   {"ga:minute"         parse-number
@@ -259,7 +259,7 @@
 (defn- built-in-metrics
   [{query :query}]
   (when-let [ags (seq (aggregations query))]
-    (s/join "," (for [[aggregation-type metric-name] ags
+    (str/join "," (for [[aggregation-type metric-name] ags
                       :when (and aggregation-type
                                  (= :metric (qputil/normalize-token aggregation-type))
                                  (string? metric-name))]
@@ -312,10 +312,12 @@
       (when (> (count filter-clause) 1)
         (vec filter-clause)))))
 
-(defn- handle-built-in-segments [query]
-  (-> query
-      (assoc-in [:ga :segment] (built-in-segments query))
-      (update-in [:query :filter] remove-built-in-segments)))
+(defn- handle-built-in-segments [{{filters :filter} :query, :as query}]
+  (let [query   (assoc-in query [:ga :segment] (built-in-segments query))
+        filters (remove-built-in-segments filters)]
+    (if (seq filters)
+      (assoc-in    query [:query :filter] filters)
+      (m/dissoc-in query [:query :filter]))))
 
 
 ;;; public

src/metabase/mbql/schema.clj

@@ -205,7 +205,8 @@
 (defclause asc,  field FieldOrAggregationReference)
 (defclause desc, field FieldOrAggregationReference)
 
-(def ^:private OrderBy
+(def OrderBy
+  "Schema for an `order-by` clause subclause."
   (one-of asc desc))
 
 
@@ -324,7 +325,10 @@
   {:query                          s/Any
    (s/optional-key :template-tags) {su/NonBlankString TemplateTag}
    ;; collection (table) this query should run against. Needed for MongoDB
-   (s/optional-key :collection)    (s/maybe su/NonBlankString)})
+   (s/optional-key :collection)    (s/maybe su/NonBlankString)
+   ;; other stuff gets added in my different bits of QP middleware to record bits of state or pass info around.
+   ;; Everyone else can ignore them.
+   s/Keyword                       s/Any})
 
 
 ;;; ----------------------------------------------- MBQL [Inner] Query -----------------------------------------------
@@ -353,7 +357,10 @@
     (s/optional-key :limit)        su/IntGreaterThanZero
     (s/optional-key :order-by)     (su/non-empty [OrderBy])
     (s/optional-key :page)         {:page  su/IntGreaterThanOrEqualToZero
-                                    :items su/IntGreaterThanZero}}
+                                    :items su/IntGreaterThanZero}
+    ;; Various bits of middleware add additonal keys, such as `fields-is-implicit?`, to record bits of state or pass
+    ;; info to other pieces of middleware. Everyone else can ignore them.
+    s/Keyword                      s/Any}
    (fn [query]
      (core/= 1 (core/count (select-keys query [:source-query :source-table]))))
    "Query must specify either `:source-table` or `:source-query`, but not both."))
@@ -371,8 +378,10 @@
 (def ^:private Settings
   "Options that tweak the behavior of the query processor."
   ;; The timezone the query should be ran in, overriding the default report timezone for the instance.
-  {(s/optional-key :report-timezone) su/NonBlankString})
-;; TODO - should we add s/Any keys here? What if someone wants to add custom middleware!
+  {(s/optional-key :report-timezone) su/NonBlankString
+   ;; other Settings might be used somewhere, but I don't know about them. Add them if you come across them for
+   ;; documentation purposes
+   s/Keyword                         s/Any})
 
 (def ^:private Constraints
   "Additional constraints added to a query limiting the maximum number of rows that can be returned. Mostly useful
@@ -381,7 +390,10 @@
   {;; maximum number of results to allow for a query with aggregations
    (s/optional-key :max-results)           su/IntGreaterThanOrEqualToZero
    ;; maximum number of results to allow for a query with no aggregations
-   (s/optional-key :max-results-bare-rows) su/IntGreaterThanOrEqualToZero})
+   (s/optional-key :max-results-bare-rows) su/IntGreaterThanOrEqualToZero
+   ;; other Constraints might be used somewhere, but I don't know about them. Add them if you come across them for
+   ;; documentation purposes
+   s/Keyword                               s/Any})
 
 (def ^:private MiddlewareOptions
   "Additional options that can be used to toggle middleware on or off."
@@ -390,7 +402,10 @@
    (s/optional-key :skip-results-metadata?) s/Bool
    ;; should we skip converting datetime types to ISO-8601 strings with appropriate timezone when post-processing
    ;; results? Used by `metabase.query-processor.middleware.format-rows`; default `false`
-   (s/optional-key :format-rows?)           s/Bool})
+   (s/optional-key :format-rows?)           s/Bool
+   ;; other middleware options might be used somewhere, but I don't know about them. Add them if you come across them
+   ;; for documentation purposes
+   s/Keyword                                s/Any})
 
 
 ;;; ------------------------------------------------------ Info ------------------------------------------------------
@@ -450,7 +465,7 @@
     (s/optional-key :query)       MBQLQuery
     (s/optional-key :parameters)  [Parameter]
     ;;
-    ;; -------------------- OPTIONS --------------------
+    ;; OPTIONS
     ;;
     ;; These keys are used to tweak behavior of the Query Processor.
     ;; TODO - can we combine these all into a single `:options` map?
@@ -458,19 +473,15 @@
     (s/optional-key :settings)    (s/maybe Settings)
     (s/optional-key :constraints) (s/maybe Constraints)
     (s/optional-key :middleware)  (s/maybe MiddlewareOptions)
-    ;; The maximum time, in seconds, to return cached results for this query rather than running a new query. This is
-    ;; added automatically when running queries belonging to Cards when caching is enabled. Caching is handled by the
-    ;; automatically by caching middleware.
-    (s/optional-key :cache-ttl)   (s/maybe su/IntGreaterThanZero)
     ;;
-    ;; -------------------- INFO --------------------
+    ;; 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)
-    ;;
-    ;; not even really info, but in some cases `:driver` gets added to the query even though we have middleware that
-    ;; is supposed to resolve driver, and we also have the `*driver*` dynamic var where we should probably be stashing
-    ;; the resolved driver anyway. It might make sense to take this out in the future.
-    (s/optional-key :driver)      {}}
+    ;; 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}
    (fn [{native :native, mbql :query, query-type :type}]
      (case query-type
        :native (core/and native (core/not mbql))

src/metabase/mbql/schema/helpers.clj

@@ -49,7 +49,9 @@
   (let [[symb-name clause-name] (if (vector? clause-name)
                                   clause-name
                                   [clause-name clause-name])]
-    `(def ~(vary-meta symb-name assoc :private true, :clause-name (keyword clause-name))
+    `(def ~(vary-meta symb-name assoc
+                      :clause-name (keyword clause-name)
+                      :doc         (format "Schema for a valid %s clause." clause-name))
        (clause ~(keyword clause-name) ~@(stringify-names arg-names-and-schemas)))))
 
 

src/metabase/mbql/util.clj

@@ -46,15 +46,30 @@
     ;;-> [[:field-id 10]]
 
     ;; look for :+ or :- clauses
-    (clause-instances #{:+ :-} ...)"
+    (clause-instances #{:+ :-} ...)
+
+  By default, this will not include subclauses of any clauses it finds, but you can toggle this behavior with the
+  `include-subclauses?` option:
+
+    (clause-instances #{:field-id :fk->} [[:field-id 1] [:fk-> [:field-id 2] [:field-id 3]]])
+    ;; -> [[:field-id 1]
+           [:fk-> [:field-id 2] [:field-id 3]]]
+
+    (clause-instances #{:field-id :fk->} [[:field-id 1] [:fk-> [:field-id 2] [:field-id 3]]], :include-subclauses? true)
+    ;; -> [[:field-id 1]
+           [:fk-> [:field-id 2] [:field-id 3]]
+           [:field-id 2]
+           [:field-id 3]]"
   {:style/indent 1}
-  [k-or-ks x]
+  [k-or-ks x & {:keys [include-subclauses?], :or {include-subclauses? false}}]
   (let [instances (atom [])]
-    (walk/postwalk
+    (walk/prewalk
      (fn [clause]
-       (u/prog1 clause
-         (when (is-clause? k-or-ks clause)
-           (swap! instances conj clause))))
+       (if (is-clause? k-or-ks clause)
+         (do (swap! instances conj clause)
+             (when include-subclauses?
+               clause))
+         clause))
      x)
     (seq @instances)))
 
@@ -134,9 +149,9 @@
   [filter-clause & more-filter-clauses]
   (simplify-compound-filter (vec (cons :and (filter identity (cons filter-clause more-filter-clauses))))))
 
-(s/defn add-filter-clause
+(s/defn add-filter-clause :- mbql.s/Query
   "Add an additional filter clause to an `outer-query`. If `new-clause` is `nil` this is a no-op."
-  [outer-query :- su/Map, new-clause :- (s/maybe mbql.s/Filter)]
+  [outer-query :- mbql.s/Query, new-clause :- (s/maybe mbql.s/Filter)]
   (if-not new-clause
     outer-query
     (update-in outer-query [:query :filter] combine-filter-clauses new-clause)))
@@ -162,3 +177,35 @@
     ;; otherwise resolve the source Table
     :else
     source-table-id))
+
+
+(defn field-clause->id-or-literal
+  "Get the actual Field ID or literal name this clause is referring to. Useful for seeing if two Field clauses are
+  referring to the same thing, e.g.
+
+    (field-clause->id-or-literal [:datetime-field [:field-id 100] ...]) ; -> 100
+    (field-clause->id-or-literal [:field-id 100])                       ; -> 100
+
+  For expressions (or any other clauses) this returns the clause as-is, so as to facilitate the primary use case of
+  comparing Field clauses."
+  [[clause-name x y, :as clause]]
+  (case clause-name
+    :field-id         x
+    :fk->             (recur y)
+    :field-literal    x
+    :datetime-field   (recur x)
+    :binning-strategy (recur x)
+    ;; for anything else, including expressions and ag clause references, just return the clause as-is
+    clause))
+
+(s/defn add-order-by-clause :- mbql.s/Query
+  "Add a new `:order-by` clause to an MBQL query. If the new order-by clause references a Field that is already being
+  used in another order-by clause, this function does nothing."
+  [outer-query :- mbql.s/Query, order-by-clause :- mbql.s/OrderBy]
+  (let [existing-clauses (set (map (comp field-clause->id-or-literal second)
+                                   (-> outer-query :query :order-by)))]
+    (if (existing-clauses (field-clause->id-or-literal (second order-by-clause)))
+      ;; Field already referenced, nothing to do
+      outer-query
+      ;; otherwise add new clause at the end
+      (update-in outer-query [:query :order-by] (comp vec conj) order-by-clause))))

src/metabase/query_processor.clj

@@ -15,6 +15,7 @@
              [add-row-count-and-status :as row-count-and-status]
              [add-settings :as add-settings]
              [annotate-and-sort :as annotate-and-sort]
+             [auto-bucket-datetime-breakouts :as bucket-datetime]
              [bind-effective-timezone :as bind-timezone]
              [binning :as binning]
              [cache :as cache]
@@ -103,8 +104,9 @@
       binning/update-binning-strategy
       resolve/resolve-middleware
       add-dim/add-remapping
-      implicit-clauses/add-implicit-clauses
       expand/expand-middleware                         ; ▲▲▲ QUERY EXPANSION POINT  ▲▲▲ All functions *above* will see EXPANDED query during PRE-PROCESSING
+      implicit-clauses/add-implicit-clauses
+      bucket-datetime/auto-bucket-datetime-breakouts
       source-table/resolve-source-table-middleware
       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
@@ -146,8 +148,8 @@
    This is useful for things that need to look at an expanded query, such as permissions checking for Cards."
   (->> identity
        resolve/resolve-middleware
-       source-table/resolve-source-table-middleware
        expand/expand-middleware
+       source-table/resolve-source-table-middleware
        parameters/substitute-parameters
        expand-macros/expand-macros
        driver-specific/process-query-in-context

src/metabase/query_processor/interface.clj

-(ns metabase.query-processor.interface
+(ns ^:deprecated metabase.query-processor.interface
   "Definitions of `Field`, `Value`, and other record types present in an expanded query.
    This namespace should just contain definitions ^:deprecated of various protocols and record types; associated logic
   should go in `metabase.query-processor.middleware.expand`."
-  (:require [metabase.models
+  (:require [metabase.config :as config]
+            [metabase.models
              [dimension :as dim]
              [field :as field]]
             [metabase.sync.interface :as i]
@@ -579,3 +580,10 @@
                     source-query
                     native-source-query))))
    "Query must specify either `:source-table` or `:source-query`, but not both."))
+
+;; Go ahead and mark all the `->Record` and `map->Record` functions as deprecated too! Just so they show up in red in
+;; Emacs
+(when config/is-dev?
+  (doseq [[_ varr] (ns-publics *ns*)
+          :when (fn? (var-get varr))]
+    (alter-meta! varr assoc :deprecated true)))

src/metabase/query_processor/middleware/add_dimension_projections.clj

@@ -19,10 +19,10 @@
    :remapped_to     nil})
 
 (defn- create-fk-remap-col [fk-field-id dest-field-id remapped-from field-display-name]
-  (i/map->FieldPlaceholder {:fk-field-id fk-field-id
-                            :field-id dest-field-id
-                            :remapped-from remapped-from
-                            :remapped-to nil
+  (i/map->FieldPlaceholder {:fk-field-id        fk-field-id
+                            :field-id           dest-field-id
+                            :remapped-from      remapped-from
+                            :remapped-to        nil
                             :field-display-name field-display-name}))
 
 (defn- row-map-fn [dim-seq]
@@ -32,8 +32,8 @@
                         dim-seq))))
 
 (defn- transform-values-for-col
-  "Converts `VALUES` to a type compatible with the base_type found for `COL`. These values should be directly comparable
-  with the values returned from the database for the given `COL`."
+  "Converts `values` to a type compatible with the base_type found for `col`. These values should be directly comparable
+  with the values returned from the database for the given `col`."
   [{:keys [base_type] :as col} values]
   (cond
     (isa? base_type :type/Decimal)
@@ -61,15 +61,15 @@
         (update :remapped_from #(or % nil)))))
 
 (defn- col->dim-map
-  [idx {{remap-to :dimension-name remap-type :dimension-type field-id :field-id} :dimensions :as col}]
+  [idx {{remap-to :dimension-name, remap-type :dimension-type, field-id :field-id} :dimensions, :as col}]
   (when field-id
     (let [remap-from (:name col)]
-      {:col-index idx
-       :from remap-from
-       :to remap-to
-       :xform-fn (zipmap (transform-values-for-col col (get-in col [:values :values]))
-                         (get-in col [:values :human-readable-values]))
-       :new-column (create-remapped-col remap-to remap-from)
+      {:col-index      idx
+       :from           remap-from
+       :to             remap-to
+       :xform-fn       (zipmap (transform-values-for-col col (get-in col [:values :values]))
+                               (get-in col [:values :human-readable-values]))
+       :new-column     (create-remapped-col remap-to remap-from)
        :dimension-type remap-type})))
 
 (defn- create-remap-col-pairs
@@ -115,16 +115,16 @@
   the column information needs to be updated with what it's being remapped from and the user specified name for the
   remapped column."
   [results]
-  (let [indexed-dims (keep-indexed col->dim-map (:cols results))
+  (let [indexed-dims       (keep-indexed col->dim-map (:cols results))
         internal-only-dims (filter #(= :internal (:dimension-type %)) indexed-dims)
-        remap-fn (row-map-fn internal-only-dims)
-        columns (concat (:cols results)
-                        (map :new-column internal-only-dims))
-        from->to (reduce (fn [acc {:keys [remapped_from name]}]
-                           (if remapped_from
-                             (assoc acc remapped_from name)
-                             acc))
-                         {} columns)]
+        remap-fn           (row-map-fn internal-only-dims)
+        columns            (concat (:cols results)
+                                   (map :new-column internal-only-dims))
+        from->to           (reduce (fn [acc {:keys [remapped_from name]}]
+                                     (if remapped_from
+                                       (assoc acc remapped_from name)
+                                       acc))
+                                   {} columns)]
     (-> results
         (update :columns into (map :to internal-only-dims))
         ;; TODO - this code doesn't look right... why use `update` if we're not using the value we're updating?

src/metabase/query_processor/middleware/add_implicit_clauses.clj

 (ns metabase.query-processor.middleware.add-implicit-clauses
   "Middlware for adding an implicit `:fields` and `:order-by` clauses to certain queries."
   (:require [clojure.tools.logging :as log]
+            [honeysql.core :as hsql]
+            [metabase
+             [db :as mdb]
+             [util :as u]]
+            [metabase.mbql
+             [schema :as mbql.s]
+             [util :as mbql.u]]
             [metabase.models.field :refer [Field]]
-            [metabase.query-processor
-             [interface :as i]
-             [sort :as sort]
-             [store :as qp.store]
-             [util :as qputil]]
-            [metabase.query-processor.middleware.resolve :as resolve]
-            [toucan
-             [db :as db]
-             [hydrate :refer [hydrate]]]))
+            [metabase.query-processor.store :as qp.store]
+            [metabase.util
+             [i18n :refer [trs]]
+             [schema :as su]]
+            [schema.core :as s]
+            [toucan.db :as db]))
 
-(defn- fetch-fields-for-souce-table-id [source-table-id]
-  (map resolve/rename-mb-field-keys
-       (-> (db/select [Field :name :display_name :base_type :special_type :visibility_type :table_id :id :position
-                       :description :fingerprint]
-             :table_id        source-table-id
-             :active          true
-             :visibility_type [:not-in ["sensitive" "retired"]]
-             :parent_id       nil
-             {:order-by [[:position :asc]
-                         [:id :desc]]})
-            (hydrate :values)
-            (hydrate :dimensions))))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                              Add Implicit Fields                                               |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- fields-for-source-table
-  "Return the all fields for SOURCE-TABLE, for use as an implicit `:fields` clause."
-  [{source-table-id :source-table, :as inner-query}]
-  (let [{source-table-id :id, :as source-table} (qp.store/table source-table-id)]
-    ;; Sort the implicit FIELDS so the SQL (or other native query) that gets generated (mostly) approximates the 'magic' sorting
-    ;; we do on the results. This is done so when the outer query we generate is a `SELECT *` the order doesn't change
-    (for [field (sort/sort-fields inner-query (fetch-fields-for-souce-table-id source-table-id))
-          :let  [field (-> field
-                           resolve/convert-db-field
-                           (resolve/resolve-table {[nil source-table-id] source-table}))]]
-      (if (qputil/datetime-field? field)
-        (i/map->DateTimeField {:field field, :unit :default})
-        field))))
+(defn- datetime-field? [{:keys [base_type special_type]}]
+  (or (isa? base_type :type/DateTime)
+      (isa? special_type :type/DateTime)))
 
-(defn- should-add-implicit-fields? [{:keys [fields breakout source-table], aggregations :aggregation}]
-  (and source-table ; if query is using another query as its source then there will be no table to add nested fields for
+(s/defn ^:private sorted-implicit-fields-for-table :- [mbql.s/Field]
+  "For use when adding implicit Field IDs to a query. Return a sequence of field clauses, sorted by the rules listed
+  in `metabase.query-processor.sort`, for all the Fields in a given Table."
+  [table-id :- su/IntGreaterThanZero]
+  (for [field (db/select [Field :id :base_type :special_type]
+                :table_id        table-id
+                :active          true
+                :visibility_type [:not-in ["sensitive" "retired"]]
+                :parent_id       nil
+                {:order-by [
+                            ;; we can skip 1-3 because queries w/ implicit Field IDs queries won't have
+                            ;; breakouts or fields clauses, and aggregation isn't an actual Field in the DB
+                            ;; anyway
+                            ;;
+                            ;; 4A. position
+                            [:position :asc]
+                            ;; 4B. special_type: :type/PK, :type/Name, then others
+                            [(hsql/call :case
+                               (mdb/isa :special_type :type/PK)   0
+                               (mdb/isa :special_type :type/Name) 1
+                               :else                              2)
+                             :asc]
+                            ;; 4C. name
+                            [:%lower.name :asc]]})]
+    (if (datetime-field? field)
+      ;; implicit datetime Fields get bucketing of `:default`. This is so other middleware doesn't try to give it
+      ;; default bucketing of `:day`
+      [:datetime-field [:field-id (u/get-id field)] :default]
+      [:field-id (u/get-id field)])))
+
+
+(s/defn ^:private should-add-implicit-fields?
+  [{:keys [fields breakout source-table], aggregations :aggregation} :- mbql.s/MBQLQuery]
+  ;; if query is using another query as its source then there will be no table to add nested fields for
+  (and source-table
        (not (or (seq aggregations)
                 (seq breakout)
                 (seq fields)))))
 
-(defn- add-implicit-fields [{:keys [source-table], :as inner-query}]
+(s/defn ^:private add-implicit-fields :- mbql.s/Query
+  "For MBQL queries with no aggregation, add a `:fields` containing all Fields in the source Table as well as any
+  expressions definied in the query."
+  [{{source-table-id :source-table, :as inner-query} :query, :as query} :- mbql.s/Query]
   (if-not (should-add-implicit-fields? inner-query)
-    inner-query
-    ;; this is a structured `:rows` query, so lets add a `:fields` clause with all fields from the source table + expressions
+    query
+    ;; add a `fields-is-implict` key to the query, which is used to determine how Fields are sorted in the `sort`
+    ;; middleware.
     (let [inner-query (assoc inner-query :fields-is-implicit true)
-          fields      (fields-for-source-table inner-query)
+          fields      (sorted-implicit-fields-for-table source-table-id)
+          ;; generate a new expression ref clause for each expression defined in the query.
           expressions (for [[expression-name] (:expressions inner-query)]
-                        (i/strict-map->ExpressionRef {:expression-name (name expression-name)}))]
+                        ;; TODO - we need to wrap this in `u/keyword->qualified-name` because `:expressions` uses
+                        ;; keywords as keys. We can remove this call once we fix that.
+                        [:expression (u/keyword->qualified-name expression-name)])]
+      ;; if the Table has no Fields, log a warning.
       (when-not (seq fields)
-        (log/warn (format "Table '%s' has no Fields associated with it." (:name source-table))))
-      (assoc inner-query
-        :fields (concat fields expressions)))))
+        (log/warn (trs "Table ''{0}'' has no Fields associated with it." (:name (qp.store/table source-table-id)))))
+      ;; add the fields & expressions under the `:fields` clause
+      (assoc-in query [:query :fields] (vec (concat fields expressions))))))
 
 
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                        Add Implicit Breakout Order Bys                                         |
+;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- add-implicit-breakout-order-by
-  "`Fields` specified in `breakout` should add an implicit ascending `order-by` subclause *unless* that field is *explicitly* referenced in `order-by`."
-  [{breakout-fields :breakout, order-by :order-by, :as inner-query}]
-  (let [order-by-fields                   (set (map (comp #(select-keys % [:field-id :fk-field-id]) :field) order-by))
-        implicit-breakout-order-by-fields (remove (comp order-by-fields #(select-keys % [:field-id :fk-field-id]))
-                                                  breakout-fields)]
-    (cond-> inner-query
-      (seq implicit-breakout-order-by-fields) (update :order-by concat (for [field implicit-breakout-order-by-fields]
-                                                                         {:field field, :direction :ascending})))))
+(s/defn ^:private add-implicit-breakout-order-by :- mbql.s/Query
+  "Fields specified in `breakout` should add an implicit ascending `order-by` subclause *unless* that Field is already
+  *explicitly* referenced in `order-by`."
+  [{{breakouts :breakout} :query, :as query} :- mbql.s/Query]
+  ;; Add a new [:asc <breakout-field>] clause for each breakout. The cool thing is `add-order-by-clause` will
+  ;; automatically ignore new ones that are reference Fields already in the order-by clause
+  (reduce mbql.u/add-order-by-clause query (for [breakout breakouts]
+                                             [:asc breakout])))
 
-(defn- add-implicit-clauses-to-inner-query [inner-query]
-  (cond-> (add-implicit-fields (add-implicit-breakout-order-by inner-query))
-    ;; if query has a source query recursively add implicit clauses to that too as needed
-    (:source-query inner-query) (update :source-query add-implicit-clauses-to-inner-query)))
 
-(defn- maybe-add-implicit-clauses [query]
-  (if-not (qputil/mbql-query? query)
-    query
-    (update query :query add-implicit-clauses-to-inner-query)))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; |                                                   Middleware                                                   |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(s/defn ^:private add-implicit-mbql-clauses :- mbql.s/Query
+  [{{:keys [source-query]} :query, :as query} :- mbql.s/Query]
+  (cond-> (-> query add-implicit-breakout-order-by add-implicit-fields)
+    ;; if query has an MBQL source query recursively add implicit clauses to that too as needed
+    (and source-query (not (:native source-query)))
+    (update-in [:query :source-query] (fn [source-query]
+                                        (:query (add-implicit-mbql-clauses
+                                                 (assoc query :query source-query)))))))
+
+(defn- maybe-add-implicit-clauses
+  [{query-type :type, :as query}]
+  (cond-> query
+    (= query-type :query) add-implicit-mbql-clauses))
 
 (defn add-implicit-clauses
   "Add an implicit `fields` clause to queries with no `:aggregation`, `breakout`, or explicit `:fields` clauses.

src/metabase/query_processor/middleware/auto_bucket_datetime_breakouts.clj

+(ns metabase.query-processor.middleware.auto-bucket-datetime-breakouts
+  "Middleware for automatically bucketing unbucketed `:type/DateTime` breakout Fields with `:day` bucketing."
+  (:require [metabase.mbql
+             [schema :as mbql.s]
+             [util :as mbql.u]]
+            [metabase.models.field :refer [Field]]
+            [metabase.util :as u]
+            [metabase.util.schema :as su]
+            [schema.core :as s]
+            [toucan.db :as db]))
+
+(def ^:private FieldTypeInfo
+  {:id           su/IntGreaterThanZero
+   :base_type    (s/maybe su/FieldType)
+   :special_type (s/maybe su/FieldType)})
+
+(s/defn ^:private is-datetime-field?
+  [{base-type :base_type, special-type :special_type} :- (s/maybe FieldTypeInfo)]
+  (or (isa? base-type :type/DateTime)
+      (isa? special-type :type/DateTime)))
+
+(s/defn ^:private unbucketed-breakouts->field-id->type-info :- {su/IntGreaterThanZero (s/maybe FieldTypeInfo)}
+  "Fetch a map of Field ID -> type information for the Fields referred to by the `unbucketed-breakouts`."
+  [unbucketed-breakouts :- (su/non-empty [mbql.s/field-id])]
+  (u/key-by :id (db/select [Field :id :base_type :special_type]
+                  :id [:in (set (map second unbucketed-breakouts))])))
+
+(s/defn ^:private wrap-unbucketed-datetime-breakouts :-  [mbql.s/Field]
+  "Wrap each breakout in `breakouts` in a `:datetime-field` clause if appropriate; look at corresponing type
+  information in `field-id->type-inf` to see if we should do so."
+  [breakouts :- [mbql.s/Field], field-id->type-info :- {su/IntGreaterThanZero (s/maybe FieldTypeInfo)}]
+  (for [breakout breakouts]
+    (if (and (mbql.u/is-clause? :field-id breakout)
+             (is-datetime-field? (field-id->type-info (second breakout))))
+      [:datetime-field breakout :day]
+      breakout)))
+
+(s/defn ^:private auto-bucket-datetime-breakouts* :- mbql.s/Query
+  [{{breakouts :breakout} :query, :as query} :- mbql.s/Query]
+  ;; find any breakouts in the query that are just plain `[:field-id ...]` clauses
+  (if-let [unbucketed-breakouts (seq (filter (partial mbql.u/is-clause? :field-id) breakouts))]
+    ;; if we found some unbuketed breakouts, fetch the Fields & type info that are referred to by those breakouts...
+    (let [field-id->type-info (unbucketed-breakouts->field-id->type-info unbucketed-breakouts)]
+      ;; ...and then update each breakout by wrapping it if appropriate
+      (update-in query [:query :breakout] #(wrap-unbucketed-datetime-breakouts % field-id->type-info)))
+    ;; otherwise if there are no unbuketed breakouts return the query as-is
+    query))
+
+(defn auto-bucket-datetime-breakouts
+  "Middleware that automatically wraps breakout `:field-id` clauses in `[:datetime-field ... :day]` if the Field they
+  refer to has a type that derives from `:type/DateTime`. (This is done for historic reasons, before datetime
+  bucketing was added to MBQL; datetime Fields defaulted to breaking out by day. We might want to revisit this
+  behavior in the future.)"
+  [qp]
+  (comp qp auto-bucket-datetime-breakouts*))

src/metabase/query_processor/middleware/expand_macros.clj

@@ -7,15 +7,16 @@
    TODO - this namespace is ancient and written with MBQL '95 in mind, e.g. it is case-sensitive.
    At some point this ought to be reworked to be case-insensitive and cleaned up."
   (:require [clojure.tools.logging :as log]
-            [metabase.mbql.util :as mbql.u]
+            [metabase.mbql
+             [schema :as mbql.s]
+             [util :as mbql.u]]
             [metabase.models
              [metric :refer [Metric]]
              [segment :refer [Segment]]]
-            [metabase.query-processor
-             [interface :as i]
-             [util :as qputil]]
+            [metabase.query-processor.interface :as i]
             [metabase.util :as u]
             [puppetlabs.i18n.core :refer [tru]]
+            [schema.core :as s]
             [toucan.db :as db]))
 
 (defn ga-metric-or-segment?
@@ -44,7 +45,8 @@
         (or (:filter (segment-id->definition segment-id))
             (throw (IllegalArgumentException. (str (tru "Segment {0} does not exist, or is invalid." segment-id)))))))))
 
-(defn- expand-segments [{inner-query :query, :as outer-query}]
+(s/defn ^:private expand-segments :- mbql.s/Query
+  [{inner-query :query, :as outer-query} :- mbql.s/Query]
   (if-let [segments (mbql.u/clause-instances :segment inner-query)]
     (replace-segment-clauses outer-query (segment-clauses->id->definition segments))
     outer-query))
@@ -85,7 +87,8 @@
       (add-metrics-filters metric-id->definition)
       (replace-metrics-aggregations metric-id->definition)))
 
-(defn- expand-metrics [query]
+(s/defn ^:private expand-metrics :- mbql.s/Query
+  [query :- mbql.s/Query]
   (if-let [metrics (metrics query)]
     (add-metrics-clauses query (metric-clauses->id->definition metrics))
     query))
@@ -95,16 +98,16 @@
 ;;; |                                                   MIDDLEWARE                                                   |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn- expand-metrics-and-segments
+(s/defn ^:private expand-metrics-and-segments  :- mbql.s/Query
   "Expand the macros (`segment`, `metric`) in a `query`."
-  [query]
+  [query  :- mbql.s/Query]
   (-> query
       expand-metrics
       expand-segments))
 
-
-(defn- expand-macros* [query]
-  (if-not (qputil/mbql-query? query)
+(defn- expand-macros*
+  [{query-type :type, :as query}]
+  (if-not (= query-type :query)
     query
     (u/prog1 (expand-metrics-and-segments query)
       (when (and (not i/*disable-qp-logging*)

src/metabase/query_processor/middleware/fetch_source_query.clj

@@ -2,9 +2,11 @@
   "Middleware responsible for 'hydrating' the source query for queries that use another query as their source."
   (:require [clojure.string :as str]
             [clojure.tools.logging :as log]
+            [metabase.mbql.schema :as mbql.s]
             [metabase.query-processor.interface :as i]
             [metabase.util :as u]
             [metabase.util.i18n :refer [trs]]
+            [schema.core :as s]
             [toucan.db :as db]))
 
 (defn- trim-query
@@ -20,14 +22,16 @@
         trimmed-string))))
 
 (defn- card-id->source-query
-  "Return the source query info for Card with CARD-ID."
+  "Return the source query info for Card with `card-id`."
   [card-id]
   (let [card       (db/select-one ['Card :dataset_query :database_id :result_metadata] :id card-id)
         card-query (:dataset_query card)]
     (assoc (or (:query card-query)
                (when-let [native (:native card-query)]
-                 {:native        (trim-query card-id (:query native))
-                  :template-tags (:template-tags native)})
+                 (merge
+                  {: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))))
       ;; include database ID as well; we'll pass that up the chain so it eventually gets put in its spot in the
       ;; outer-query
@@ -45,7 +49,7 @@
                    ;; 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- ^:deprecated expand-card-source-tables
+(defn- expand-card-source-tables
   "If `source-table` is a Card reference (a string like `card__100`) then replace that with appropriate
   `:source-query` information. Does nothing if `source-table` is a normal ID. Recurses for nested-nested queries."
   [{:keys [source-table], :as inner-query}]
@@ -63,7 +67,8 @@
               :database        (:database source-query)
               :result_metadata (:result_metadata source-query))))))
 
-(defn- fetch-source-query* [{inner-query :query, :as outer-query}]
+(s/defn ^:private fetch-source-query* :- mbql.s/Query
+  [{inner-query :query, :as outer-query} :- mbql.s/Query]
   (if-not inner-query
     ;; for non-MBQL queries there's nothing to do since they have nested queries
     outer-query

src/metabase/query_processor/middleware/resolve.clj

@@ -19,7 +19,9 @@
              [interface :as i]
              [store :as qp.store]
              [util :as qputil]]
-            [metabase.util.date :as du]
+            [metabase.util
+             [date :as du]
+             [schema :as su]]
             [schema.core :as s]
             [toucan
              [db :as db]
@@ -471,9 +473,9 @@
       (map #(resolve-field % field-id->field) fields)
       fields)))
 
-(defn resolve
+(s/defn resolve :- su/Map
   "Resolve placeholders by fetching `Fields`, `Databases`, and `Tables` that are referred to in EXPANDED-QUERY-DICT."
-  [expanded-query-dict]
+  [expanded-query-dict :- su/Map]
   (some-> expanded-query-dict
           record-fk-field-ids
           resolve-fields

src/metabase/query_processor/sort.clj

 (ns metabase.query-processor.sort
   "Code for determining the order columns should be returned in from query results."
-  ;; TODO - This namespace should be called something like `metabase.query-processor.middleware.sort`
   (:require [clojure.tools.logging :as log]
             [metabase.query-processor.interface :as i]
             [metabase.util :as u]))
 
+;; TODO - shouldn't this be `metabase.query-processor.middleware.sort` ??
+
 ;; Fields should be returned in the following order:
 ;; 1.  Breakout Fields
 ;;
@@ -14,8 +15,9 @@
 ;;
 ;; 4.  All other Fields, sorted by:
 ;;     A.  :position (ascending)
-;;         Users can manually specify default Field ordering for a Table in the Metadata admin. In that case, return Fields in the specified
-;;         order; most of the time they'll have the default value of 0, in which case we'll compare...
+;;         Users can manually specify default Field ordering for a Table in the Metadata admin. In that case, return
+;;         Fields in the specified order; most of the time they'll have the default value of 0, in which case we'll
+;;         compare...
 ;;
 ;;     B.  :special_type "group" -- :id Fields, then :name Fields, then everyting else
 ;;         Attempt to put the most relevant Fields first. Order the Fields as follows:
@@ -24,14 +26,19 @@
 ;;         3.  all other Fields
 ;;
 ;;     C.  Field Name
-;;         When two Fields have the same :position and :special_type "group", fall back to sorting Fields alphabetically by name.
-;;         This is arbitrary, but it makes the QP deterministic by keeping the results in a consistent order, which makes it testable.
+;;         When two Fields have the same :position and :special_type "group", fall back to sorting Fields
+;;         alphabetically by name. This is arbitrary, but it makes the QP deterministic by keeping the results in a
+;;         consistent order, which makes it testable.
 
 ;;; ## Field Sorting
 
 ;; We sort Fields with a "importance" vector like [source-importance position special-type-importance name]
 
-(defn- source-importance-fn
+;; THE FOLLOWING FUNCTIONS ARE DEPRECATED: THEY WILL BE REMOVED IN A FUTURE RELEASE.
+;;
+;; We plan to move towards a pattern of figuring out sort order *before* queries are ran, rather than after.
+
+(defn- ^:deprecated source-importance-fn
   "Create a function to return a importance for FIELD based on its source clause in the query.
    e.g. if a Field comes from a `:breakout` clause, we should return that column first in the results."
   [{:keys [fields-is-implicit]}]
@@ -43,7 +50,7 @@
            (= source :fields))      :2-fields
       :else                         :3-other)))
 
-(defn- special-type-importance
+(defn- ^:deprecated special-type-importance
   "Return a importance for FIELD based on the relative importance of its `:special-type`.
    i.e. a Field with special type `:id` should be sorted ahead of all other Fields in the results."
   [{:keys [special-type]}]
@@ -52,7 +59,7 @@
     (isa? special-type :type/Name) :1-name
     :else                          :2-other))
 
-(defn- field-importance-fn
+(defn- ^:deprecated field-importance-fn
   "Create a function to return an \"importance\" vector for use in sorting FIELD."
   [query]
   (let [source-importance (source-importance-fn query)]
@@ -65,7 +72,7 @@
        (special-type-importance field)
        field-name])))
 
-(defn- should-sort? [inner-query]
+(defn- ^:deprecated should-sort? [inner-query]
   (or
    ;; if there's no source query then always sort
    (not (:source-query inner-query))
@@ -75,7 +82,7 @@
    (:aggregation inner-query)
    (:breakout inner-query)))
 
-(defn sort-fields
+(defn ^:deprecated sort-fields
   "Sort FIELDS by their \"importance\" vectors."
   [inner-query fields]
   (if-not (should-sort? inner-query)

src/metabase/query_processor/util.clj

@@ -17,8 +17,9 @@
   [query]
   (= :query (keyword (:type query))))
 
-(defn datetime-field?
-  "Is FIELD a `DateTime` field?"
+(defn ^:deprecated datetime-field?
+  "Is FIELD a `DateTime` field?
+   (DEPRECATED because this only works for expanded queries.)"
   [{:keys [base-type special-type]}]
   (or (isa? base-type :type/DateTime)
       (isa? special-type :type/DateTime)))

test/metabase/driver/generic_sql/native_test.clj

@@ -3,7 +3,7 @@
             [medley.core :as m]
             [metabase.models.database :refer [Database]]
             [metabase.query-processor :as qp]
-            [metabase.test.data :refer :all]
+            [metabase.test.data :as data]
             [toucan.db :as db]))
 
 (def ^:private col-defaults
@@ -20,7 +20,7 @@
                :native_form {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2", :params []}}}
   (-> (qp/process-query {:native   {:query "SELECT ID FROM VENUES ORDER BY ID DESC LIMIT 2"}
                          :type     :native
-                         :database (id)})
+                         :database (data/id)})
       (m/dissoc-in [:data :results_metadata])))
 
 ;; Check that column ordering is maintained
@@ -37,7 +37,7 @@
                :native_form {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2", :params []}}}
   (-> (qp/process-query {:native   {:query "SELECT ID, NAME, CATEGORY_ID FROM VENUES ORDER BY ID DESC LIMIT 2"}
                          :type     :native
-                         :database (id)})
+                         :database (data/id)})
       (m/dissoc-in [:data :results_metadata])))
 
 ;; Check that we get proper error responses for malformed SQL
@@ -46,7 +46,7 @@
          :error  "Column \"ZID\" not found"}
   (dissoc (qp/process-query {:native   {:query "SELECT ZID FROM CHECKINS LIMIT 2"} ; make sure people know it's to be expected
                              :type     :native
-                             :database (id)})
+                             :database (data/id)})
           :stacktrace
           :query
           :expanded-query))

test/metabase/driver/googleanalytics_test.clj

@@ -22,23 +22,15 @@
 
 ;; check that a built-in Metric gets removed from the query and put in `:ga`
 (expect
-  {:query {:filter nil}
-   :ga    {:segment nil, :metrics "ga:users"}}
-  (qp/transform-query {:query {:aggregation [:metric "ga:users"]}}))
+  {:ga {:segment nil, :metrics "ga:users"}}
+  (qp/transform-query {:query {:aggregation [[:metric "ga:users"]]}}))
 
 
 ;; check that a built-in segment gets removed from the query and put in `:ga`
 (expect
-  {:query {:filter nil}
-   :ga    {:segment "gaid::-4", :metrics nil}}
+  {:ga {:segment "gaid::-4", :metrics nil}}
   (qp/transform-query {:query {:filter [:segment "gaid::-4"]}}))
 
-;; check that it still works if wrapped in an `:and`
-(expect
-  {:query {:filter nil}
-   :ga    {:segment "gaid::-4", :metrics nil}}
-  (qp/transform-query {:query {:filter [:and [:segment "gaid::-4"]]}}))
-
 ;; check that other things stay in the order-by clause
 (expect
   {:query {:filter [:< 100 200]}

test/metabase/mbql/util_test.clj

@@ -10,6 +10,22 @@
                                                        [:field-id 10]
                                                        [:field-id 20]]}}))
 
+;; clause-instances shouldn't include subclauses of certain clauses if we don't want them
+(expect
+  [[:field-id 1]
+   [:fk-> [:field-id 2] [:field-id 3]]]
+  (mbql.u/clause-instances #{:field-id :fk->} [[:field-id 1] [:fk-> [:field-id 2] [:field-id 3]]]))
+
+;; ...but we should be able to ask for them
+(expect
+  [[:field-id 1]
+   [:fk-> [:field-id 2] [:field-id 3]]
+   [:field-id 2]
+   [:field-id 3]]
+  (mbql.u/clause-instances #{:field-id :fk->}
+    [[:field-id 1] [:fk-> [:field-id 2] [:field-id 3]]]
+    :include-subclauses? true))
+
 (expect
   [[:field-id 10]
    [:field-id 20]]
@@ -45,3 +61,55 @@
 (expect
   [:= [:field-id 1] 2]
   (mbql.u/simplify-compound-filter [:not [:not [:= [:field-id 1] 2]]]))
+
+;; can we add an order-by clause to a query?
+(expect
+  {:database 1, :type :query, :query {:source-table 1, :order-by [[:asc [:field-id 10]]]}}
+  (mbql.u/add-order-by-clause {:database 1, :type :query, :query {:source-table 1}} [:asc [:field-id 10]]))
+
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :order-by     [[:asc [:field-id 10]]
+                             [:asc [:field-id 20]]]}}
+  (mbql.u/add-order-by-clause {:database 1
+                               :type     :query
+                               :query    {:source-table 1
+                                          :order-by     [[:asc [:field-id 10]]]}}
+                              [:asc [:field-id 20]]))
+
+;; duplicate clauses should get ignored
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :order-by     [[:asc [:field-id 10]]]}}
+  (mbql.u/add-order-by-clause {:database 1
+                               :type     :query
+                               :query    {:source-table 1
+                                          :order-by     [[:asc [:field-id 10]]]}}
+                              [:asc [:field-id 10]]))
+
+;; as should clauses that reference the same Field
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :order-by     [[:asc [:field-id 10]]]}}
+  (mbql.u/add-order-by-clause {:database 1
+                               :type     :query
+                               :query    {:source-table 1
+                                          :order-by     [[:asc [:field-id 10]]]}}
+                              [:desc [:field-id 10]]))
+
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :order-by     [[:asc [:field-id 10]]]}}
+  (mbql.u/add-order-by-clause {:database 1
+                               :type     :query
+                               :query    {:source-table 1
+                                          :order-by     [[:asc [:field-id 10]]]}}
+                              [:asc [:datetime-field [:field-id 10] :day]]))

test/metabase/query_processor/middleware/add_implicit_clauses_test.clj

+(ns metabase.query-processor.middleware.add-implicit-clauses-test
+  (:require [expectations :refer :all]
+            [metabase.models.field :refer [Field]]
+            [metabase.query-processor.middleware.add-implicit-clauses :as add-implicit-clauses]
+            [metabase.test.data :as data]
+            [metabase.util :as u]
+            [toucan.util.test :as tt]))
+
+;; we should add order-bys for breakout clauses
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :breakout     [[:field-id 1]]
+              :order-by     [[:asc [:field-id 1]]]}}
+  (#'add-implicit-clauses/add-implicit-breakout-order-by
+   {:database 1
+    :type     :query
+    :query    {:source-table 1
+               :breakout     [[:field-id 1]]}}))
+
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :breakout     [[:field-id 2]]
+              :order-by     [[:asc [:field-id 1]]
+                             [:asc [:field-id 2]]]}}
+  (#'add-implicit-clauses/add-implicit-breakout-order-by
+   {:database 1
+    :type     :query
+    :query    {:source-table 1
+               :breakout     [[:field-id 2]]
+               :order-by     [[:asc [:field-id 1]]]}}))
+
+;; ...but not if the Field is already in an order-by
+(expect
+  {:database 1
+   :type    :query
+   :query   {:source-table 1
+             :breakout     [[:field-id 1]]
+             :order-by     [[:asc [:field-id 1]]]}}
+  (#'add-implicit-clauses/add-implicit-breakout-order-by
+   {:database 1
+    :type    :query
+    :query   {:source-table 1
+              :breakout     [[:field-id 1]]
+              :order-by     [[:asc [:field-id 1]]]}}))
+
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :breakout     [[:field-id 1]]
+              :order-by     [[:desc [:field-id 1]]]}}
+  (#'add-implicit-clauses/add-implicit-breakout-order-by
+   {:database 1
+    :type     :query
+    :query    {:source-table 1
+               :breakout     [[:field-id 1]]
+               :order-by     [[:desc [:field-id 1]]]}}))
+
+(expect
+  {:database 1
+   :type     :query
+   :query    {:source-table 1
+              :breakout     [[:datetime-field [:field-id 1] :day]]
+              :order-by     [[:asc [:field-id 1]]]}}
+  (#'add-implicit-clauses/add-implicit-breakout-order-by
+   {:database 1
+    :type     :query
+    :query    {:source-table 1
+               :breakout     [[:datetime-field [:field-id 1] :day]]
+               :order-by     [[:asc [:field-id 1]]]}}))
+
+
+;; We should add sorted implicit Fields for a query with no aggregations
+(expect
+  {:database (data/id)
+   :type     :query
+   :query    {:source-table (data/id :venues)
+              :fields       [ ;; :type/PK Fields should get sorted first
+                             [:field-id (data/id :venues :id)]
+                             ;; followed by :type/Name Fields
+                             [:field-id (data/id :venues :name)]
+                             ;; followed by other Fields sorted by name
+                             [:field-id (data/id :venues :category_id)]
+                             [:field-id (data/id :venues :latitude)]
+                             [:field-id (data/id :venues :longitude)]
+                             [:field-id (data/id :venues :price)]]}}
+  (#'add-implicit-clauses/add-implicit-fields
+   {:database (data/id)
+    :type     :query
+    :query    {:source-table (data/id :venues)}}))
+
+;; when adding sorted implicit Fields, Field positions should be taken into account
+(tt/expect-with-temp [Field [field-1 {:table_id (data/id :venues), :position 1, :name "bbbbb"}]
+                      Field [field-2 {:table_id (data/id :venues), :position 2, :name "aaaaa"}]]
+  {:database (data/id)
+   :type     :query
+   :query    {:source-table (data/id :venues)
+              :fields       [;; all fields with position = 0 should get sorted first according to rules above
+                             [:field-id (data/id :venues :id)]
+                             [:field-id (data/id :venues :name)]
+                             [:field-id (data/id :venues :category_id)]
+                             [:field-id (data/id :venues :latitude)]
+                             [:field-id (data/id :venues :longitude)]
+                             [:field-id (data/id :venues :price)]
+                             ;; followed by position = 1
+                             [:field-id (u/get-id field-1)]
+                             ;; followed by position = 2
+                             [:field-id (u/get-id field-2)]]}}
+  (#'add-implicit-clauses/add-implicit-fields
+   {:database (data/id)
+    :type     :query
+    :query    {:source-table (data/id :venues)}}))
+
+;; datetime Fields should get default bucketing of :day
+(tt/expect-with-temp [Field [field {:table_id (data/id :venues), :position 0, :name "aaaaa", :base_type :type/DateTime}]]
+  {:database (data/id)
+   :type     :query
+   :query    {:source-table (data/id :venues)
+              :fields       [[:field-id (data/id :venues :id)]
+                             [:field-id (data/id :venues :name)]
+                             [:datetime-field [:field-id (u/get-id field)] :default]
+                             [:field-id (data/id :venues :category_id)]
+                             [:field-id (data/id :venues :latitude)]
+                             [:field-id (data/id :venues :longitude)]
+                             [:field-id (data/id :venues :price)]]}}
+  (#'add-implicit-clauses/add-implicit-fields
+   {:database (data/id)
+    :type     :query
+    :query    {:source-table (data/id :venues)}}))

test/metabase/query_processor/middleware/auto_bucket_datetime_breakouts_test.clj

+(ns metabase.query-processor.middleware.auto-bucket-datetime-breakouts-test
+  (:require [expectations :refer [expect]]
+            [metabase.models.field :refer [Field]]
+            [metabase.query-processor.middleware.auto-bucket-datetime-breakouts :as auto-bucket-datetime-breakouts]
+            [metabase.util :as u]
+            [toucan.util.test :as tt]))
+
+(defn- auto-bucket [query]
+  ((auto-bucket-datetime-breakouts/auto-bucket-datetime-breakouts identity)
+   query))
+
+(defn- auto-bucket-mbql [mbql-query]
+  (-> (auto-bucket {:database 1, :type :query, :query mbql-query})
+      :query))
+
+;; does a :type/DateTime Field get auto-bucketed when present in a breakout clause?
+(tt/expect-with-temp [Field [field {:base_type :type/DateTime, :special_type nil}]]
+  {:source-table 1
+   :breakout     [[:datetime-field [:field-id (u/get-id field)] :day]]}
+  (auto-bucket-mbql
+   {:source-table 1
+    :breakout     [[:field-id (u/get-id field)]]}))
+
+;; should be considered to be :type/DateTime based on `special_type` as well
+(tt/expect-with-temp [Field [field {:base_type :type/Integer, :special_type :type/DateTime}]]
+  {:source-table 1
+   :breakout     [[:datetime-field [:field-id (u/get-id field)] :day]]}
+  (auto-bucket-mbql
+   {:source-table 1
+    :breakout     [[:field-id (u/get-id field)]]}))
+
+;; do native queries pass thru unchanged?
+(let [native-query {:database 1, :type :native, :native {:query "SELECT COUNT(cans) FROM birds;"}}]
+  (expect
+    native-query
+    (auto-bucket native-query)))
+
+;; do MBQL queries w/o breakouts pass thru unchanged?
+(expect
+  {:source-table 1}
+  (auto-bucket-mbql
+   {:source-table 1}))
+
+;; does a breakout Field that isn't :type/DateTime pass thru unchnaged?
+(tt/expect-with-temp [Field [field {:base_type :type/Integer, :special_type nil}]]
+  {:source-table 1
+   :breakout     [[:field-id (u/get-id field)]]}
+  (auto-bucket-mbql
+   {:source-table 1
+    :breakout     [[:field-id (u/get-id field)]]}))
+
+;; does a :type/DateTime breakout Field that is already bucketed pass thru unchanged?
+(tt/expect-with-temp [Field [field {:base_type :type/DateTime, :special_type nil}]]
+  {:source-table 1
+   :breakout     [[:datetime-field [:field-id (u/get-id field)] :month]]}
+  (auto-bucket-mbql
+   {:source-table 1
+    :breakout     [[:datetime-field [:field-id (u/get-id field)] :month]]}))
+
+;; does the middleware avoid barfing if for some reason the Field could not be resolved in the DB? (That is the job of
+;; the resolve middleware to worry about that stuff.)
+(expect
+  {:source-table 1
+   :breakout     [[:field-id Integer/MAX_VALUE]]}
+  (auto-bucket-mbql
+   {:source-table 1
+    :breakout     [[:field-id Integer/MAX_VALUE]]}))