-
Notifications
You must be signed in to change notification settings - Fork 57
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
across schema query when sqlalchemy.Table autoload=True #30
Conversation
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 your contribution!
Can you add some test cases for these changes? None of these errors were detected by the SQLAlchemy test suite we use, so it would be good to get some sort of test coverage for them to make sure we don't regress in the future.
cockroachdb/sqlalchemy/dialect.py
Outdated
@@ -155,6 +157,10 @@ def get_pk_constraint(self, conn, table_name, schema=None, **kw): | |||
res["name"] = pk["name"] | |||
return res | |||
|
|||
def get_foreign_keys(self, connection, table_name, schema=None, **kw): |
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.
Add a # TODO(bdarnell):
comment here to implement foreign key introspection.
|
||
|
||
class CockroachCompiler(PGCompiler_psycopg2): | ||
def returning_clause(self, stmt, returning_cols): |
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.
Add a TODO
comment to remove this when cockroachdb/cockroach#17008 is fixed.
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.
Looks good, but let me get #29 merged first instead of including it here.
cockroachdb/sqlalchemy/dialect.py
Outdated
@@ -97,7 +99,9 @@ def has_table(self, conn, table, schema=None): | |||
def get_columns(self, conn, table_name, schema=None, **kw): | |||
res = [] | |||
# TODO(bdarnell): escape table name | |||
for row in conn.execute('SHOW COLUMNS FROM "%s"' % table_name): | |||
# when table's schema not equal db uri default schema, need pass schema.table_name | |||
# for row in conn.execute('SHOW COLUMNS FROM "%s"' % table_name): |
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.
Don't just comment out the old code, remove it completely. And I don't think we need the comment above.
Affects get_columns, get_indexes, and the RETURNING clause.
Thanks! I've rebased this to clean up the history now that #29 is merged. |
in dialect:CockroachDBDialect
get_columns and get_indexes change
show * from table
to
show * from schema.table
add stmt_compiler:CockroachCompiler
change
insert into *** returning schema.table.pkey
to
insert into *** returning table.pkey