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

feat(views): Add schema support for views & MVs #621

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kozlek
Copy link

@kozlek kozlek commented Jul 10, 2022

This PR mainly adds schema support for create_view(), create_materialized_view() and refresh_materialized_view().

Also, it adds a populate keyword argument to create_materialized_view(). When set to False, the materialized view will be created with the WITH 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 for create_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:

  • it introduces the usage of f-string, which is supported only from Python 3.6 but it seems to match the actual requirements for the project
  • it does not include typing annotations as the project currently doesn't have any. If you want me to add them, I can do it.
  • it set some arguments like schema and populate 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

@kurtmckee kurtmckee requested a review from kvesteri July 11, 2022 12:16
Copy link
Collaborator

@kurtmckee kurtmckee left a 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?

sqlalchemy_utils/view.py Outdated Show resolved Hide resolved
kozlek and others added 3 commits July 24, 2022 23:18
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.
@kozlek kozlek force-pushed the views-add-schema-and-populate branch from e8ff2c1 to 3a52888 Compare July 24, 2022 21:19
@kozlek
Copy link
Author

kozlek commented Jul 24, 2022

Thanks for your review @kurtmckee.

Pipeline should be fixed by now, and I added typing annotations to the whole views.py.

Regarding missing tests related to cascade option, I realised the actual cascade test suite (based on life_cycle method) is a total non-sense.

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)

create_view is returning a Table object, while views DDL operations (CreateView / DropView )are added as events to be triggered at metadata creation / deletion.
That's why when we call __table__.create(engine), we're calling a Table DDL operation leading to the creation of a table instead of a view.
Postgres logs as a proof:

CREATE TABLE trivial_view (
                id SERIAL NOT NULL,
                PRIMARY KEY (id)
        );
DROP TABLE trivial_view;

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.

@halali
Copy link

halali commented Jun 22, 2023

any update in this ?

@disbr007
Copy link

I would love to see this feature 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.

4 participants