-
Notifications
You must be signed in to change notification settings - Fork 321
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
feat(views): Add schema support for views & MVs #621
base: master
Are you sure you want to change the base?
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.
It's fine to use f-strings. Type annotations will be helpful because I'd like to see strong type annotations in the future.
The tests are currently failing; please review the CI logs for details. The tests also appear to be incomplete because several code paths are not exercised. In particular, cascade
and populate
can be one of three values now, and I don't think all of the possibilities are getting tested.
I like that cascade_on_drop
is None
to better match postgres defaults, but this is a breaking change. I cannot merge this because I'm not familiar with views or how sqlalchemy-utils deals with breaking changes. @kvesteri, would you provide feedback about this change, including (a) converting parameters to be keyword-only, (b) changing the cascade_on_drop
behavior, (c) anything else you see?
Add also populate=False kwarg for create_materialized_view(), which allows MVs to be created WITH NO DATA. BREAKING CHANGE: cascade_on_drop is now None by default, was True before. Also, cascade_on_drop is now a keyword-only argument.
Co-authored-by: Kurt McKee <[email protected]>
e8ff2c1
to
3a52888
Compare
Thanks for your review @kurtmckee. Pipeline should be fixed by now, and I added typing annotations to the whole Regarding missing tests related to class TrivialViewTestCases:
def life_cycle(
self,
engine,
metadata,
column,
cascade_on_drop
):
__table__ = create_view(
name='trivial_view',
selectable=sa.select(*_select_args(column)),
metadata=metadata,
cascade_on_drop=cascade_on_drop
)
__table__.create(engine)
__table__.drop(engine)
We're not testing anything here, at least not something related to views. In order to complete this PR, I'd like to add missing tests as you asked, but I'd also like to rewrite the whole cascade related tests suite if that's okay for you. |
any update in this ? |
I would love to see this feature merged. |
This PR mainly adds schema support for
create_view()
,create_materialized_view()
andrefresh_materialized_view()
.Also, it adds a
populate
keyword argument tocreate_materialized_view()
. When set toFalse
, the materialized view will be created with theWITH NO DATA
flag, which is useful when migrating a database schema with heavy materialized views.Regarding
cascade_on_drop
when creating views, it is now available forcreate_materialized_view()
.However, it has been set to None by default, instead of the previous True, in order to better match PostgreSQL default's mode of operation.
I understand this is a breaking change, and I'll be happy to revert that part if you're not confortable with that.
Others remarks about the PR:
schema
andpopulate
as keyword-only arguments, as I believe they are non trivial arguments. It can be removed if you don't like that.See also: #427