- The limit/offset keywords to select() as well

as the value passed to select.limit()/offset()
will be coerced to integer.  [ticket:2116]
(also in 0.6.7)
- Oracle dialect adds use_binds_for_limits=False
create_engine() flag, will render the LIMIT/OFFSET
values inline instead of as binds, reported to
modify the execution plan used by Oracle.
[ticket:2116] (Also in 0.6.7)

7 files changed, 121 insertions, 14 deletions

diff --git a/CHANGES b/CHANGES
index c71aa03d5..a1d49f036 100644
--- a/CHANGES
+++ b/CHANGES
@@ -58,6 +58,11 @@ CHANGES
     collection of Sequence objects, list
     of schema names.  [ticket:2104]
 
+  - The limit/offset keywords to select() as well
+    as the value passed to select.limit()/offset()
+    will be coerced to integer.  [ticket:2116]
+    (also in 0.6.7)
+
 - schema
   - The 'useexisting' flag on Table has been superceded
     by a new pair of flags 'keep_existing' and 
@@ -101,6 +106,12 @@ CHANGES
     talking to cx_oracle.  [ticket:2100]  (Also
     in 0.6.7)
 
+  - Oracle dialect adds use_binds_for_limits=False
+    create_engine() flag, will render the LIMIT/OFFSET
+    values inline instead of as binds, reported to
+    modify the execution plan used by Oracle.
+    [ticket:2116] (Also in 0.6.7)
+
 - documentation
   - Documented SQLite DATE/TIME/DATETIME types.
     [ticket:2029] (also in 0.6.7)
diff --git a/lib/sqlalchemy/dialects/oracle/base.py b/lib/sqlalchemy/dialects/oracle/base.py
index 72411d735..14e6309cb 100644
--- a/lib/sqlalchemy/dialects/oracle/base.py
+++ b/lib/sqlalchemy/dialects/oracle/base.py
@@ -22,6 +22,8 @@ affect the behavior of the dialect regardless of driver in use.
 
 * *optimize_limits* - defaults to ``False``. see the section on LIMIT/OFFSET.
 
+* *use_binds_for_limits* - defaults to ``True``.  see the section on LIMIT/OFFSET.
+
 Auto Increment Behavior
 -----------------------
 
@@ -74,13 +76,27 @@ requires NLS_LANG to be set.
 LIMIT/OFFSET Support
 --------------------
 
-Oracle has no support for the LIMIT or OFFSET keywords.  Whereas previous versions of SQLAlchemy
-used the "ROW NUMBER OVER..." construct to simulate LIMIT/OFFSET, SQLAlchemy 0.5 now uses 
-a wrapped subquery approach in conjunction with ROWNUM.  The exact methodology is taken from
-http://www.oracle.com/technology/oramag/oracle/06-sep/o56asktom.html .  Note that the 
-"FIRST ROWS()" optimization keyword mentioned is not used by default, as the user community felt
-this was stepping into the bounds of optimization that is better left on the DBA side, but this
-prefix can be added by enabling the optimize_limits=True flag on create_engine().
+Oracle has no support for the LIMIT or OFFSET keywords.  SQLAlchemy uses 
+a wrapped subquery approach in conjunction with ROWNUM.  The exact methodology 
+is taken from
+http://www.oracle.com/technology/oramag/oracle/06-sep/o56asktom.html .  
+
+There are two options which affect its behavior:
+
+* the "FIRST ROWS()" optimization keyword is not used by default.  To enable the usage of this
+  optimization directive, specify ``optimize_limits=True`` to :func:`.create_engine`.
+* the values passed for the limit/offset are sent as bound parameters.   Some users have observed
+  that Oracle produces a poor query plan when the values are sent as binds and not
+  rendered literally.   To render the limit/offset values literally within the SQL 
+  statement, specify ``use_binds_for_limits=False`` to :func:`.create_engine`.
+
+Some users have reported better performance when the entirely different approach of a 
+window query is used, i.e. ROW_NUMBER() OVER (ORDER BY), to provide LIMIT/OFFSET (note 
+that the majority of users don't observe this).  To suit this case the 
+method used for LIMIT/OFFSET can be replaced entirely.  See the recipe at 
+http://www.sqlalchemy.org/trac/wiki/UsageRecipes/WindowFunctionsByDefault
+which installs a select compiler that overrides the generation of limit/offset with
+a window function.
 
 ON UPDATE CASCADE
 -----------------
@@ -524,6 +540,8 @@ class OracleCompiler(compiler.SQLCompiler):
                     max_row = select._limit
                     if select._offset is not None:
                         max_row += select._offset
+                    if not self.dialect.use_binds_for_limits:
+                        max_row = sql.literal_column("%d" % max_row)
                     limitselect.append_whereclause(
                             sql.literal_column("ROWNUM")<=max_row)
 
@@ -542,8 +560,11 @@ class OracleCompiler(compiler.SQLCompiler):
                     offsetselect._oracle_visit = True
                     offsetselect._is_wrapper = True
 
+                    offset_value = select._offset
+                    if not self.dialect.use_binds_for_limits:
+                        offset_value = sql.literal_column("%d" % offset_value)
                     offsetselect.append_whereclause(
-                             sql.literal_column("ora_rn")>select._offset)
+                             sql.literal_column("ora_rn")>offset_value)
 
                     offsetselect.for_update = select.for_update
                     select = offsetselect
