-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
src/sqlalchemy_cratedb/compiler.py
Outdated
class CrateIdentifierPreparer(sa.sql.compiler.IdentifierPreparer): | ||
""" | ||
Define CrateDB's reserved words to be quoted properly. | ||
""" | ||
# TODO: There are certainly more to add here than just `object`? | ||
reserved_words = set(list(POSTGRESQL_RESERVED_WORDS) + ["object"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the past I extracted many into https://github.com/surister/cratedb-admin-alt/blob/master/src/store/crate_api/crate_lang.js but it is incomplete, perhaps we could maintain a crate-owned Gist with all of this properly done and complete. It could be javascript or python, dicts and lists are generally very copy-paste friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be sweet to maintain such a list on behalf of a Python generator, which derives them directly from the ANTLR grammar or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again. Do you think cratedb-sqlparse should be used here, or would it be too heavy? Can you think of any alternatives?
33e74cf
to
27f3e35
Compare
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
27f3e35
to
7970219
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx!
By using this component of the SQLAlchemy dialect compiler, it can define CrateDB's reserved words to be quoted properly when building SQL statements. This allows to quote reserved words like `index` or `object` properly, for example when used as column names.
7970219
to
f4b4ab3
Compare
About
By using this component of the SQLAlchemy dialect compiler, it can define CrateDB's reserved words to be quoted properly when building SQL statements.
References
In this case,
OBJECT
needs to be quoted when used as column name.Backlog