Skip to content

Commit

Permalink
Merge pull request #3629 from metabase/merge-0.20.1
Browse files Browse the repository at this point in the history 
Merge 0.20.1 to master
  • Loading branch information
@tlrobinson
tlrobinson committed Oct 19, 2016
2 parents a7bf0b4 + 85fb197 commit a9ec0eb
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 34 deletions.
57 changes: 43 additions & 14 deletions src/metabase/query_processor/sql_parameters.clj
View file
Expand Up @@ -20,15 +20,19 @@
(defprotocol ^:private ISQLParamSubstituion
(^:private ->sql ^String [this]))

(defrecord ^:private Dimension [^FieldInstance field, param])
(defrecord ^:private Dimension [^FieldInstance field, param]) ;; param is either a single param or a vector of params

(defrecord ^:private DateRange [start end])

(defrecord ^:private NumberValue [value])

(defn- dimension-value->sql [dimension-type value]
(defn- dimension-value->sql
"Return an appropriate operator and rhs of a SQL `WHERE` clause, e.g. \"= 100\"."
^String [dimension-type value]
(if (contains? #{"date/range" "date/month-year" "date/quarter-year" "date/relative"} dimension-type)
;; for relative dates convert the param to a `DateRange` record type and call `->sql` on it
(->sql (map->DateRange ((resolve 'metabase.query-processor.parameters/date->range) value *timezone*))) ; TODO - get timezone from query dict
;; for everything else just call `->sql` on it directly
(str "= " (->sql value))))

(defn- honeysql->sql ^String [x]
Expand All @@ -45,7 +49,7 @@
(extend-protocol ISQLParamSubstituion
nil (->sql [_] "NULL")
Object (->sql [this] (str this))
Boolean (->sql [this] (if this "TRUE" "FALSE"))
Boolean (->sql [this] (honeysql->sql this))
NumberValue (->sql [this] (:value this))
String (->sql [this] (str \' (s/replace this #"'" "\\\\'") \')) ; quote single quotes inside the string
Keyword (->sql [this] (honeysql->sql this))
Expand Down Expand Up @@ -74,10 +78,16 @@
(format "BETWEEN '%s' AND '%s'" start end))))

Dimension
(->sql [{:keys [field param]}]
(if-not param
(->sql [{:keys [field param], :as dimension}]
(cond
;; if the param is `nil` just put in something that will always be true, such as `1` (e.g. `WHERE 1 = 1`)
"1 = 1"
(nil? param) "1 = 1"
;; if we have a vector of multiple params recursively convert them to SQL and combine into an `AND` clause
(vector? param)
(format "(%s)" (s/join " AND " (for [p param]
(->sql (assoc dimension :param p)))))
;; otherwise convert single param to SQL
:else
(let [param-type (:type param)]
(format "%s %s" (->sql (assoc field :type param-type)) (dimension-value->sql param-type (:value param)))))))

Expand All @@ -89,12 +99,17 @@
v (->sql (k params))]
(s/replace-first s match v)))

(defn- handle-simple [s params] (loop [s s, [[match param] & more] (re-seq #"\{\{\s*(\w+)\s*\}\}" s)]
(defn- handle-simple
"Replace a 'simple' SQL parameter (curly brackets only, i.e. `{{...}}`)."
[s params]
(loop [s s, [[match param] & more] (re-seq #"\{\{\s*(\w+)\s*\}\}" s)]
(if-not match
s
(recur (replace-param s params match param) more))))

(defn- handle-optional [s params]
(defn- handle-optional
"Replace an 'optional' parameter."
[s params]
(try (handle-simple s params)
(catch Throwable _
"")))
Expand All @@ -117,11 +132,16 @@

;;; ------------------------------------------------------------ Param Resolution ------------------------------------------------------------

(defn- param-with-target [params target]
(some (fn [param]
(when (= (:target param) target)
param))
params))
(defn- param-with-target
"Return the param in PARAMS with a matching TARGET."
[params target]
(when-let [matching-params (seq (for [param params
:when (= (:target param) target)]
param))]
;; if there's only one matching param no need to nest it inside a vector. Otherwise return vector of params
((if (= (count matching-params) 1)
first
vec) matching-params)))

(defn- param-value-for-tag [tag params]
(when (not= (:type tag) "dimension")
Expand Down Expand Up @@ -158,7 +178,16 @@
(dimension-value-for-tag tag params)
(default-value-for-tag tag))))

(defn- query->params-map [{{tags :template_tags} :native, params :parameters}]
(defn- query->params-map
"Extract parameters info from QUERY. Return a map of parameter name -> value.
(query->params-map some-query)
->
{:checkin_date {:field {:name \"date\", :parent_id nil, :table_id 1375}
:param {:type \"date/range\"
:target [\"dimension\" [\"template-tag\" \"checkin_date\"]]
:value \"2015-01-01~2016-09-01\"}}}"
[{{tags :template_tags} :native, params :parameters}]
(into {} (for [[k tag] tags
:let [v (value-for-tag tag params)]
:when v]
Expand Down
68 changes: 48 additions & 20 deletions test/metabase/query_processor/sql_parameters_test.clj
View file
Expand Up @@ -17,7 +17,9 @@

;;; ------------------------------------------------------------ simple substitution -- {{x}} ------------------------------------------------------------

(tu/resolve-private-vars metabase.query-processor.sql-parameters substitute)
(defn- substitute {:style/indent 1} [sql params]
(binding [metabase.query-processor.sql-parameters/*driver* (driver/engine->driver :h2)] ; apparently you can still bind private dynamic vars
((resolve 'metabase.query-processor.sql-parameters/substitute) sql params)))

(expect "SELECT * FROM bird_facts WHERE toucans_are_cool = TRUE"
(substitute "SELECT * FROM bird_facts WHERE toucans_are_cool = {{toucans_are_cool}}"
Expand Down Expand Up @@ -180,7 +182,23 @@
:parent_id nil
:table_id (data/id :checkins)}
:param nil}
(into {} (value-for-tag {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]} nil)))
(into {} (value-for-tag {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}
nil)))

;; multiple values for the same tag should return a vector with multiple params instead of a single param
(expect
{:field {:name "DATE"
:parent_id nil
:table_id (data/id :checkins)}
:param [{:type "date/range"
:target ["dimension" ["template-tag" "checkin_date"]]
:value "2015-01-01~2016-09-01"}
{:type "date/single"
:target ["dimension" ["template-tag" "checkin_date"]]
:value "2015-07-01"}]}
(into {} (value-for-tag {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}
[{:type "date/range", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-01-01~2016-09-01"}
{:type "date/single", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-07-01"}])))


;;; ------------------------------------------------------------ expansion tests: variables ------------------------------------------------------------
Expand Down Expand Up @@ -342,38 +360,48 @@
(def ^:private ^:const sql-parameters-engines
(set/difference (engines-that-support :native-parameters) #{:redshift :crate}))

(defn- process-native {:style/indent 0} [& kvs]
(qp/process-query
(apply assoc {:database (data/id), :type :native} kvs)))

(datasets/expect-with-engines sql-parameters-engines
[29]
(first-row
(format-rows-by [int]
(qp/process-query
{:database (data/id)
:type :native
:native {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
:template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
:parameters [{:type "date/range", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-04-01~2015-05-01"}]}))))
(process-native
:native {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
:template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
:parameters [{:type "date/range", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-04-01~2015-05-01"}]))))

;; no parameter -- should give us a query with "WHERE 1 = 1"
(datasets/expect-with-engines sql-parameters-engines
[1000]
(first-row
(format-rows-by [int]
(qp/process-query
{:database (data/id)
:type :native
:native {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
:template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
:parameters []}))))
(process-native
:native {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
:template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
:parameters []))))

;; test that relative dates work correctly. It should be enough to try just one type of relative date here,
;; since handling them gets delegated to the functions in `metabase.query-processor.parameters`, which is fully-tested :D
(datasets/expect-with-engines sql-parameters-engines
[0]
(first-row
(format-rows-by [int]
(qp/process-query
{:database (data/id)
:type :native
:native {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
:template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
:parameters [{:type "date/relative", :target ["dimension" ["template-tag" "checkin_date"]], :value "thismonth"}]}))))
(process-native
:native {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
:template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
:parameters [{:type "date/relative", :target ["dimension" ["template-tag" "checkin_date"]], :value "thismonth"}]))))


;; test that multiple filters applied to the same variable combine into `AND` clauses (#3539)
(datasets/expect-with-engines sql-parameters-engines
[4]
(first-row
(format-rows-by [int]
(process-native
:native {:query (format "SELECT COUNT(*) FROM %s WHERE {{checkin_date}}" (checkins-identifier))
:template_tags {:checkin_date {:name "checkin_date", :display_name "Checkin Date", :type "dimension", :dimension ["field-id" (data/id :checkins :date)]}}}
:parameters [{:type "date/range", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-01-01~2016-09-01"}
{:type "date/single", :target ["dimension" ["template-tag" "checkin_date"]], :value "2015-07-01"}]))))

0 comments on commit a9ec0eb

Please sign in to comment.