Add conditional migration for native matview VarsPerHistory
#73
Conversation
|
Are you on the failing tests here? |
|
@jameshiebert , my clever idea of importing items from deeper packages into the top-level package created a circular import. I think that fixing that won't be simple; just converting to "absolute" imports in the lower level packages does not work. I'm inclined to do without, given that I am at this point thinking about doing this quite soon. But if you have any other knowledge to add, I'd be happy to hear it. |
There was a problem hiding this comment.
Yeah, you definitely can't import the pycds module from inside pycds's own __init__.py 😉 If you really want to go down that path, take everything that's defined in __init__.py and define it somewhere else to be imported. But generally, __init__.py should be super conservative.
|
Yeah, I just figured out that moving the contents of |
There was a problem hiding this comment.
This mostly looks good to me, modulo a few minutes comments here and there. I didn't read the testing code super closely, but I did a quick coverage test and most of the modules score pretty high (we do need to work on some upgrades to our testing packages, but I filed a different issue for that).
Overall looks pretty good to me. Thanks for working on this!
| schema=schema_name, | ||
| ) | ||
| op.drop_native_materialized_view(VarsPerHistory, schema=schema_name) | ||
| op.create_table( |
There was a problem hiding this comment.
Should the create_table operation be done *regardless of whether the target database supports native matviews? Presumably, the target revision on a downgrade should have the vars_per_history_mv, no matter what.
There was a problem hiding this comment.
The downgrade does have the table. It deletes the matview and replaces it with a table.
There was a problem hiding this comment.
But only inside the if db_supports_matviews(engine): block. I'm arguing that the migration should ensure that table is created always. Whether the database supports matviews or not.
There was a problem hiding this comment.
Terminology: A migration is a sequence of revisions. A revision is like a git commit: it builds on previous revisions. This revision is concerned with the matview. A previous revision created the table. So all this revision requires is to drop the previously created table and replace it with the matview. The downgrade does the reverse.
|
@jameshiebert , I addressed your comment re. creating table I believe we are ready to migrate
I suggest this particular path because it is simpler than also adding facilities to conditionally drop matviews. It's not much of a simplification, but this PR is getting big and complicated and I'd rather not add to it unnecessarily. Any further comments or feedback? If not, then I can merge this PR and do the above steps. |
|
Looks good @rod-glover . The steps you outline sound good to me, and appear to be a great path forward. |
Resolves #72
TODO:
pycds(enable, e.g.,import VarsPerHistory from pycds).Add drop-if-exists functionality for views, matviews