@@ -635,10 +656,12 @@ class OracleDialect(default.DefaultDialect):
     def __init__(self, 
                 use_ansi=True, 
                 optimize_limits=False, 
+                use_binds_for_limits=True,
                 **kwargs):
         default.DefaultDialect.__init__(self, **kwargs)
         self.use_ansi = use_ansi
         self.optimize_limits = optimize_limits
+        self.use_binds_for_limits = use_binds_for_limits
 
     def initialize(self, connection):
         super(OracleDialect, self).initialize(connection)
diff --git a/lib/sqlalchemy/sql/expression.py b/lib/sqlalchemy/sql/expression.py
index 3323dcca9..f5ec41a60 100644
--- a/lib/sqlalchemy/sql/expression.py
+++ b/lib/sqlalchemy/sql/expression.py
@@ -3972,6 +3972,8 @@ class _SelectBase(Executable, FromClause):
 
     _order_by_clause = ClauseList()
     _group_by_clause = ClauseList()
+    _limit = None
+    _offset = None
 
     def __init__(self,
             use_labels=False,
@@ -3991,8 +3993,10 @@ class _SelectBase(Executable, FromClause):
             self._execution_options = \
                 self._execution_options.union({'autocommit'
                     : autocommit})
-        self._limit = limit
-        self._offset = offset
+        if limit is not None:
+            self._limit = util.asint(limit)
+        if offset is not None:
+            self._offset = util.asint(offset)
         self._bind = bind
 
         if order_by is not None:
@@ -4061,14 +4065,14 @@ class _SelectBase(Executable, FromClause):
         """return a new selectable with the given LIMIT criterion
         applied."""
 
-        self._limit = limit
+        self._limit = util.asint(limit)
 
     @_generative
     def offset(self, offset):
         """return a new selectable with the given OFFSET criterion
         applied."""
 
-        self._offset = offset
+        self._offset = util.asint(offset)
 
     @_generative
     def order_by(self, *clauses):
diff --git a/lib/sqlalchemy/util/__init__.py b/lib/sqlalchemy/util/__init__.py
index 7e8ce59c4..93c418eda 100644
--- a/lib/sqlalchemy/util/__init__.py
+++ b/lib/sqlalchemy/util/__init__.py
@@ -25,7 +25,7 @@ from langhelpers import iterate_attributes, class_hierarchy, \
     monkeypatch_proxied_specials, asbool, bool_or_str, coerce_kw_type,\
     duck_type_collection, assert_arg_type, symbol, dictlike_iteritems,\
     classproperty, set_creation_order, warn_exception, warn, NoneType,\
-    constructor_copy, methods_equivalent, chop_traceback
+    constructor_copy, methods_equivalent, chop_traceback, asint
 
 from deprecations import warn_deprecated, warn_pending_deprecation, \
     deprecated, pending_deprecation
diff --git a/lib/sqlalchemy/util/langhelpers.py b/lib/sqlalchemy/util/langhelpers.py
index cbe0d2a4f..ba612bc2c 100644
--- a/lib/sqlalchemy/util/langhelpers.py
+++ b/lib/sqlalchemy/util/langhelpers.py
@@ -551,6 +551,14 @@ def bool_or_str(*text):
             return asbool(obj)
     return bool_or_value
 
+def asint(value):
+    """Coerce to integer."""
+
+    if value is None:
+        return value
+    return int(value)
+
+
 def coerce_kw_type(kw, key, type_, flexi_bool=True):
     """If 'key' is present in dict 'kw', coerce its value to type 'type\_' if
     necessary.  If 'flexi_bool' is True, the string '0' is considered false
diff --git a/test/dialect/test_oracle.py b/test/dialect/test_oracle.py
index 26e46349e..f3cdcc3a9 100644
--- a/test/dialect/test_oracle.py
+++ b/test/dialect/test_oracle.py
@@ -132,6 +132,49 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL):
                             ':ROWNUM_1) WHERE ora_rn > :ora_rn_1 FOR '
                             'UPDATE')
 
+    def test_use_binds_for_limits_disabled(self):
+        t = table('sometable', column('col1'), column('col2'))
+        dialect = oracle.OracleDialect(use_binds_for_limits = False)
+
+        self.assert_compile(select([t]).limit(10),
+                "SELECT col1, col2 FROM (SELECT sometable.col1 AS col1, "
+                "sometable.col2 AS col2 FROM sometable) WHERE ROWNUM <= 10",
+                dialect=dialect)
+
+        self.assert_compile(select([t]).offset(10),
+                "SELECT col1, col2 FROM (SELECT col1, col2, ROWNUM AS ora_rn "
+                "FROM (SELECT sometable.col1 AS col1, sometable.col2 AS col2 "
+                "FROM sometable)) WHERE ora_rn > 10",
+                dialect=dialect)
+
+        self.assert_compile(select([t]).limit(10).offset(10),
+                "SELECT col1, col2 FROM (SELECT col1, col2, ROWNUM AS ora_rn "
+                "FROM (SELECT sometable.col1 AS col1, sometable.col2 AS col2 "
+                "FROM sometable) WHERE ROWNUM <= 20) WHERE ora_rn > 10",
+                dialect=dialect)
+
+    def test_use_binds_for_limits_enabled(self):
+        t = table('sometable', column('col1'), column('col2'))
+        dialect = oracle.OracleDialect(use_binds_for_limits = True)
+
+        self.assert_compile(select([t]).limit(10),
+                "SELECT col1, col2 FROM (SELECT sometable.col1 AS col1, "
+                "sometable.col2 AS col2 FROM sometable) WHERE ROWNUM "
+                "<= :ROWNUM_1",
+                dialect=dialect)
+
+        self.assert_compile(select([t]).offset(10),
+                "SELECT col1, col2 FROM (SELECT col1, col2, ROWNUM AS ora_rn "
+                "FROM (SELECT sometable.col1 AS col1, sometable.col2 AS col2 "
+                "FROM sometable)) WHERE ora_rn > :ora_rn_1",
+                dialect=dialect)
+
+        self.assert_compile(select([t]).limit(10).offset(10),
+                "SELECT col1, col2 FROM (SELECT col1, col2, ROWNUM AS ora_rn "
+                "FROM (SELECT sometable.col1 AS col1, sometable.col2 AS col2 "
+                "FROM sometable) WHERE ROWNUM <= :ROWNUM_1) WHERE ora_rn > "
+                ":ora_rn_1",
+                dialect=dialect)
 
     def test_long_labels(self):
         dialect = default.DefaultDialect()
diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py
index bfb282050..ce3e9003b 100644
--- a/test/sql/test_compiler.py
+++ b/test/sql/test_compiler.py
@@ -1,5 +1,5 @@
 from test.lib.testing import eq_, assert_raises, assert_raises_message
-import datetime, re, operator
+import datetime, re, operator, decimal
 from sqlalchemy import *
 from sqlalchemy import exc, sql, util
 from sqlalchemy.sql import table, column, label, compiler
@@ -106,6 +106,24 @@ class SelectTest(fixtures.TestBase, AssertsCompiledSQL):
         assert_raises(exc.ArgumentError, select, table1)
         assert_raises(exc.ArgumentError, select, table1.c.myid)
 
+    def test_int_limit_offset_coercion(self):
+        for given, exp in [
+            ("5", 5),
+            (5, 5),
+            (5.2, 5),
+            (decimal.Decimal("5"), 5),
+            (None, None),
+        ]:
+            eq_(select().limit(given)._limit, exp)
+            eq_(select().offset(given)._offset, exp)
+            eq_(select(limit=given)._limit, exp)
+            eq_(select(offset=given)._offset, exp)
+
+        assert_raises(ValueError, select().limit, "foo")
+        assert_raises(ValueError, select().offset, "foo")
+        assert_raises(ValueError, select, offset="foo")
+        assert_raises(ValueError, select, limit="foo")
+
     def test_from_subquery(self):
         """tests placing select statements in the column clause of another select, for the
         purposes of selecting from the exported columns of that select."""