-
Notifications
You must be signed in to change notification settings - Fork 177
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
Add ability to specify catalog as a SQLALchemy table argument #186
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,17 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
from sqlalchemy.sql import compiler | ||
try: | ||
from sqlalchemy.sql.expression import ( | ||
Alias, | ||
CTE, | ||
Subquery, | ||
) | ||
except ImportError: | ||
# For SQLAlchemy versions < 1.4, the CTE and Subquery classes did not explicitly exist | ||
from sqlalchemy.sql.expression import Alias | ||
CTE = type(None) | ||
Subquery = type(None) | ||
Comment on lines
+22
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you leave a comment here why this is ok to do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, this is because the |
||
|
||
# https://trino.io/docs/current/language/reserved.html | ||
RESERVED_WORDS = { | ||
|
@@ -102,6 +113,31 @@ def limit_clause(self, select, **kw): | |
text += "\nLIMIT " + self.process(select._limit_clause, **kw) | ||
return text | ||
|
||
def visit_table(self, table, asfrom=False, iscrud=False, ashint=False, | ||
fromhints=None, use_schema=True, **kwargs): | ||
sql = super(TrinoSQLCompiler, self).visit_table( | ||
table, asfrom, iscrud, ashint, fromhints, use_schema, **kwargs | ||
) | ||
return self.add_catalog(sql, table) | ||
|
||
@staticmethod | ||
def add_catalog(sql, table): | ||
if table is None: | ||
return sql | ||
|
||
if isinstance(table, (Alias, CTE, Subquery)): | ||
return sql | ||
|
||
if ( | ||
'trino' not in table.dialect_options | ||
or 'catalog' not in table.dialect_options['trino'] | ||
): | ||
return sql | ||
|
||
catalog = table.dialect_options['trino']['catalog'] | ||
sql = f'"{catalog}".{sql}' | ||
return sql | ||
|
||
|
||
class TrinoDDLCompiler(compiler.DDLCompiler): | ||
pass | ||
|
@@ -173,3 +209,7 @@ def visit_TIME(self, type_, **kw): | |
|
||
class TrinoIdentifierPreparer(compiler.IdentifierPreparer): | ||
reserved_words = RESERVED_WORDS | ||
|
||
def format_table(self, table, use_schema=True, name=None): | ||
result = super(TrinoIdentifierPreparer, self).format_table(table, use_schema, name) | ||
return TrinoSQLCompiler.add_catalog(result, table) |
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.
Seems pre-existing but any idea why the schema name doesn't get quoted?
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.
Looked into this a little.
visit_table
quotes table names by default, but it checks whether schema names "need" to be quoted (e.g. if they are a reserved word). The PyHive package got around this by considering everything to be a reserved word, which would trigger this quoting. I don't think it's necessary here, but it definitely is a little weird to see schema names not quoted while table names are.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 for looking into this and explaining. I think we can leave as is for now since from the few queries I tested with special schema names it was able to handle them correctly. I didn't try too hard to break it though to be honest. 😄