Skip to content
Snippets Groups Projects
Commit 4699a5cd authored by Cam Saül's avatar Cam Saül
Browse files

Fix SQL for :limit clause for old versions of Oracle :wrench: [ci drivers]

parent 3761a9df
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/db.clj
+ 1
− 1
View file @ 4699a5cd
......@@ -478,7 +478,7 @@
(defn- format-sql [sql]
(when sql
(loop [sql sql, [k & more] ["FROM" "WHERE" "LEFT JOIN" "INNER JOIN" "ORDER BY" "LIMIT"]]
(loop [sql sql, [k & more] ["FROM" "LEFT JOIN" "INNER JOIN" "WHERE" "GROUP BY" "HAVING" "ORDER BY" "OFFSET" "LIMIT"]]
(if-not k
sql
(recur (s/replace sql (re-pattern (format "\\s+%s\\s+" k)) (format "\n%s " k))
......
src/metabase/driver/generic_sql.clj
+ 4
− 3
View file @ 4699a5cd
......@@ -218,11 +218,12 @@
transform-fn (if (isa? (:base_type field) :type/Text)
u/jdbc-clob->str
identity)
select* {:select [[field-k :field]] ; if we don't specify an explicit ORDER BY some DBs like Redshift will return them in a (seemingly) random order
select* {:select [[field-k :field]]
:from [(qualify+escape table)] ; if we don't specify an explicit ORDER BY some DBs like Redshift will return them in a (seemingly) random order
:order-by [[(or pk-field-k field-k) :asc]]} ; try to order by the table's Primary Key to avoid doing full table scans
fetch-one-page (fn [page-num]
(for [{v :field} (query driver db table (apply-page driver select* {:page {:items driver/field-values-lazy-seq-chunk-size
:page (inc page-num)}}))]
(for [{v :field} (query driver db (apply-page driver select* {:page {:items driver/field-values-lazy-seq-chunk-size
:page (inc page-num)}}))]
(transform-fn v)))
;; This function returns a chunked lazy seq that will fetch some range of results, e.g. 0 - 500, then concat that chunk of results
......
src/metabase/driver/generic_sql/query_processor.clj
+ 11
− 22
View file @ 4699a5cd
......@@ -221,42 +221,31 @@
(h/limit items)
(h/offset (* items (dec page)))))
;; TODO - not sure "pprint" is an appropriate name for this since this function doesn't print anything
(defn pprint-sql
"Add newlines to the SQL to make it more readable."
[sql]
(when sql
(-> sql
(s/replace #"\sFROM" "\nFROM")
(s/replace #"\sLEFT JOIN" "\nLEFT JOIN")
(s/replace #"\sWHERE" "\nWHERE")
(s/replace #"\sGROUP BY" "\nGROUP BY")
(s/replace #"\sORDER BY" "\nORDER BY")
(s/replace #"\sLIMIT" "\nLIMIT")
(s/replace #"\sAND\s" "\n AND ")
(s/replace #"\sOR\s" "\n OR "))))
;; TODO - make this a protocol method ?
;; TODO - is there any reason to make this a protocol method ?
(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)))
(def ^:private clause-handlers
{:aggregation #'sql/apply-aggregation ; use the vars rather than the functions themselves because them implementation
:breakout #'sql/apply-breakout ; will get swapped around and we'll be left with old version of the function that nobody implements
;; 1) Use the vars rather than the functions themselves because them implementation
;; 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
:aggregation #'sql/apply-aggregation
:breakout #'sql/apply-breakout
:fields #'sql/apply-fields
:filter #'sql/apply-filter
:join-tables #'sql/apply-join-tables
:limit #'sql/apply-limit
:order-by #'sql/apply-order-by
:page #'sql/apply-page
:source-table apply-source-table})
:limit #'sql/apply-limit])
(defn- apply-clauses
"Loop through all the `clause->handler` entries; if the query contains a given clause, apply the handler fn."
[driver honeysql-form query]
(loop [honeysql-form honeysql-form, [[clause f] & more] (seq clause-handlers)]
(loop [honeysql-form honeysql-form, [clause f & more] (seq clause-handlers)]
(let [honeysql-form (if (clause query)
(f driver honeysql-form query)
honeysql-form)]
......
src/metabase/driver/oracle.clj
+ 58
− 18
View file @ 4699a5cd
......@@ -3,12 +3,14 @@
(clojure [set :as set]
[string :as s])
[clojure.tools.logging :as log]
[honeysql.core :as hsql]
(honeysql [core :as hsql]
[helpers :as h])
[metabase.config :as config]
[metabase.db :as db]
[metabase.db.spec :as dbspec]
[metabase.driver :as driver]
[metabase.driver.generic-sql :as sql]
[metabase.driver.generic-sql.query-processor :as sqlqp]
[metabase.util :as u]
[metabase.util.honeysql-extensions :as hx]))
......@@ -110,26 +112,52 @@
:seconds field-or-value
:milliseconds (hx// field-or-value (hsql/raw 1000))))))
(defn- apply-offset-and-limit
"Append SQL like `OFFSET 20 ROWS FETCH NEXT 10 ROWS ONLY` to the end of the query."
[honeysql-query offset limit]
(assoc honeysql-query
:offset (hsql/raw (format "%d ROWS FETCH NEXT %d ROWS ONLY" offset limit))))
;; Oracle doesn't support `LIMIT n` syntax. Instead we have to use `WHERE ROWNUM <= n` (`NEXT n ROWS ONLY` isn't supported on Oracle versions older than 12).
;; This has to wrap the actual query, e.g.
;;
;; SELECT *
;; FROM (
;; SELECT *
;; FROM employees
;; ORDER BY employee_id
;; )
;; WHERE ROWNUM < 10;
;;
;; To do an offset we have to do something like:
;;
;; SELECT *
;; FROM (
;; SELECT __table__.*, ROWNUM AS __rownum__
;; FROM (
;; SELECT *
;; FROM employees
;; ORDER BY employee_id
;; ) __table__
;; WHERE ROWNUM <= 150
;; )
;; WHERE __rownum__ >= 100;
;;
;; See issue #3568 and the Oracle documentation for more details: http://docs.oracle.com/cd/B19306_01/server.102/b14200/pseudocolumns009.htm
(defn- apply-limit [honeysql-query {value :limit}]
;; HoneySQL doesn't support ANSI SQL "OFFSET <n> ROWS FETCH NEXT <n> ROWS ONLY"
;; The semi-official workaround as suggested by yours truly is just to pass a
;; raw string as the `:offset` which HoneySQL puts in the appropriate place
;; see my comment here: https://github.com/jkk/honeysql/issues/58#issuecomment-220450400
(apply-offset-and-limit honeysql-query 0 value))
{:pre [(integer? value)]}
{:select [:*]
:from [honeysql-query]
:where [:<= (hsql/raw "rownum") value]})
(defn- apply-page [honeysql-query {{:keys [items page]} :page}]
;; ex.:
;; items | page | sql
;; ------+------+------------------------
;; 5 | 1 | OFFSET 0 ROWS FETCH FIRST 5 ROWS ONLY
;; 5 | 2 | OFFSET 5 ROWS FETCH FIRST 5 ROWS ONLY
(apply-offset-and-limit honeysql-query (* (dec page) items) items))
(let [offset (* (dec page) items)]
(if (zero? offset)
;; if there's no offset we can use use the single-nesting implementation for `apply-limit`
(apply-limit honeysql-query {:limit items})
;; if we need to do an offset we have to do double-nesting
{:select [:*]
:from [{:select [:__table__.* [(hsql/raw "rownum") :__rownum__]]
:from [[honeysql-query :__table__]]
:where [:<= (hsql/raw "rownum") (+ offset items)]}]
:where [:> :__rownum__ offset]})))
;; Oracle doesn't support `TRUE`/`FALSE`; use `1`/`0`, respectively; convert these booleans to numbers.
(defn- prepare-value [{value :value}]
......@@ -142,6 +170,17 @@
(hsql/call :length field-key))
(defn- remove-rownum-column
"Remove the `:__rownum__` column from results, if present."
[{:keys [columns rows], :as results}]
(if-not (contains? (set columns) :__rownum__)
results
;; if we added __rownum__ it will always be the last column and value so we can just remove that
{:columns (butlast columns)
:rows (for [row rows]
(butlast row))}))
(defrecord OracleDriver []
clojure.lang.Named
(getName [_] "Oracle"))
......@@ -168,7 +207,8 @@
{:name "password"
:display-name "Database password"
:type :password
:placeholder "*******"}])})
:placeholder "*******"}])
:execute-query (comp remove-rownum-column sqlqp/execute-query)})
sql/ISQLDriver
(merge (sql/ISQLDriverDefaultsMixin)
......
src/metabase/query_processor/sql_parameters.clj
+ 27
− 16
View file @ 4699a5cd
......@@ -22,6 +22,8 @@
(defrecord ^:private Dimension [^FieldInstance field, param]) ;; param is either a single param or a vector of params
(defrecord ^:private Date [s])
(defrecord ^:private DateRange [start end])
(defrecord ^:private NumberValue [value])
......@@ -29,10 +31,15 @@
(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)
(cond
;; for relative dates convert the param to a `DateRange` record type and call `->sql` on it
(contains? #{"date/range" "date/month-year" "date/quarter-year" "date/relative"} dimension-type)
(->sql (map->DateRange ((resolve 'metabase.query-processor.parameters/date->range) value *timezone*))) ; TODO - get timezone from query dict
;; for other date types convert the param to a date
(s/starts-with? dimension-type "date/")
(str "= " (->sql (map->Date {:s value})))
;; for everything else just call `->sql` on it directly
:else
(str "= " (->sql value))))
(defn- honeysql->sql ^String [x]
......@@ -40,11 +47,20 @@
:quoting ((resolve 'metabase.driver.generic-sql/quote-style) *driver*))))
(defn- format-oracle-date [s]
(format "TO_TIMESTAMP('%s', 'yyyy-MM-dd')" (u/format-date "yyyy-MM-dd" (u/->Date s))))
(format "to_timestamp('%s', 'YYYY-MM-DD')" (u/format-date "yyyy-MM-dd" (u/->Date s))))
(defn- oracle-driver? ^Boolean []
(when-let [oracle-driver-class (u/ignore-exceptions (Class/forName "metabase.driver.oracle.OracleDriver"))]
(instance? oracle-driver-class *driver*)))
;; we can't just import OracleDriver the normal way here because that would cause a cyclic load dependency
(boolean (when-let [oracle-driver-class (u/ignore-exceptions (Class/forName "metabase.driver.oracle.OracleDriver"))]
(instance? oracle-driver-class *driver*))))
(defn- format-date
;; This is a dirty dirty HACK! Unfortuantely Oracle is super-dumb when it comes to automatically converting strings to dates
;; so we need to add the cast here
[date]
(if (oracle-driver?)
(format-oracle-date date)
(str \' date \')))
(extend-protocol ISQLParamSubstituion
nil (->sql [_] "NULL")
......@@ -62,20 +78,15 @@
((resolve 'metabase.driver.generic-sql/date) *driver* :day identifier)
identifier))))
Date
(->sql [{:keys [s]}]
(format-date s))
DateRange
(->sql [{:keys [start end]}]
;; This is a dirty dirty HACK! Unfortuantely Oracle is super-dumb when it comes to automatically converting strings to dates
;; so we need to add the cast here
;; TODO - fix this when we move to support native params in non-SQL databases
;; Perhaps by making ->sql a multimethod that dispatches off of type and engine
(if (oracle-driver?)
(if (= start end)
(format "= %s" (format-oracle-date start))
(format "BETWEEN %s AND %s" (format-oracle-date start) (format-oracle-date end)))
;; Not the Oracle driver
(if (= start end)
(format "= '%s'" start)
(format "BETWEEN '%s' AND '%s'" start end))))
(if (= start end)
(format "= %s" (format-date start))
(format "BETWEEN %s AND %s" (format-date start) (format-date end))))
Dimension
(->sql [{:keys [field param], :as dimension}]
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment