Merge branch 'master' into move-add-implicit-clauses-to-before-expansion

src/metabase/api/card.clj

@@ -564,7 +564,7 @@
         ttl   (when (public-settings/enable-query-caching)
                 (or (:cache_ttl card)
                     (query-magic-ttl query)))]
-    (assoc query :cache_ttl ttl)))
+    (assoc query :cache-ttl ttl)))
 
 (defn run-query-for-card
   "Run the query for Card with PARAMETERS and CONSTRAINTS, and return results in the usual format."

src/metabase/automagic_dashboards/core.clj

@@ -35,7 +35,7 @@
              [query :refer [Query]]
              [segment :refer [Segment]]
              [table :refer [Table]]]
-            [metabase.query-processor.middleware.expand-macros :as qp.expand]
+            [metabase.query-processor.middleware.expand-macros :as qp.macros]
             [metabase.query-processor.util :as qp.util]
             [metabase.sync.analyze.classify :as classify]
             [metabase.util.date :as date]
@@ -82,7 +82,7 @@
 
 (def ^:private ^{:arglists '([metric])} saved-metric?
   (every-pred (partial mbql.u/is-clause? :metric)
-              (complement qp.expand/ga-metric?)))
+              (complement qp.macros/ga-metric-or-segment?)))
 
 (def ^:private ^{:arglists '([metric])} custom-expression?
   (partial mbql.u/is-clause? :named))
@@ -94,10 +94,10 @@
   "Return the name of the metric or name by describing it."
   [[op & args :as metric]]
   (cond
-    (qp.expand/ga-metric? metric) (-> args first str (subs 3) str/capitalize)
-    (adhoc-metric? metric)        (-> op qp.util/normalize-token op->name)
-    (saved-metric? metric)        (-> args first Metric :name)
-    :else                         (second args)))
+    (qp.macros/ga-metric-or-segment? metric) (-> args first str (subs 3) str/capitalize)
+    (adhoc-metric? metric)                   (-> op qp.util/normalize-token op->name)
+    (saved-metric? metric)                   (-> args first Metric :name)
+    :else                                    (second args)))
 
 (defn metric-op
   "Return the name op of the metric"

src/metabase/mbql/normalize.clj

@@ -297,9 +297,8 @@
         (cond-> arg
           (mbql-clause? arg) canonicalize-aggregation-subclause))))
 
-    ;; for metric and segment macros (e.g. [:metric <metric-id>]) do not wrap the metric/segment ID in a :field-id
-    ;; clause
-    (#{:metric :segment} ag-type)
+    ;; for metric macros (e.g. [:metric <metric-id>]) do not wrap the metric in a :field-id clause
+    (= :metric ag-type)
     ag-subclause
 
     ;; something with an arg like [:sum [:field-id 41]]

src/metabase/mbql/schema.clj

@@ -297,7 +297,10 @@
 
 ;; A segment is a special `macro` that saves some pre-definied filter clause, e.g. [:segment 1]
 ;; this gets replaced by a normal Filter clause in MBQL macroexpansion
-(defclause segment, segment-id su/IntGreaterThanZero)
+;;
+;; It can also be used for GA, which looks something like `[:segment "gaid::-11"]`. GA segments aren't actually MBQL
+;; segments and pass-thru to GA.
+(defclause segment, segment-id (s/cond-pre su/IntGreaterThanZero su/NonBlankString))
 
 (def Filter
   "Schema for a valid MBQL `:filter` clause."
@@ -320,7 +323,9 @@
 (def ^:private NativeQuery
   "Schema for a valid, normalized native [inner] query."
   {:query                          s/Any
-   (s/optional-key :template-tags) {su/NonBlankString TemplateTag}})
+   (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)})
 
 
 ;;; ----------------------------------------------- MBQL [Inner] Query -----------------------------------------------
@@ -464,6 +469,10 @@
     (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
     ;;

src/metabase/query_processor/middleware/cache.clj

@@ -85,7 +85,7 @@
 
 ;;; --------------------------------------------------- Middleware ---------------------------------------------------
 
-(defn- is-cacheable? ^Boolean [{cache-ttl :cache_ttl}]
+(defn- is-cacheable? ^Boolean [{:keys [cache-ttl]}]
   (boolean (and (public-settings/enable-query-caching)
                 cache-ttl)))
 
@@ -121,7 +121,7 @@
       (save-results-if-successful! query-hash results))
     results))
 
