Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler: Add CrateIdentifierPreparer for properly quoting reserved words #21

Merged
merged 1 commit into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

## Unreleased
- Added/reactivated documentation as `sqlalchemy-cratedb`
- Added `CrateIdentifierPreparer`, in order to quote reserved words
like `object` properly, for example when used as column names.

## 2024/06/13 0.37.0
- Added support for CrateDB's [FLOAT_VECTOR] data type and its accompanying
Expand Down
37 changes: 37 additions & 0 deletions src/sqlalchemy_cratedb/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

import sqlalchemy as sa
from sqlalchemy.dialects.postgresql.base import PGCompiler
from sqlalchemy.dialects.postgresql.base import RESERVED_WORDS as POSTGRESQL_RESERVED_WORDS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have some deviations from the postgres reserved keywords, I think it's better if we used the python sqlparser to get the list for CrateDB programmatically.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do it on a later iteration if we agree. I am not sure if it would be too heavy, see #21 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, we can do in next step.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Maybe you want to submit a corresponding patch, @surister?

from sqlalchemy.sql import compiler
from sqlalchemy.types import String
from .type.geo import Geopoint, Geoshape
Expand Down Expand Up @@ -323,3 +324,39 @@
warnings.warn("CrateDB does not support the 'INSERT ... FOR UPDATE' clause, "
"it will be omitted when generating SQL statements.")
return ''


CRATEDB_RESERVED_WORDS = \
matriv marked this conversation as resolved.
Show resolved Hide resolved
"add, alter, between, by, called, costs, delete, deny, directory, drop, escape, exists, " \
"extract, first, function, if, index, input, insert, last, match, nulls, object, " \
"persistent, recursive, reset, returns, revoke, set, stratify, transient, try_cast, " \
"unbounded, update".split(", ")


class CrateIdentifierPreparer(sa.sql.compiler.IdentifierPreparer):
"""
Define CrateDB's reserved words to be quoted properly.
"""
reserved_words = set(list(POSTGRESQL_RESERVED_WORDS) + CRATEDB_RESERVED_WORDS)

def _unquote_identifier(self, value):
if value[0] == self.initial_quote:
value = value[1:-1].replace(

Check warning on line 344 in src/sqlalchemy_cratedb/compiler.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/compiler.py#L343-L344

Added lines #L343 - L344 were not covered by tests
self.escape_to_quote, self.escape_quote
)
return value

Check warning on line 347 in src/sqlalchemy_cratedb/compiler.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/compiler.py#L347

Added line #L347 was not covered by tests

def format_type(self, type_, use_schema=True):
if not type_.name:
raise sa.exc.CompileError("Type requires a name.")

Check warning on line 351 in src/sqlalchemy_cratedb/compiler.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/compiler.py#L350-L351

Added lines #L350 - L351 were not covered by tests

name = self.quote(type_.name)
effective_schema = self.schema_for_object(type_)

Check warning on line 354 in src/sqlalchemy_cratedb/compiler.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/compiler.py#L353-L354

Added lines #L353 - L354 were not covered by tests

if (

Check warning on line 356 in src/sqlalchemy_cratedb/compiler.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/compiler.py#L356

Added line #L356 was not covered by tests
not self.omit_schema
and use_schema
and effective_schema is not None
):
name = self.quote_schema(effective_schema) + "." + name
return name

Check warning on line 362 in src/sqlalchemy_cratedb/compiler.py

View check run for this annotation

Codecov / codecov/patch

src/sqlalchemy_cratedb/compiler.py#L361-L362

Added lines #L361 - L362 were not covered by tests
4 changes: 3 additions & 1 deletion src/sqlalchemy_cratedb/dialect.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@

from .compiler import (
CrateTypeCompiler,
CrateDDLCompiler
CrateDDLCompiler,
CrateIdentifierPreparer,
)
from crate.client.exceptions import TimezoneUnawareException
from .sa_version import SA_VERSION, SA_1_4, SA_2_0
Expand Down Expand Up @@ -174,6 +175,7 @@ class CrateDialect(default.DefaultDialect):
statement_compiler = statement_compiler
ddl_compiler = CrateDDLCompiler
type_compiler = CrateTypeCompiler
preparer = CrateIdentifierPreparer
use_insertmanyvalues = True
use_insertmanyvalues_wo_returning = True
supports_multivalues_insert = True
Expand Down
28 changes: 28 additions & 0 deletions tests/compiler_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -432,3 +432,31 @@ class FooBar(Base):
self.assertIsSubclass(w[-1].category, UserWarning)
self.assertIn("CrateDB does not support unique constraints, "
"they will be omitted when generating DDL statements.", str(w[-1].message))

def test_ddl_with_reserved_words(self):
"""
Verify CrateDB's reserved words like `object` are quoted properly.
"""

Base = declarative_base(metadata=self.metadata)

class FooBar(Base):
"""The entity."""

__tablename__ = "foobar"

index = sa.Column(sa.Integer, primary_key=True)
array = sa.Column(sa.String)
object = sa.Column(sa.String)

# Verify SQL DDL statement.
self.metadata.create_all(self.engine, tables=[FooBar.__table__], checkfirst=False)
self.assertEqual(self.executed_statement, dedent("""
CREATE TABLE testdrive.foobar (
\t"index" INT NOT NULL,
\t"array" STRING,
\t"object" STRING,
\tPRIMARY KEY ("index")
)

""")) # noqa: W291, W293