Fix generating appropriate syntax for OFFSET/LIMIT clauses

The standard SQLAlchemy statements compiler can generate `LIMIT -1`
clauses, but PostgreSQL and CrateDB do not accept them, and need
`LIMIT ALL` instead.

The fix is to re-use the existing `PGCompiler.limit_clause` method
within `CrateCompiler`.

Author: amotl

CHANGES.txt

@@ -7,6 +7,10 @@ Unreleased
 - Added a generic data type converter to the ``Cursor`` object, for converting
   fetched data from CrateDB data types to Python data types.
 
+- Fixed generating appropriate syntax for OFFSET/LIMIT clauses. It was possible
+  that SQL statement clauses like ``LIMIT -1`` could have been generated. Both
+  PostgreSQL and CrateDB only accept ``LIMIT ALL`` instead.
+
 
 2022/10/10 0.27.2
 =================

src/crate/client/sqlalchemy/compiler.py

@@ -23,6 +23,7 @@
 from collections import defaultdict
 
 import sqlalchemy as sa  # lgtm[py/import-and-import-from]
+from sqlalchemy.dialects.postgresql.base import PGCompiler
 from sqlalchemy.sql import compiler, crud, selectable  # lgtm[py/import-and-import-from]
 from .types import MutableDict
 from .sa_version import SA_VERSION, SA_1_4
@@ -289,6 +290,12 @@ def visit_update(self, update_stmt, **kw):
 
         return text
 
+    def limit_clause(self, select, **kw):
+        """
+        Generate OFFSET / LIMIT clause, PostgreSQL-compatible.
+        """
+        return PGCompiler.limit_clause(self, select, **kw)
+
     def _get_crud_params(compiler, stmt, **kw):
         """ extract values from crud parameters
         taken from SQLAlchemy's crud module (since 1.0.x) and

src/crate/client/sqlalchemy/tests/compiler_test.py

@@ -34,8 +34,8 @@ class SqlAlchemyCompilerTest(TestCase):
     def setUp(self):
         self.crate_engine = sa.create_engine('crate://')
         self.sqlite_engine = sa.create_engine('sqlite://')
-        metadata = sa.MetaData()
-        self.mytable = sa.Table('mytable', metadata,
+        self.metadata = sa.MetaData()
+        self.mytable = sa.Table('mytable', self.metadata,
                                 sa.Column('name', sa.String),
                                 sa.Column('data', Craty))
 
@@ -69,3 +69,30 @@ def test_bulk_update_on_builtin_type(self):
         )
 
         self.assertFalse(hasattr(clauseelement, '_crate_specific'))
+
+    def test_select_with_offset(self):
+        """
+        Verify the `CrateCompiler.limit_clause` method, with offset.
+        """
+        self.metadata.bind = self.crate_engine
+        selectable = self.mytable.select().offset(5)
+        statement = str(selectable.compile())
+        self.assertEqual(statement, "SELECT mytable.name, mytable.data \nFROM mytable\n LIMIT ALL OFFSET ?")
+
+    def test_select_with_limit(self):
+        """
+        Verify the `CrateCompiler.limit_clause` method, with limit.
+        """
+        self.metadata.bind = self.crate_engine
+        selectable = self.mytable.select().limit(42)
+        statement = str(selectable.compile())
+        self.assertEqual(statement, "SELECT mytable.name, mytable.data \nFROM mytable \n LIMIT ?")
+
+    def test_select_with_offset_and_limit(self):
+        """
+        Verify the `CrateCompiler.limit_clause` method, with offset and limit.
+        """
+        self.metadata.bind = self.crate_engine
+        selectable = self.mytable.select().offset(5).limit(42)
+        statement = str(selectable.compile())
+        self.assertEqual(statement, "SELECT mytable.name, mytable.data \nFROM mytable \n LIMIT ? OFFSET ?")