diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index fc9cf849ab7faf1a2edd506f3eb2b8fd40253e9d..50b64842fd682a1f979ace15cf879365581101f8 100644
--- a/src/metabase/api/card.clj
+++ b/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."
diff --git a/src/metabase/automagic_dashboards/core.clj b/src/metabase/automagic_dashboards/core.clj
index 4ad7cc6d871e9571b5304fd531549e356e2a9e8d..d0173fcc9ed3a79c96fcb79992fc8ab86595c50d 100644
--- a/src/metabase/automagic_dashboards/core.clj
+++ b/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"
diff --git a/src/metabase/mbql/normalize.clj b/src/metabase/mbql/normalize.clj
index 313f6cba0c4f96c7212cda7ba9a8e87e3148945d..2360ada24d7bf024cff2182a395fa5b72731b334 100644
--- a/src/metabase/mbql/normalize.clj
+++ b/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]]
diff --git a/src/metabase/mbql/schema.clj b/src/metabase/mbql/schema.clj
index 0394770a66eef31bcc667402c93e06b8fd9695c1..d249df9fd4e7a984edeeffc41645ff4de1f7df7e 100644
--- a/src/metabase/mbql/schema.clj
+++ b/src/metabase/mbql/schema.clj
@@ -296,7 +296,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."
@@ -319,7 +322,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 -----------------------------------------------
@@ -453,6 +458,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 --------------------
;;
diff --git a/src/metabase/query_processor/middleware/cache.clj b/src/metabase/query_processor/middleware/cache.clj
index 58b088a3414e9e15de0a3f666f58fb683650f9fe..471040a384a71c5d646d7c8c27ddfd0c85ef310b 100644
--- a/src/metabase/query_processor/middleware/cache.clj
+++ b/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.
diff --git a/src/metabase/query_processor/middleware/expand_macros.clj b/src/metabase/query_processor/middleware/expand_macros.clj
index 524d0b550e59445940ccb7c83d69e471dfd41edd..acdb06333142fe18363ebb0d4c9fa503fc76bf3c 100644
--- a/src/metabase/query_processor/middleware/expand_macros.clj
+++ b/src/metabase/query_processor/middleware/expand_macros.clj
@@ -1,9 +1,8 @@
(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."
@@ -19,18 +18,31 @@
[puppetlabs.i18n.core :refer [tru]]
[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)))))))))
(defn- expand-segments [{inner-query :query, :as outer-query}]
(if-let [segments (mbql.u/clause-instances :segment inner-query)]
@@ -42,19 +54,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))]))
@@ -66,7 +69,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.
diff --git a/test/metabase/mbql/normalize_test.clj b/test/metabase/mbql/normalize_test.clj
index b8bd055d1175a96a17d19b1b4d9eef558b8f602a..215f708d608012e31290aa3d1a6a4ae0a3757a03 100644
--- a/test/metabase/mbql/normalize_test.clj
+++ b/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,
diff --git a/test/metabase/query_processor/middleware/cache_test.clj b/test/metabase/query_processor/middleware/cache_test.clj
index 5b51cf02b06a36c4196ec3c60dbd06d2aca87db5..905f92d8f293c7cd84c1a186579f51d3729dbf31 100644
--- a/test/metabase/query_processor/middleware/cache_test.clj
+++ b/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
diff --git a/test/metabase/query_processor/middleware/expand_macros_test.clj b/test/metabase/query_processor/middleware/expand_macros_test.clj
index d5f295bcaedb1e324e708dce6eac743cb94a72a7..05c1199f9a5db7ba6bcf98df9134d62cdcd05cc9 100644
--- a/test/metabase/query_processor/middleware/expand_macros_test.clj
+++ b/test/metabase/query_processor/middleware/expand_macros_test.clj
@@ -177,23 +177,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]]}))))