-(defn- run-query-with-cache [qp {cache-ttl :cache_ttl, :as query}]
+(defn- run-query-with-cache [qp {:keys [cache-ttl], :as query}]
   ;; TODO - Query should already have a `info.hash`, shouldn't it?
   (let [query-hash (qputil/query-hash query)]
     (or (cached-results query-hash cache-ttl)
@@ -133,7 +133,7 @@
   In order for a query to be eligible for caching:
 
      *  Caching (the `enable-query-caching` Setting) must be enabled
-     *  The query must pass a `:cache_ttl` value. For Cards, this can be the value of `:cache_ttl`,
+     *  The query must pass a `:cache-ttl` value. For Cards, this can be the value of `:cache_ttl`,
         otherwise falling back to the value of the `query-caching-default-ttl` Setting.
      *  The query must already be permissions-checked. Since the cache bypasses the normal
         query processor pipeline, the ad-hoc permissions-checking middleware isn't applied for cached results.

src/metabase/query_processor/middleware/expand_macros.clj

 (ns metabase.query-processor.middleware.expand-macros
-  "Middleware for expanding `METRIC` and `SEGMENT` 'macros' in *unexpanded* MBQL queries.
+  "Middleware for expanding `:metric` and `:segment` 'macros' in *unexpanded* MBQL queries.
 
-   Code in charge of expanding [\"METRIC\" ...] and [\"SEGMENT\" ...] forms in MBQL queries.
-   (METRIC forms are expanded into aggregations and sometimes filter clauses, while SEGMENT forms
-    are expanded into filter clauses.)
+  (`:metric` forms are expanded into aggregations and sometimes filter clauses, while `:segment` forms are expanded
+  into filter clauses.)
 
    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."
@@ -20,18 +19,31 @@
             [schema.core :as s]
             [toucan.db :as db]))
 
+(defn ga-metric-or-segment?
+  "Is this metric or segment clause not a Metabase Metric or Segment, but rather a GA one? E.g. something like `[:metric
+  ga:users]`. We want to ignore those because they're not the same thing at all as MB Metrics/Segments and don't
+  correspond to objects in our application DB."
+  [[_ id]]
+  (boolean
+   (when ((some-fn string? keyword?) id)
+     (re-find #"^ga(id)?:" (name id)))))
+
+
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                    SEGMENTS                                                    |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defn- segment-clauses->id->definition [segment-clauses]
-  (db/select-id->field :definition Segment, :id [:in (set (map second segment-clauses))]))
+  (when-let [segment-ids (seq (filter integer? (map second segment-clauses)))]
+    (db/select-id->field :definition Segment, :id [:in (set segment-ids)])))
 
 (defn- replace-segment-clauses [outer-query segment-id->definition]
   (mbql.u/replace-clauses-in outer-query [:query] :segment
-    (fn [[_ segment-id]]
-      (or (:filter (segment-id->definition segment-id))
-          (throw (IllegalArgumentException. (str (tru "Segment {0} does not exist, or is invalid." segment-id))))))))
+    (fn [[_ segment-id, :as segment]]
+      (if (ga-metric-or-segment? segment)
+        segment
+        (or (:filter (segment-id->definition segment-id))
+            (throw (IllegalArgumentException. (str (tru "Segment {0} does not exist, or is invalid." segment-id)))))))))
 
 (s/defn ^:private expand-segments :- mbql.s/Query
   [{inner-query :query, :as outer-query} :- mbql.s/Query]
@@ -44,19 +56,10 @@
 ;;; |                                                    METRICS                                                     |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn ga-metric?
-  "Is this metric clause not a Metabase Metric, but rather a GA one? E.g. something like [metric ga:users]. We want to
-   ignore those because they're not the same thing at all as MB Metrics and don't correspond to objects in our
-   application DB."
-  [[_ id]]
-  (boolean
-   (when ((some-fn string? keyword?) id)
-     (re-find #"^ga(id)?:" (name id)))))
-
 (defn- metrics
   "Return a sequence of any (non-GA) `:metric` MBQL clauses in `query`."
   [{inner-query :query}] ; metrics won't be in a native query but they could be in source-query or aggregation clause
-  (seq (filter (complement ga-metric?) (mbql.u/clause-instances :metric inner-query))))
+  (seq (filter (complement ga-metric-or-segment?) (mbql.u/clause-instances :metric inner-query))))
 
 (defn- metric-clauses->id->definition [metric-clauses]
   (db/select-id->field :definition Metric, :id [:in (set (map second metric-clauses))]))
@@ -68,7 +71,7 @@
 (defn- replace-metrics-aggregations [query metric-id->definition]
   (mbql.u/replace-clauses-in query [:query] :metric
     (fn [[_ metric-id, :as metric]]
-      (if (ga-metric? metric)
+      (if (ga-metric-or-segment? metric)
         metric
         (or (first (:aggregation (metric-id->definition metric-id)))
             (throw (IllegalArgumentException.

test/metabase/mbql/normalize_test.clj

@@ -519,6 +519,21 @@
     :type     :query
     :query    {:filter []}}))
 
+;; make sure we can handle GA segments
+(expect
+  {:database 1,
+   :type     :query
+   :query    {:filter
+              [:and
+               [:segment "gaid:-11"]
+               [:time-interval [:field-id 6851] -365 :day {}]]}}
+  (#'normalize/canonicalize
+   {:database 1
+    :type     :query
+    :query    {:filter [:and
+                        [:segment "gaid:-11"]
+                        [:time-interval [:field-id 6851] -365 :day {}]]}}))
+
 ;; ORDER BY: MBQL 95 [field direction] should get translated to MBQL 98 [direction field]
 (expect
   {:query {:order-by [[:asc [:field-id 10]]]}}
@@ -711,6 +726,7 @@
                                                            :required     true
                                                            :default      "Widget"}}}}}))
 
+;; make sure `rows` queries get removed
 (expect
   {:database 4,
    :type     :query,

test/metabase/query_processor/middleware/cache_test.clj

@@ -34,7 +34,7 @@
     :not-cached))
 
 (defn- run-query [& {:as query-kvs}]
-  (cached? (maybe-return-cached-results (merge {:cache_ttl 60, :query :abc} query-kvs))))
+  (cached? (maybe-return-cached-results (merge {:cache-ttl 60, :query :abc} query-kvs))))
 
 
 ;;; -------------------------------------------- tests for is-cacheable? ---------------------------------------------
@@ -42,17 +42,17 @@
 ;; something is-cacheable? if it includes a cach_ttl and the caching setting is enabled
 (expect
   (tu/with-temporary-setting-values [enable-query-caching true]
-    (#'cache/is-cacheable? {:cache_ttl 100})))
+    (#'cache/is-cacheable? {:cache-ttl 100})))
 
 (expect
   false
   (tu/with-temporary-setting-values [enable-query-caching false]
-    (#'cache/is-cacheable? {:cache_ttl 100})))
+    (#'cache/is-cacheable? {:cache-ttl 100})))
 
 (expect
   false
   (tu/with-temporary-setting-values [enable-query-caching true]
-    (#'cache/is-cacheable? {:cache_ttl nil})))
+    (#'cache/is-cacheable? {:cache-ttl nil})))
 
 
 ;;; ------------------------------------- results-are-below-max-byte-threshold? --------------------------------------
@@ -104,9 +104,9 @@
   (tu/with-temporary-setting-values [enable-query-caching  true
                                      query-caching-min-ttl 0]
     (clear-cache!)
-    (run-query :cache_ttl 1)
+    (run-query :cache-ttl 1)
     (Thread/sleep 2000)
-    (run-query :cache_ttl 1)))
+    (run-query :cache-ttl 1)))
 
 ;; if caching is disabled then cache shouldn't be used even if there's something valid in there
 (expect

test/metabase/query_processor/middleware/expand_macros_test.clj

@@ -181,23 +181,36 @@
                           :aggregation  [[:metric (u/get-id metric)]]
                           :breakout     [[:field-id (data/id :venues :price)]]}})))))
 
+(defn- mbql-query [inner-query]
+  {:database 1, :type :query, :query inner-query})
+
 ;; make sure that we don't try to expand GA "metrics" (#6104)
 (expect
-  {:query {:aggregation [[:metric :ga:users]]}}
-  (#'expand-macros/expand-metrics-and-segments {:query {:aggregation [[:metric :ga:users]]}}))
+  (mbql-query {:aggregation [[:metric "ga:users"]]})
+  (#'expand-macros/expand-metrics-and-segments (mbql-query {:aggregation [[:metric "ga:users"]]})))
 
 (expect
-  {:query {:aggregation [[:metric :gaid:users]]}}
-  (#'expand-macros/expand-metrics-and-segments {:query {:aggregation [[:metric :gaid:users]]}}))
+  (mbql-query {:aggregation [[:metric "gaid:users"]]})
+  (#'expand-macros/expand-metrics-and-segments (mbql-query {:aggregation [[:metric "gaid:users"]]})))
 
 ;; make sure expansion works with multiple GA "metrics" (#7399)
 (expect
-  {:query {:aggregation [[:metric :ga:users] [:metric :ga:1dayUsers]]}}
-  (#'expand-macros/expand-metrics-and-segments {:query {:aggregation [[:metric :ga:users] [:metric :ga:1dayUsers]]}}))
+  (mbql-query {:aggregation [[:metric "ga:users"]
+                             [:metric "ga:1dayUsers"]]})
+  (#'expand-macros/expand-metrics-and-segments (mbql-query {:aggregation [[:metric "ga:users"]
+                                                                          [:metric "ga:1dayUsers"]]})))
+
+;; make sure we don't try to expand GA "segments"
+(expect
+  (mbql-query {:filter [:segment "gaid:-11"]})
+  (#'expand-macros/expand-metrics-and-segments (mbql-query {:filter [:segment "gaid:-11"]})))
 
 ;; make sure we can name a :metric (ick)
 (expect
+  (mbql-query
+   {:aggregation [[:named [:sum [:field-id 20]] "My Cool Metric"]]
+    :breakout    [[:field-id 10]]})
   (tt/with-temp Metric [metric {:definition {:aggregation [[:sum [:field-id 20]]]}}]
     (#'expand-macros/expand-metrics-and-segments
-     {:query {:aggregation [[:named [:metric (u/get-id metric)] "My Cool Metric"]]
-              :breakout    [[:field-id 10]]}})))
+     (mbql-query {:aggregation [[:named [:metric (u/get-id metric)] "My Cool Metric"]]
+                  :breakout    [[:field-id 10]]}))))