crate / sqlalchemy-cratedb

SQLAlchemy dialect for CrateDB.
https://cratedb.com/docs/sqlalchemy-cratedb/
Apache License 2.0
3 stars 2 forks source link

SQLParseException[line 4:8: no viable alternative at input 'LIMIT -'] #113

Closed amotl closed 1 year ago

amotl commented 1 year ago

Problem

sqlalchemy.exc.ProgrammingError: (crate.client.exceptions.ProgrammingError) SQLParseException[line 4:8: no viable alternative at input 'LIMIT -']
[SQL: SELECT testdrive.id, testdrive.name, testdrive.age, testdrive.gender
FROM testdrive
WHERE testdrive.name = ?
 LIMIT -1 OFFSET ?]
[parameters: ('John Doe', 0)]
(Background on this error at: https://sqlalche.me/e/14/f405)

Reproduction

Enable the Query multiple records code of cratedb-dataset-demo.py and run it.

Reason

SQLAlchemy's compiler.py:limit_clause implementation:

    def limit_clause(self, select, **kw):
        text = ""
        if select._limit_clause is not None:
            text += "\n LIMIT " + self.process(select._limit_clause, **kw)
        if select._offset_clause is not None:
            if select._limit_clause is None:
                text += "\n LIMIT -1"
            text += " OFFSET " + self.process(select._offset_clause, **kw)
        return text
seut commented 1 year ago

We should probably switch to extend from the PGCompiler instead of the default SQLCompiler. See https://github.com/sqlalchemy/sqlalchemy/blob/rel_1_4_41/lib/sqlalchemy/dialects/postgresql/base.py#L2394. CrateDB (like PG) does not support negative LIMIT values.

amotl commented 1 year ago

Wow, thanks for quickly looking that up. My thoughts have been similar, like »let's just take the limit_clause function from the PostgreSQL dialect?«.

amotl commented 1 year ago

On this matter, I just discovered that LIMIT ALL is only possible when phrased without OFFSET.

Works:

cr> SELECT * FROM testdrive WHERE name='John Doe' LIMIT ALL;

Croaks:

cr> SELECT * FROM testdrive WHERE name='John Doe' LIMIT ALL OFFSET 0;
SQLParseException[Invalid LIMIT: value must be >= 0; got: -1]

When looking at the SQLAlchemy code, we can clearly see that it can build such an SQL statement. Please +1 on this comment if you think I should create a corresponding issue at crate/crate.

Edit: Upstream issue and patch:

amotl commented 1 year ago
  1. The CrateDB-related fix is https://github.com/crate/crate/pull/13108. Thanks, @matriv.
  2. The SQLAlchemy-related fix is crate/crate-python#472.