Cleanup backported from nested-queries branch

ced090aa · Cam Saül · 2f9bc079 · ced090aa · ced090aa · ced090aa
Commit ced090aa authored 7 years ago by Cam Saül
--- a/src/metabase/api/database.clj
+++ b/src/metabase/api/database.clj
@@ -246,6 +246,7 @@
  (sample-data/add-sample-dataset!)
  (Database :is_sample true))
 ;;; ------------------------------------------------------------ PUT /api/database/:id ------------------------------------------------------------
 (api/defendpoint PUT "/:id"

--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -88,11 +88,11 @@
  (card-with-uuid uuid))
 (defn run-query-for-card-with-id
  "Run the query belonging to Card with CARD-ID with PARAMETERS and other query options (e.g. `:constraints`)."
  [card-id parameters & options]
  (u/prog1 (-> (let [parameters (if (string? parameters) (json/parse-string parameters keyword) parameters)]
+                 ;; run this query with full superuser perms
                 (binding [api/*current-user-permissions-set*     (atom #{"/"})
                           qp/*allow-queries-with-no-executor-id* true]
                   (apply card-api/run-query-for-card card-id, :parameters parameters, :context :public-question, options)))

--- a/src/metabase/api/table.clj
+++ b/src/metabase/api/table.clj
@@ -2,6 +2,7 @@
  "/api/table endpoints."
  (:require [clojure.tools.logging :as log]
            [compojure.core :refer [GET PUT]]
+            [medley.core :as m]
            [metabase
             [sync-database :as sync-database]
             [util :as u]]
@@ -87,6 +88,7 @@
  {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])
      (update-in [:fields] (if (Boolean/parseBoolean include_sensitive_fields)
                             ;; If someone passes include_sensitive_fields return hydrated :fields as-is
                             identity

--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -323,6 +323,17 @@
      (log/warn (format "Don't know how to map class '%s' to a Field base_type, falling back to :type/*." klass))
      :type/*))
+(defn values->base-type
+  "Given a sequence of VALUES, return the most common base type."
+  [values]
+  (->> values
+       (filter (complement nil?))                   ; filter out `nil` values
+       (take 1000)                                  ; take up to 1000 values
+       (group-by (comp class->base-type class))     ; now group by their base-type
+       (sort-by (comp (partial * -1) count second)) ; sort the map into pairs of [base-type count] with highest count as first pair
+       ffirst))                                     ; take the base-type from the first pair
 ;; ## Driver Lookup
 (defn engine->driver

--- a/src/metabase/driver/generic_sql.clj
+++ b/src/metabase/driver/generic_sql.clj
@@ -220,6 +220,7 @@
 (defn honeysql-form->sql+args
  "Convert HONEYSQL-FORM to a vector of SQL string and params, like you'd pass to JDBC."
+  {:style/indent 1}
  [driver honeysql-form]
  {:pre [(map? honeysql-form)]}
  (let [[sql & args] (try (binding [hformat/*subquery?* false]

--- a/src/metabase/driver/generic_sql/query_processor.clj
+++ b/src/metabase/driver/generic_sql/query_processor.clj
@@ -243,7 +243,7 @@
      (h/limit items)
      (h/offset (* items (dec page)))))
-(defn- apply-source-table [_ honeysql-form {{table-name :name, schema :schema} :source-table}]
+(defn- apply-source-table [honeysql-form {{table-name :name, schema :schema} :source-table}]
  {:pre [table-name]}
  (h/from honeysql-form (hx/qualify-and-escape-dots schema table-name)))
@@ -252,7 +252,7 @@
  ;;    will get swapped around and  we'll be left with old version of the function that nobody implements
  ;; 2) This is a vector rather than a map because the order the clauses get handled is important for some drivers.
  ;;    For example, Oracle needs to wrap the entire query in order to apply its version of limit (`WHERE ROWNUM`).
-  [:source-table apply-source-table
+  [:source-table (u/drop-first-arg apply-source-table)
   :aggregation  #'sql/apply-aggregation
   :breakout     #'sql/apply-breakout
   :fields       #'sql/apply-fields

--- a/src/metabase/driver/googleanalytics/query_processor.clj
+++ b/src/metabase/driver/googleanalytics/query_processor.clj
@@ -3,7 +3,7 @@
  (:require [clojure.string :as s]
            [clojure.tools.reader.edn :as edn]
            [medley.core :as m]
-            [metabase.query-processor.expand :as ql]
+            [metabase.query-processor.util :as qputil]
            [metabase.util :as u])
  (:import [com.google.api.services.analytics.model GaData GaData$ColumnHeaders]
           [metabase.query_processor.interface AgFieldRef DateTimeField DateTimeValue Field RelativeDateTimeValue Value]))
@@ -251,7 +251,7 @@
  [{query :query}]
  (let [[aggregation-type metric-name] (first-aggregation query)]
    (when (and aggregation-type
-               (= :metric (ql/normalize-token aggregation-type))
+               (= :metric (qputil/normalize-token aggregation-type))
               (string? metric-name))
      metric-name)))
@@ -266,7 +266,7 @@
 (defn- filter-type ^clojure.lang.Keyword [filter-clause]
  (when (and (sequential? filter-clause)
             (u/string-or-keyword? (first filter-clause)))
-    (ql/normalize-token (first filter-clause))))
+    (qputil/normalize-token (first filter-clause))))
 (defn- compound-filter? [filter-clause]
  (contains? #{:and :or :not} (filter-type filter-clause)))

--- a/src/metabase/driver/mongo/util.clj
+++ b/src/metabase/driver/mongo/util.clj
@@ -137,26 +137,3 @@
     (if *mongo-connection*
       (f# *mongo-connection*)
       (-with-mongo-connection f# ~database))))
-;; TODO - this isn't neccesarily Mongo-specific; consider moving
-(defn values->base-type
-  "Given a sequence of values, return `Field.base_type` in the most ghetto way possible.
-   This just gets counts the types of *every* value and returns the `base_type` for class whose count was highest."
-  [values-seq]
-  {:pre [(sequential? values-seq)]}
-  (or (->> values-seq
-           ;; TODO - why not do a query to return non-nil values of this column instead
-           (filter identity)
-           ;; it's probably fine just to consider the first 1,000 *non-nil* values when trying to type a column instead
-           ;; of iterating over the whole collection. (VALUES-SEQ should be up to 10,000 values, but we don't know how many are
-           ;; nil)
-           (take 1000)
-           (group-by type)
-           ;; create tuples like [Integer count].
-           (map (fn [[klass valus]]
-                  [klass (count valus)]))
-           (sort-by second)
-           last                     ; last result will be tuple with highest count
-           first                    ; keep just the type
-           driver/class->base-type) ; convert to Field base_type
-      :type/*))
--- a/src/metabase/middleware.clj
+++ b/src/metabase/middleware.clj
@@ -100,17 +100,12 @@
  "Return User ID and superuser status for Session with SESSION-ID if it is valid and not expired."
  [session-id]
  (when (and session-id (init-status/complete?))
-    (when-let [session (or (session-with-id session-id)
+    (when-let [session (session-with-id session-id)]
-                           (println "no matching session with ID") ; DEBUG
+      (when-not (session-expired? session)
-                           )]
-      (if (session-expired? session)
-        (printf "session-is-expired! %d min / %d min\n" (session-age-minutes session) (config/config-int :max-session-age)) ; DEBUG
        {:metabase-user-id (:user_id session)
         :is-superuser?    (:is_superuser session)}))))
 (defn- add-current-user-info [{:keys [metabase-session-id], :as request}]
-  (when-not (init-status/complete?)
-    (println "Metabase is not initialized yet!")) ; DEBUG
  (merge request (current-user-info-for-session metabase-session-id)))
 (defn wrap-current-user-id
@@ -347,7 +342,7 @@
    (try (binding [*automatically-catch-api-exceptions* false]
           (handler request))
         (catch Throwable e
-           (log/error (.getMessage e))
+           (log/warn (.getMessage e))
           {:status 400, :body "An error occurred."}))))
 (defn message-only-exceptions

--- a/src/metabase/query_processor/annotate.clj
+++ b/src/metabase/query_processor/annotate.clj
@@ -134,16 +134,19 @@
                       (expression-aggregate-field-info ag)
                       (aggregate-field-info ag))))))
 (defn- generic-info-for-missing-key
-  "Return a set of bare-bones metadata for a Field named K when all else fails."
+  "Return a set of bare-bones metadata for a Field named K when all else fails.
-  [k]
+   Scan the INITIAL-VALUES of K in an attempt to determine the `base-type`."
-  {:base-type          :type/*
+  [k & [initial-values]]
+  {:base-type          (if (seq initial-values)
+                         (driver/values->base-type initial-values)
+                         :type/*)
   :preview-display    true
   :special-type       nil
   :field-name         k
   :field-display-name k})
 (defn- info-for-duplicate-field
  "The Clojure JDBC driver automatically appends suffixes like `count_2` to duplicate columns if multiple columns come back with the same name;
   since at this time we can't resolve those normally (#1786) fall back to using the metadata for the first column (e.g., `count`).
@@ -159,14 +162,14 @@
 (defn- info-for-missing-key
  "Metadata for a field named K, which we weren't able to resolve normally.
   If possible, we work around This defaults to generic information "
-  [fields k]
+  [fields k initial-values]
  (or (info-for-duplicate-field fields k)
-      (generic-info-for-missing-key k)))
+      (generic-info-for-missing-key k initial-values)))
 (defn- add-unknown-fields-if-needed
  "When create info maps for any fields we didn't expect to come back from the query.
   Ideally, this should never happen, but on the off chance it does we still want to return it in the results."
-  [actual-keys fields]
+  [actual-keys initial-rows fields]
  {:pre [(set? actual-keys) (every? keyword? actual-keys)]}
  (let [expected-keys (u/prog1 (set (map :field-name fields))
                        (assert (every? keyword? <>)))
@@ -175,7 +178,7 @@
      (log/warn (u/format-color 'yellow "There are fields we weren't expecting in the results: %s\nExpected: %s\nActual: %s"
                  missing-keys expected-keys actual-keys)))
    (concat fields (for [k missing-keys]
-                     (info-for-missing-key fields k)))))
+                     (info-for-missing-key fields k (map k initial-rows))))))
 (defn- convert-field-to-expected-format
  "Rename keys, provide default values, etc. for FIELD so it is in the format expected by the frontend."
@@ -238,14 +241,14 @@
 (defn- resolve-sort-and-format-columns
  "Collect the Fields referenced in QUERY, sort them according to the rules at the top
   of this page, format them as expected by the frontend, and return the results."
-  [query result-keys]
+  [query result-keys initial-rows]
  {:pre [(set? result-keys)]}
  (when (seq result-keys)
    (->> (collect-fields (dissoc query :expressions))
         (map qualify-field-name)
         (add-aggregate-fields-if-needed query)
         (map (u/rpartial update :field-name keyword))
-         (add-unknown-fields-if-needed result-keys)
+         (add-unknown-fields-if-needed result-keys initial-rows)
         (sort/sort-fields query)
         (map convert-field-to-expected-format)
         (filter (comp (partial contains? result-keys) :name))
@@ -261,7 +264,7 @@
  [query {:keys [columns rows], :as results}]
  (let [row-maps (for [row rows]
                   (zipmap columns row))
-        cols    (resolve-sort-and-format-columns (:query query) (set columns))
+        cols    (resolve-sort-and-format-columns (:query query) (set columns) (take 10 row-maps))
        columns (mapv :name cols)]
    (assoc results
      :cols    (vec (for [col cols]

--- a/src/metabase/query_processor/expand.clj
+++ b/src/metabase/query_processor/expand.clj
@@ -2,25 +2,16 @@
  "Converts a Query Dict as received by the API into an *expanded* one that contains extra information that will be needed to
   construct the appropriate native Query, and perform various post-processing steps such as Field ordering."
  (:refer-clojure :exclude [< <= > >= = != and or not filter count distinct sum min max + - / *])
-  (:require [clojure
+  (:require [clojure.core :as core]
-             [core :as core]
-             [string :as str]]
            [clojure.tools.logging :as log]
-            [metabase.query-processor.interface :as i]
+            [metabase.query-processor
+             [interface :as i]
+             [util :as qputil]]
            [metabase.util :as u]
            [metabase.util.schema :as su]
            [schema.core :as s])
-  (:import [metabase.query_processor.interface AgFieldRef BetweenFilter ComparisonFilter CompoundFilter Expression ExpressionRef FieldPlaceholder RelativeDatetime StringFilter ValuePlaceholder]))
+  (:import [metabase.query_processor.interface AgFieldRef BetweenFilter ComparisonFilter CompoundFilter Expression ExpressionRef
+            FieldPlaceholder RelativeDatetime StringFilter Value ValuePlaceholder]))
-;;; # ------------------------------------------------------------ Token dispatch ------------------------------------------------------------
-(s/defn ^:always-validate normalize-token :- s/Keyword
-  "Convert a string or keyword in various cases (`lisp-case`, `snake_case`, or `SCREAMING_SNAKE_CASE`) to a lisp-cased keyword."
-  [token :- su/KeywordOrString]
-  (-> (name token)
-      str/lower-case
-      (str/replace #"_" "-")
-      keyword))
 ;;; # ------------------------------------------------------------ Clause Handlers ------------------------------------------------------------
@@ -38,7 +29,7 @@
  [id :- su/IntGreaterThanZero]
  (i/map->FieldPlaceholder {:field-id id}))
-(s/defn ^:private ^:always-validate field :- i/AnyFieldOrExpression
+(s/defn ^:private ^:always-validate field :- i/AnyField
  "Generic reference to a `Field`. F can be an integer Field ID, or various other forms like `fk->` or `aggregation`."
  [f]
  (if (integer? f)
@@ -58,7 +49,7 @@
  ([f _ unit] (log/warn (u/format-color 'yellow (str "The syntax for datetime-field has changed in MBQL '98. [:datetime-field <field> :as <unit>] is deprecated. "
                                                     "Prefer [:datetime-field <field> <unit>] instead.")))
              (datetime-field f unit))
-  ([f unit]   (assoc (field f) :datetime-unit (normalize-token unit))))
+  ([f unit]   (assoc (field f) :datetime-unit (qputil/normalize-token unit))))
 (s/defn ^:ql ^:always-validate fk-> :- FieldPlaceholder
  "Reference to a `Field` that belongs to another `Table`. DEST-FIELD-ID is the ID of this Field, and FK-FIELD-ID is the ID of the foreign key field
@@ -72,11 +63,12 @@
  (i/map->FieldPlaceholder {:fk-field-id fk-field-id, :field-id dest-field-id}))
-(s/defn ^:private ^:always-validate value :- ValuePlaceholder
+(s/defn ^:private ^:always-validate value :- (s/cond-pre Value ValuePlaceholder)
  "Literal value. F is the `Field` it relates to, and V is `nil`, or a boolean, string, numerical, or datetime value."
  [f v]
  (cond
    (instance? ValuePlaceholder v) v
+    (instance? Value v)            v
    :else                          (i/map->ValuePlaceholder {:field-placeholder (field f), :value v})))
 (s/defn ^:private ^:always-validate field-or-value
@@ -94,11 +86,11 @@
     (relative-datetime :current)
     (relative-datetime -31 :day)"
-  ([n]                (s/validate (s/eq :current) (normalize-token n))
+  ([n]                (s/validate (s/eq :current) (qputil/normalize-token n))
                      (relative-datetime 0 nil))
  ([n :- s/Int, unit] (i/map->RelativeDatetime {:amount n, :unit (if (nil? unit)
                                                                   :day                        ; give :unit a default value so we can simplify the schema a bit and require a :unit
-                                                                   (normalize-token unit))})))
+                                                                   (qputil/normalize-token unit))})))
 (s/defn ^:ql ^:always-validate expression :- ExpressionRef
  {:added "0.17.0"}
@@ -170,8 +162,8 @@
                                                         ;; make sure the ag map is still typed correctly
                                                         (u/prog1 (cond
                                                                    (:operator ag) (i/map->Expression ag)
-                                                                    (:field ag)    (i/map->AggregationWithField    (update ag :aggregation-type normalize-token))
+                                                                    (:field ag)    (i/map->AggregationWithField    (update ag :aggregation-type qputil/normalize-token))
-                                                                    :else          (i/map->AggregationWithoutField (update ag :aggregation-type normalize-token)))
+                                                                    :else          (i/map->AggregationWithoutField (update ag :aggregation-type qputil/normalize-token)))
                                                           (s/validate i/Aggregation <>)))))))
  ;; also handle varargs for convenience
@@ -288,7 +280,7 @@
    (filter {} (time-interval (field-id 100) :current :day)) "
  [f n unit]
  (if-not (integer? n)
-    (case (normalize-token n)
+    (case (qputil/normalize-token n)
      :current (recur f  0 unit)
      :last    (recur f -1 unit)
      :next    (recur f  1 unit))
@@ -353,7 +345,7 @@
    (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))))
+                          (order-by-subclause (qputil/normalize-token direction) f))))
 (defn ^:ql order-by
  "Specify how ordering should be done for this query.
@@ -432,7 +424,7 @@
     (fn-for-token :starts-with) -> #'starts-with"
  [token]
-  (let [token (normalize-token token)]
+  (let [token (qputil/normalize-token token)]
    (core/or (token->ql-fn token)
             (throw (Exception. (str "Illegal clause (no matching fn found): " token))))))
@@ -506,4 +498,4 @@
     (is-clause? :field-id [\"FIELD-ID\" 2000]) ; -> true"
  [clause-keyword clause]
  (core/and (sequential? clause)
-            (core/= (normalize-token (first clause)) clause-keyword)))
+            (core/= (qputil/normalize-token (first clause)) clause-keyword)))
--- a/src/metabase/query_processor/interface.clj
+++ b/src/metabase/query_processor/interface.clj
@@ -33,13 +33,21 @@
   Not neccesarily bound when using various functions like `fk->` in the REPL."
  nil)
+(defn driver-supports?
+  "Does the currently bound `*driver*` support FEATURE?
+   (This returns `nil` if `*driver*` is unbound. `*driver*` is always bound when running queries the normal way,
+   but may not be when calling this function directly from the REPL.)"
+  [feature]
+  (when *driver*
+    ((resolve 'metabase.driver/driver-supports?) *driver* feature)))
 ;; `assert-driver-supports` doesn't run check when `*driver*` is unbound (e.g., when used in the REPL)
 ;; Allows flexibility when composing queries for tests or interactive development
 (defn assert-driver-supports
  "When `*driver*` is bound, assert that is supports keyword FEATURE."
  [feature]
  (when *driver*
-    (when-not (contains? ((resolve 'metabase.driver/features) *driver*) feature)
+    (when-not (driver-supports? feature)
      (throw (Exception. (str (name feature) " is not supported by this driver."))))))
 ;; Expansion Happens in a Few Stages:
@@ -70,9 +78,11 @@
    "Return a vector of name components of the form `[table-name parent-names... field-name]`"))
-;;; # ------------------------------------------------------------ "RESOLVED" TYPES: FIELD + VALUE ------------------------------------------------------------
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; |                                                                             FIELDS                                                                             |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
-;; Field is the expansion of a Field ID in the standard QL
+;; Field is the "expanded" form of a Field ID (field reference) in MBQL
 (s/defrecord Field [field-id           :- su/IntGreaterThanZero
                    field-name         :- su/NonBlankString
                    field-display-name :- su/NonBlankString
@@ -98,6 +108,7 @@
            [table-name])
          field-name)))
+;;; DateTimeField
 (def ^:const datetime-field-units
  "Valid units for a `DateTimeField`."
@@ -122,7 +133,7 @@
  (contains? relative-datetime-value-units (keyword unit)))
-;; wrapper around Field
+;; DateTimeField is just a simple wrapper around Field
 (s/defrecord DateTimeField [field :- Field
                            unit  :- DatetimeFieldUnit]
  clojure.lang.Named
@@ -136,78 +147,53 @@
    [nil expression-name]))
-;; Value is the expansion of a value within a QL clause
+;;; Placeholder Types
-;; Information about the associated Field is included for convenience
-(s/defrecord Value [value   :- (s/maybe (s/cond-pre s/Bool s/Num su/NonBlankString))
-                    field   :- (s/named (s/cond-pre Field ExpressionRef)   ; TODO - Value doesn't need the whole field, just the relevant type info / units
-                                        "field or expression reference")])
-;; e.g. an absolute point in time (literal)
-(s/defrecord DateTimeValue [value :- Timestamp
-                            field :- DateTimeField])
-(s/defrecord RelativeDateTimeValue [amount :- s/Int
-                                    unit   :- DatetimeValueUnit
-                                    field  :- DateTimeField])
-(defprotocol ^:private IDateTimeValue
-  (unit [this]
-    "Get the `unit` associated with a `DateTimeValue` or `RelativeDateTimeValue`.")
-  (add-date-time-units [this n]
-    "Return a new `DateTimeValue` or `RelativeDateTimeValue` with N `units` added to it."))
-(extend-protocol IDateTimeValue
-  DateTimeValue
-  (unit                [this]   (:unit (:field this)))
-  (add-date-time-units [this n] (assoc this :value (u/relative-date (unit this) n (:value this))))
-  RelativeDateTimeValue
-  (unit                [this]   (:unit this))
-  (add-date-time-units [this n] (update this :amount (partial + n))))
-;;; # ------------------------------------------------------------ PLACEHOLDER TYPES: FIELDPLACEHOLDER + VALUEPLACEHOLDER ------------------------------------------------------------
 ;; Replace Field IDs with these during first pass
 (s/defrecord FieldPlaceholder [field-id      :- su/IntGreaterThanZero
                               fk-field-id   :- (s/maybe (s/constrained su/IntGreaterThanZero
-                                                                        (fn [_] (or (assert-driver-supports :foreign-keys) true))
+                                                                        (fn [_] (or (assert-driver-supports :foreign-keys) true)) ; assert-driver-supports will throw Exception if driver is bound
-                                                                        "foreign-keys is not supported by this driver."))
+                                                                        "foreign-keys is not supported by this driver."))         ; and driver does not support foreign keys
                               datetime-unit :- (s/maybe (apply s/enum datetime-field-units))])
 (s/defrecord AgFieldRef [index :- s/Int])
 ;; TODO - add a method to get matching expression from the query?
-(def FieldPlaceholderOrAgRef
-  "Schema for either a `FieldPlaceholder` or `AgFieldRef`."
-  (s/named (s/cond-pre FieldPlaceholder AgFieldRef) "Valid field (not a field ID or aggregate field reference)"))
 (def FieldPlaceholderOrExpressionRef
  "Schema for either a `FieldPlaceholder` or `ExpressionRef`."
  (s/named (s/cond-pre FieldPlaceholder ExpressionRef)
           "Valid field or expression reference."))
 (s/defrecord RelativeDatetime [amount :- s/Int
                               unit   :- DatetimeValueUnit])
+(declare Aggregation AnyField AnyValueLiteral)
-(declare RValue Aggregation)
 (def ^:private ExpressionOperator (s/named (s/enum :+ :- :* :/) "Valid expression operator"))
 (s/defrecord Expression [operator   :- ExpressionOperator
-                         args       :- [(s/cond-pre (s/recursive #'RValue)
+                         args       :- [(s/cond-pre (s/recursive #'AnyValueLiteral)
+                                                    (s/recursive #'AnyField)
                                                    (s/recursive #'Aggregation))]
                         custom-name :- (s/maybe su/NonBlankString)])
-(def AnyFieldOrExpression
-  "Schema for a `FieldPlaceholder`, `AgRef`, or `Expression`."
-  (s/named (s/cond-pre ExpressionRef Expression FieldPlaceholderOrAgRef)
-           "Valid field, ag field reference, expression, or expression reference."))
+(def AnyField
+  "Schema for a anything that is considered a valid 'field'."
+  (s/named (s/cond-pre Field
+                       FieldPlaceholder
+                       AgFieldRef
+                       Expression
+                       ExpressionRef)
+           "AnyField: field, ag field reference, expression, expression reference, or field literal."))
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; |                                                                             VALUES                                                                             |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
 (def LiteralDatetimeString
  "Schema for an MBQL datetime string literal, in ISO-8601 format."
@@ -225,14 +211,48 @@
  "Schema for something that is orderable value in MBQL (either a number or datetime)."
  (s/named (s/cond-pre s/Num Datetime) "Valid orderable value (must be number or datetime)"))
-(def AnyValue
+(def AnyValueLiteral
-  "Schema for anything that is a considered a valid value in MBQL - `nil`, a `Boolean`, `Number`, `String`, or relative datetime form."
+  "Schema for anything that is a considered a valid value literal in MBQL - `nil`, a `Boolean`, `Number`, `String`, or relative datetime form."
  (s/named (s/maybe (s/cond-pre s/Bool su/NonBlankString OrderableValue)) "Valid value (must be nil, boolean, number, string, or a relative-datetime form)"))
+;; Value is the expansion of a value within a QL clause
+;; Information about the associated Field is included for convenience
+;; TODO - Value doesn't need the whole field, just the relevant type info / units
+(s/defrecord Value [value   :- AnyValueLiteral
+                    field   :- (s/recursive #'AnyField)])
+;; e.g. an absolute point in time (literal)
+(s/defrecord DateTimeValue [value :- Timestamp
+                            field :- DateTimeField])
+(s/defrecord RelativeDateTimeValue [amount :- s/Int
+                                    unit   :- DatetimeValueUnit
+                                    field  :- DateTimeField])
+(defprotocol ^:private IDateTimeValue
+  (unit [this]
+    "Get the `unit` associated with a `DateTimeValue` or `RelativeDateTimeValue`.")
+  (add-date-time-units [this n]
+    "Return a new `DateTimeValue` or `RelativeDateTimeValue` with N `units` added to it."))
+(extend-protocol IDateTimeValue
+  DateTimeValue
+  (unit                [this]   (:unit (:field this)))
+  (add-date-time-units [this n] (assoc this :value (u/relative-date (unit this) n (:value this))))
+  RelativeDateTimeValue
+  (unit                [this]   (:unit this))
+  (add-date-time-units [this n] (update this :amount (partial + n))))
+;;; Placeholder Types
 ;; Replace values with these during first pass over Query.
 ;; Include associated Field ID so appropriate the info can be found during Field resolution
 (s/defrecord ValuePlaceholder [field-placeholder :- FieldPlaceholderOrExpressionRef
-                               value             :- AnyValue])
+                               value             :- AnyValueLiteral])
 (def OrderableValuePlaceholder
  "`ValuePlaceholder` schema with the additional constraint that the value be orderable (a number or datetime)."
@@ -242,21 +262,16 @@
  "`ValuePlaceholder` schema with the additional constraint that the value be a string/"
  (s/constrained ValuePlaceholder (comp string? :value) ":value must be a string"))
-(def FieldOrAnyValue
+(def AnyFieldOrValue
-  "Schema that accepts either a `FieldPlaceholder` or `ValuePlaceholder`."
+  "Schema that accepts anything normally considered a field (including expressions and literals) *or* a value or value placehoder."
-  (s/named (s/cond-pre FieldPlaceholder ValuePlaceholder) "Field or value"))
+  (s/named (s/cond-pre AnyField Value ValuePlaceholder) "Field or value"))
-;; (def FieldOrOrderableValue (s/named (s/cond-pre FieldPlaceholder OrderableValuePlaceholder) "Field or orderable value (number or datetime)"))
-;; (def FieldOrStringValue    (s/named (s/cond-pre FieldPlaceholder StringValuePlaceholder)    "Field or string literal"))
-(def RValue
-  "Schema for anything that can be an [RValue](https://github.com/metabase/metabase/wiki/Query-Language-'98#rvalues) -
-   a `Field`, `Value`, or `Expression`."
-  (s/named (s/cond-pre AnyValue FieldPlaceholderOrExpressionRef Expression)
-           "RValue"))
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; |                                                                             CLAUSES                                                                            |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
-;;; # ------------------------------------------------------------ CLAUSE SCHEMAS ------------------------------------------------------------
+;;; aggregation
 (s/defrecord AggregationWithoutField [aggregation-type :- (s/named (s/enum :count :cumulative-count)
                                                                   "Valid aggregation type")
@@ -281,9 +296,11 @@
   "standard-deviation-aggregations is not supported by this driver."))
+;;; filter
 (s/defrecord EqualityFilter [filter-type :- (s/enum := :!=)
                             field       :- FieldPlaceholderOrExpressionRef
-                             value       :- FieldOrAnyValue])
+                             value       :- AnyFieldOrValue])
 (s/defrecord ComparisonFilter [filter-type :- (s/enum :< :<= :> :>=)
                               field       :- FieldPlaceholderOrExpressionRef
@@ -316,28 +333,38 @@
  (s/named (s/cond-pre SimpleFilterClause NotFilter CompoundFilter)
           "Valid filter clause"))
+;;; order-by
 (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     AnyFieldOrExpression
+  (s/named {:field     AnyField
            :direction OrderByDirection}
           "Valid order-by subclause"))
+;;; page
 (def Page
  "Schema for the top-level `page` clause in a MBQL query."
  (s/named {:page  su/IntGreaterThanZero
            :items su/IntGreaterThanZero}
           "Valid page clause"))
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
+;;; |                                                                             QUERY                                                                              |
+;;; +----------------------------------------------------------------------------------------------------------------------------------------------------------------+
 (def Query
  "Schema for an MBQL query."
  {(s/optional-key :aggregation) [Aggregation]
   (s/optional-key :breakout)    [FieldPlaceholderOrExpressionRef]
-   (s/optional-key :fields)      [AnyFieldOrExpression]
+   (s/optional-key :fields)      [AnyField]
   (s/optional-key :filter)      Filter
   (s/optional-key :limit)       su/IntGreaterThanZero
   (s/optional-key :order-by)    [OrderBy]

--- a/src/metabase/query_processor/middleware/permissions.clj
+++ b/src/metabase/query_processor/middleware/permissions.clj
@@ -5,12 +5,14 @@
            [metabase.util :as u]))
 (defn- check-query-permissions* [query]
-  ;; TODO - should we do anything if there is no *current-user-id* (for something like a pulse?)
  (u/prog1 query
    (when *current-user-id*
      (perms/check-query-permissions *current-user-id* query))))
 (defn check-query-permissions
-  "Middleware that check that the current user has permissions to run the current query."
+  "Middleware that check that the current user has permissions to run the current query.
+   This only applies if `*current-user-id*` is bound. In other cases, like when running
+   public Cards or sending pulses, permissions need to be checked separately before allowing
+   the relevant objects to be create (e.g., when saving a new Pulse or 'publishing' a Card)."
  [qp]
  (comp qp check-query-permissions*))
--- a/src/metabase/query_processor/permissions.clj
+++ b/src/metabase/query_processor/permissions.clj
@@ -62,6 +62,7 @@
 (defn- ^:deprecated table-id [source-or-join-table]
+  {:post [(integer? %)]}
  (or (:id source-or-join-table)
      (:table-id source-or-join-table)))
@@ -75,6 +76,7 @@
  (or (user-can-run-query-referencing-table? user-id (table-id table))
      (throw-permissions-exception "You do not have permissions to run queries referencing table '%s'." (table-identifier table))))
+;; TODO - why is this the only function here that takes `user-id`?
 (defn- throw-if-cannot-run-query
  "Throw an exception if USER-ID doesn't have permissions to run QUERY."
  [user-id {:keys [source-table join-tables]}]
@@ -118,8 +120,8 @@
 (defn check-query-permissions
  "Check that User with USER-ID has permissions to run QUERY, or throw an exception."
-  [user-id {query-type :type, database :database, query :query, {card-id :card-id} :info}]
+  [user-id {query-type :type, database :database, query :query, {card-id :card-id} :info, :as outer-query}]
-  {:pre [(integer? user-id)]}
+  {:pre [(integer? user-id) (map? outer-query)]}
  (let [native?       (= (keyword query-type) :native)
        collection-id (db/select-one-field :collection_id 'Card :id card-id)]
    (cond

--- a/src/metabase/query_processor/resolve.clj
+++ b/src/metabase/query_processor/resolve.clj
@@ -194,7 +194,7 @@
        ;; Otherwise fetch + resolve the Fields in question
        (let [fields (->> (u/key-by :id (db/select [field/Field :name :display_name :base_type :special_type :visibility_type :table_id :parent_id :description :id]
                                          :visibility_type [:not= "sensitive"]
-                                          :id [:in field-ids]))
+                                          :id              [:in field-ids]))
                          (m/map-vals rename-mb-field-keys)
                          (m/map-vals #(assoc % :parent (when-let [parent-id (:parent-id %)]
                                                          (i/map->FieldPlaceholder {:field-id parent-id})))))]
@@ -207,7 +207,11 @@
           ;; Recurse in case any new (nested) unresolved fields were found.
           (recur (dec max-iterations))))))))
-(defn- fk-field-ids->info [source-table-id fk-field-ids]
+(defn- fk-field-ids->info
+  "Given a SOURCE-TABLE-ID and collection of FK-FIELD-IDS, return a sequence of maps containing IDs and identifiers for those FK fields and their target tables and fields.
+   FK-FIELD-IDS are IDs of fields that belong to the source table. For example, SOURCE-TABLE-ID might be 'checkins' and FK-FIELD-IDS might have the IDs for 'checkins.user_id'
+   and the like."
+  [source-table-id fk-field-ids]
  (when (seq fk-field-ids)
    (db/query {:select    [[:source-fk.name      :source-field-name]
                           [:source-fk.id        :source-field-id]
@@ -219,8 +223,8 @@
               :from      [[field/Field :source-fk]]
               :left-join [[field/Field :target-pk] [:= :source-fk.fk_target_field_id :target-pk.id]
                           [Table :target-table]    [:= :target-pk.table_id :target-table.id]]
-               :where     [:and [:in :source-fk.id      (set fk-field-ids)]
+               :where     [:and [:in :source-fk.id       (set fk-field-ids)]
-                                [:=  :source-fk.table_id     source-table-id]
+                                [:=  :source-fk.table_id source-table-id]
                                (mdb/isa :source-fk.special_type :type/FK)]})))
 (defn- fk-field-ids->joined-tables

--- a/src/metabase/query_processor/util.clj
+++ b/src/metabase/query_processor/util.clj
@@ -3,7 +3,11 @@
  (:require [buddy.core
             [codecs :as codecs]
             [hash :as hash]]
-            [cheshire.core :as json]))
+            [cheshire.core :as json]
+            [clojure.string :as str]
+            [metabase.util :as u]
+            [metabase.util.schema :as su]
+            [schema.core :as s]))
 (defn mbql-query?
  "Is the given query an MBQL query?"
@@ -33,6 +37,69 @@
                    (format ":: userID: %s queryType: %s queryHash: %s" executed-by query-type (codecs/bytes->hex query-hash)))))
+;;; ------------------------------------------------------------ Normalization ------------------------------------------------------------
+;; The following functions make it easier to deal with MBQL queries, which are case-insensitive, string/keyword insensitive, and underscore/hyphen insensitive.
+;; These should be preferred instead of assuming the frontend will always pass in clauses the same way, since different variation are all legal under MBQL '98.
+;; TODO - In the future it might make sense to simply walk the entire query and normalize the whole thing when it comes in. I've tried implementing middleware
+;; to do that but it ended up breaking a few things that wrongly assume different clauses will always use a certain case (e.g. SQL `:template_tags`). Fixing
+;; all of that is out-of-scope for the nested queries PR but should possibly be revisited in the future.
+(s/defn ^:always-validate normalize-token :- s/Keyword
+  "Convert a string or keyword in various cases (`lisp-case`, `snake_case`, or `SCREAMING_SNAKE_CASE`) to a lisp-cased keyword."
+  [token :- su/KeywordOrString]
+  (-> (name token)
+      str/lower-case
+      (str/replace #"_" "-")
+      keyword))
+(defn get-normalized
+  "Get the value for normalized key K in map M, regardless of how the key was specified in M,
+   whether string or keyword, lisp-case, snake_case, or SCREAMING_SNAKE_CASE.
+     (get-normalized {\"NUM_TOUCANS\" 2} :num-toucans) ; -> 2"
+  ([m k]
+   {:pre [(or (u/maybe? map? m)
+              (println "Not a map:" m))]}
+   (let [k (normalize-token k)]
+     (some (fn [[map-k v]]
+             (when (= k (normalize-token map-k))
+               v))
+           m)))
+  ([m k not-found]
+   (or (get-normalized m k)
+       not-found)))
+(defn get-in-normalized
+  "Like `get-normalized`, but accepts a sequence of keys KS, like `get-in`.
+    (get-in-normalized {\"NUM_BIRDS\" {\"TOUCANS\" 2}} [:num-birds :toucans]) ; -> 2"
+  ([m ks]
+   {:pre [(u/maybe? sequential? ks)]}
+   (loop [m m, [k & more] ks]
+     (if-not k
+       m
+       (recur (get-normalized m k) more))))
+  ([m ks not-found]
+   (or (get-in-normalized m ks)
+       not-found)))
+(defn dissoc-normalized
+  "Remove all matching keys from map M regardless of case, string/keyword, or hypens/underscores.
+     (dissoc-normalized {\"NUM_TOUCANS\" 3} :num-toucans) ; -> {}"
+  [m k]
+  {:pre [(or (u/maybe? map? m)
+             (println "Not a map:" m))]}
+  (let [k (normalize-token k)]
+    (loop [m m, [map-k & more, :as ks] (keys m)]
+      (cond
+        (not (seq ks)) m
+        (= k (normalize-token map-k)) (recur (dissoc m map-k) more)
+        :else                         (recur m                more)))))
 ;;; ------------------------------------------------------------ Hashing ------------------------------------------------------------
 (defn- select-keys-for-hashing

--- a/src/metabase/sync_database/analyze.clj
+++ b/src/metabase/sync_database/analyze.clj
@@ -44,7 +44,7 @@
  (try
    (queries/table-row-count table)
    (catch Throwable e
-      (log/error (u/format-color 'red "Unable to determine row count for '%s': %s\n%s" (:name table) (.getMessage e) (u/pprint-to-str (u/filtered-stacktrace e)))))))
+      (log/warn (u/format-color 'red "Unable to determine row count for '%s': %s\n%s" (:name table) (.getMessage e) (u/pprint-to-str (u/filtered-stacktrace e)))))))
 (defn test-for-cardinality?
  "Should FIELD should be tested for cardinality?"

--- a/src/metabase/util/honeysql_extensions.clj
+++ b/src/metabase/util/honeysql_extensions.clj
@@ -84,6 +84,7 @@
                 (rest sql-string-or-vector))))))
+;; Single-quoted string literal
 (defrecord Literal [literal]
  ToSql
  (to-sql [_]

--- a/test/metabase/http_client.clj
+++ b/test/metabase/http_client.clj
@@ -67,7 +67,6 @@
   or throw an Exception if that fails."
  [{:keys [email password], :as credentials}]
  {:pre [(string? email) (string? password)]}
-  (println "Authenticating" email) ; DEBUG
  (try
    (:id (client :post 200 "session" credentials))
    (catch Throwable e

--- a/test/metabase/models/card_test.clj
+++ b/test/metabase/models/card_test.clj
@@ -9,8 +9,8 @@
             [permissions :as perms]]
            [metabase.query-processor.expand :as ql]
            [metabase.test
-             [data :refer [id]]
+             [data :as data]
-             [util :as tu :refer [random-name]]]
+             [util :as tu]]
            [metabase.test.data.users :refer :all]
            [metabase.util :as u]
            [toucan.db :as db]
@@ -26,9 +26,9 @@
    (let [get-dashboard-count (fn [] (dashboard-count (Card card-id)))]
      [(get-dashboard-count)
-       (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (random-name))), :parameter_mappings [])
+       (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (tu/random-name))), :parameter_mappings [])
           (get-dashboard-count))
-       (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (random-name))), :parameter_mappings [])
+       (do (db/insert! DashboardCard :card_id card-id, :dashboard_id (:id (create-dash! (tu/random-name))), :parameter_mappings [])
           (get-dashboard-count))])))
@@ -60,25 +60,25 @@
 (expect
  false
-  (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
+  (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
    (binding [*current-user-permissions-set* (delay #{})]
      (mi/can-read? card))))
 (expect
-  (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
+  (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
-    (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (id))})]
+    (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (data/id))})]
      (mi/can-read? card))))
 ;; in order to *write* a native card user should need native readwrite access
 (expect
  false
-  (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
+  (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
-    (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (id))})]
+    (binding [*current-user-permissions-set* (delay #{(perms/native-read-path (data/id))})]
      (mi/can-write? card))))
 (expect
-  (tt/with-temp Card [card {:dataset_query {:database (id), :type "native"}}]
+  (tt/with-temp Card [card {:dataset_query {:database (data/id), :type "native"}}]
-    (binding [*current-user-permissions-set* (delay #{(perms/native-readwrite-path (id))})]
+    (binding [*current-user-permissions-set* (delay #{(perms/native-readwrite-path (data/id))})]
      (mi/can-write? card))))
@@ -100,24 +100,24 @@
 (defn- mbql [query]
-  {:database (id)
+  {:database (data/id)
   :type     :query
   :query    query})
 ;; MBQL w/o JOIN
 (expect
-  #{(perms/object-path (id) "PUBLIC" (id :venues))}
+  #{(perms/object-path (data/id) "PUBLIC" (data/id :venues))}
  (query-perms-set (mbql (ql/query
-                           (ql/source-table (id :venues))))
+                           (ql/source-table (data/id :venues))))
                   :read))
 ;; MBQL w/ JOIN
 (expect
-  #{(perms/object-path (id) "PUBLIC" (id :checkins))
+  #{(perms/object-path (data/id) "PUBLIC" (data/id :checkins))
-    (perms/object-path (id) "PUBLIC" (id :venues))}
+    (perms/object-path (data/id) "PUBLIC" (data/id :venues))}
  (query-perms-set (mbql (ql/query
-                           (ql/source-table (id :checkins))
+                           (ql/source-table (data/id :checkins))
-                           (ql/order-by (ql/asc (ql/fk-> (id :checkins :venue_id) (id :venues :name))))))
+                           (ql/order-by (ql/asc (ql/fk-> (data/id :checkins :venue_id) (data/id :venues :name))))))
                   :read))
 ;; invalid/legacy card should return perms for something that doesn't exist so no one gets to see it