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

Compile Alembic's RenameTable the Postgres way #7

Merged
merged 1 commit into from Aug 11, 2015

Conversation

@bouk
Copy link
Contributor

@bouk bouk commented Aug 10, 2015

@graingert this fixes an issue where RenameTable would be compiled as

ALTER TABLE scheme."old" RENAME TO scheme."new"

@graingert
Copy link
Collaborator

@graingert graingert commented Aug 10, 2015

Nice, but what did this compile as before? Edit: ah it's in your PR description

@bouk
Copy link
Contributor Author

@bouk bouk commented Aug 10, 2015

I put it in the description:

ALTER TABLE scheme."old" RENAME TO scheme."new"

Note the scheme that's included in the TO, which Postgres doesn't support

@@ -12,6 +12,9 @@
except ImportError:
pass
else:
from alembic.ddl.base import RenameTable
compiles(RenameTable, 'redshift')(postgresql.visit_rename_table)

class RedshiftImpl(postgresql.PostgresqlImpl):

This comment has been minimized.

@graingert

graingert Aug 10, 2015
Collaborator

Do you know why this isn't pulled in by subclassing the PostgresalImpl?

This comment has been minimized.

@bouk

bouk Aug 10, 2015
Author Contributor

It's because sqlalchemy uses the value in __dialect__, which RedshiftImpl overrides

This is where alembic defines the postgres override: https://github.com/zzzeek/alembic/blob/eb077a6c8d2c468736ba537eb1ab5400c9059aad/alembic/ddl/postgresql.py#L124

This comment has been minimized.

@graingert

graingert Aug 10, 2015
Collaborator

@zzzeek do you know the best way to inherit dialects' "compiles" declarations?

Eg in a way that if more are added I don't have to keep updating my subclass.

This comment has been minimized.

@zzzeek

zzzeek Aug 10, 2015

well Redshift should have an alembic impl called RedshiftImpl and use __dialect__ = 'redshift', subclassing PostgresqlImpl

This comment has been minimized.

@zzzeek

zzzeek Aug 10, 2015

so you have that....and....@compiles(RenameTable, 'redshift') should do the individual elements

This comment has been minimized.

@graingert

graingert Aug 10, 2015
Collaborator

Ah, so there's no nice way to automatically inherit the @compiles declarations

This comment has been minimized.

@zzzeek

zzzeek Aug 10, 2015

right so why not subclassing. well @compiles someday might want to take into account the hierarchy of SQLCompiler subclasses. not really sure. redshift is a very odd case.

@graingert
graingert reviewed Aug 10, 2015
View changes
tests/test_alembic_dialect.py Outdated

def test_rename_table():
compiler = dialect.RedShiftDDLCompiler(dialect.RedshiftDialect(), None)
assert compiler.process(RenameTable("old", "new", "scheme")) == 'ALTER TABLE scheme."old" RENAME TO "new"'

This comment has been minimized.

@graingert

graingert Aug 10, 2015
Collaborator

Nice test, but I'd like to avoid horizontal GitHub scrolling for new code.

sql = compiler.process(RenameTable('old', 'new, 'schema'))
assert sql == 'ALTER TABLE schema."old" RENAME TO "new"'

This comment has been minimized.

@graingert

graingert Aug 11, 2015
Collaborator

Also it's schema not scheme

@bouk bouk force-pushed the bouk:compile-rename-properly branch to b99b8c3 Aug 11, 2015
graingert added a commit that referenced this pull request Aug 11, 2015
Compile Alembic's RenameTable the Postgres way
@graingert graingert merged commit a9578c8 into sqlalchemy-redshift:master Aug 11, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bouk bouk deleted the bouk:compile-rename-properly branch Aug 11, 2015
haleemur pushed a commit to haleemur/redshift_sqlalchemy that referenced this pull request Sep 2, 2015
…perly

Compile Alembic's RenameTable the Postgres way
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.