Skip to content

Materialized Views for sqlalchemy-redshift#202

Merged
jklukas merged 26 commits intosqlalchemy-redshift:masterfrom
ewengillies:materialized_view
May 27, 2020
Merged

Materialized Views for sqlalchemy-redshift#202
jklukas merged 26 commits intosqlalchemy-redshift:masterfrom
ewengillies:materialized_view

Conversation

@ewengillies
Copy link
Contributor

@ewengillies ewengillies commented May 23, 2020

I will be creating a DDLElement that can support Redshift's CREATE MATERIALIZED VIEW with table attributes, noteably sortkeys and distkeys/styles.

Todos

  • Factored out and tested table_attributes parser
  • Write the CreateMaterializedView DDLElement
  • Write the DDL compiler
  • Test on redshift db

Default Todos

  • MIT compatible
  • Unit Tests
  • Documentation
  • Updated CHANGES.rst

Notes:

  • I made the table_attributes parser less permissive to match expected outcomes from Redshift. I'm happy to relax this and delegate the error messages to Redshift, but it could be nice to catch them early. Up to the maintainers.
  • It looked like the same parsing code wanted to allow Columns in additions to strings (and iterables thereof) to be allowed to specify keys. I added the checked needed in if this is the case.
  • I took the liberty of making a new module ddl, since the Create and Drop queries are normally treated somewhat differently than Executable or ClauseElement. This was just an observation of mind, i'm happy to move it all into commands.
  • Unfortunately REFRESH MATERIALIZED VIEWS is a command. So my contribution is a little split between these two modules, perhaps awkwardly. Happy to take advice here.
  • Finally, I made some changes to the doc folder to reflect the new module. I haven't used Sphinx so much, but it all seemed pretty self explanatory. Even so, I'm not sure how to generate the docs to check locally. Please advise, thanks!

@ewengillies ewengillies changed the title refactored table attributes code so we don't need to repeat it Materialized Views for sqlalchemy-redshift May 23, 2020
@ewengillies
Copy link
Contributor Author

@jklukas , here is the MR we discussed on #201 . Awaiting your review, no rush tho! Thanks :)

Copy link
Member

@jklukas jklukas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks great and integration tests are passing, so the refactoring involved looks correct. I left a few nits to fix and otherwise this should be ready to merge.

Ewen Gillies and others added 2 commits May 26, 2020 14:30
Co-authored-by: Jeff Klukas <jeff@klukas.net>
Ewen Gillies and others added 2 commits May 26, 2020 14:36
Co-authored-by: Jeff Klukas <jeff@klukas.net>
@jklukas jklukas merged commit 57384b3 into sqlalchemy-redshift:master May 27, 2020
@jklukas
Copy link
Member

jklukas commented May 27, 2020

I just posted a new release 0.7.9 which includes this change.

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