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

across schema query when sqlalchemy.Table autoload=True #30

Merged
merged 1 commit into from
Jul 15, 2017

Conversation

xinbo
Copy link

@xinbo xinbo commented Jul 12, 2017

  1. in dialect:CockroachDBDialect
    get_columns and get_indexes change
    show * from table
    to
    show * from schema.table

  2. add stmt_compiler:CockroachCompiler
    change
    insert into *** returning schema.table.pkey
    to
    insert into *** returning table.pkey

Copy link
Contributor

@bdarnell bdarnell left a 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.

@@ -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):
Copy link
Contributor

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):
Copy link
Contributor

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.

@xinbo xinbo changed the title 1. in dialect:CockroachDBDialect across schema query when Table(autoload=True, schema=self.TEST_DATABASE). Jul 13, 2017
@xinbo xinbo changed the title across schema query when Table(autoload=True, schema=self.TEST_DATABASE). across schema query when sqlalchemy.Table autoload=True Jul 13, 2017
Copy link
Contributor

@bdarnell bdarnell left a 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.

@@ -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):
Copy link
Contributor

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.
@bdarnell
Copy link
Contributor

Thanks! I've rebased this to clean up the history now that #29 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants