Fix: Handle view materialization alterations#610
Conversation
axellpadilla
changed the title
axellpadilla
force-pushed
the
view_materialization_improvement
branch
from
30a2bf6 to
18ea497
Compare
|
Hi @cody-scott , did you have any time to review this? Maybe you could authorize some extra hands to make reviews or even maintainers, thing is, on sql server a drop/create view are 2 steps and plenty of system tables modifications, while alter is 1 step and less overhead, this should be a high impact change for big dbt projects (it is for my use cases). |
axellpadilla
force-pushed
the
view_materialization_improvement
branch
from
18ea497 to
a031b2b
Compare
Benjamin-Knight
left a comment
Benjamin-Knight
left a comment
There was a problem hiding this comment.
Based on the potential issue with the view.sql changes I think this change is missing 3 test paths, 1 is optional, that the base dbt tests are not covering.
Going from a table -> view needs checking.
Ensuring that if the new view definition is incorrect that we do not break the deployment.
The optional path would be that the request has 'non-dbt grants deletion' as part of the reason to make this change. You could add in a test to ensure that this path preserves these, partly as a regression test but also because of the all the intermediate objects in the macro to ensure that grants persist across that dance.,
axellpadilla
force-pushed
the
view_materialization_improvement
branch
from
a031b2b to
9e46039
Compare
axellpadilla
force-pushed
the
view_materialization_improvement
branch
from
9e46039 to
1b5b22d
Compare
|
@Benjamin-Knight added the no-op please check if a good solution and merge or change when ready |
|
That new grant logic, it takes grants from tables and applies to to views, have you tested it with grants that would only apply for tables like an insert grant? I think SQL will complain if you try and grant something like that to a view. |
SQL Server natively allows you to grant INSERT, UPDATE, and DELETE permissions on views, just as you would on a table. The GRANT statement itself will execute perfectly fine. The action of actually using it probably not if the view isn't writable. |
This commit addresses the following: - Modifies the `create view` macro to use `create or alter view` when the view already exists, preventing errors during re-materialization. - Updates the view materialization logic to correctly avoid renaming if no table exists. - Adds test cases to verify that materializing an existing view updates its definition correctly, including column changes, previous grants and avoids table changes if view fails.
Add get_view_definition_sql metadata macro for SQL Server views Detect unchanged view definition and skip rebuild by using a no-op statement Add test ensuring rerunning an unchanged view does not alter modify_date
…-to-view swap and no other grants pass on table swap
axellpadilla
force-pushed
the
view_materialization_improvement
branch
from
1b5b22d to
19d344b
Compare
|
@Benjamin-Knight hardened the concept, it only passes waranted compatible grants, should be ready to merge after v1.9 branched |
2 participants
This pr addresses the following: