fix: implicit view schema changes with alter view approach#1358
fix: implicit view schema changes with alter view approach#1358canbekley wants to merge 2 commits into
Conversation
|
Thanks for the PR. Will look into it. |
tejassp-db
left a comment
tejassp-db
left a comment
There was a problem hiding this comment.
It looks like there are two different fixes - column comments and view refresh, in this PR. Can you please split it into two separate PRs?
| {% if column_comments %} | ||
| {{ alter_column_comments(target_relation, column_comments.comments) }} | ||
| {% if config.persist_column_docs() and model.columns %} | ||
| {#-- ALTER VIEW AS <query> will wipe all column comments in a column_list, so we need to reapply them here. --#} |
There was a problem hiding this comment.
Can you explain this comment please?
There was a problem hiding this comment.
that's the issue described in #1357. basically every ALTER VIEW AS command that changes the view defintion will currently (as of dbr 18.1) drop all existing column comments, without adding them back. this issue already existed, but #1356 makes #1357 strictly worse, because we would run ALTER VIEW AS on every invocation.
good point, I should definitely split this in two separate PRs. there is going to be an order dependency though, as we would need to merge the column comment fix before the bahavioural changes that enforce |
canbekley
force-pushed
the
fix/alter-view-not-applied-on-implicit-changes
branch
from
d485235 to
1dca899
Compare
canbekley
changed the title
|
hi @tejassp-db , this PRs is now split into two dependent PRs: column comment fix in #1444 (which would need to be merged first) and the view schema refresh fix in the PR. I also updated the descriptions accordingly. could you please have another look? |
Signed-off-by: Can Bekleyici <can.bekleyici@deepl.com>
Signed-off-by: Can Bekleyici <can.bekleyici@deepl.com>
canbekley
force-pushed
the
fix/alter-view-not-applied-on-implicit-changes
branch
from
1dca899 to
d35b3d4
Compare
Resolves #1357 Split out from #1358 per maintainer request to land independently of the #1356 behavior change. ### Description When `view_update_via_alter` is enabled and the alter path issues `ALTER VIEW <view> AS <query>`, Databricks silently drops every persisted column comment. The existing `if column_comments` block in `databricks__alter_view` doesn't catch this because the column-comment diff is computed against the pre-alter state — when model and existing comments match, the diff is empty, so nothing reapplies them after `ALTER VIEW AS` wipes them. Fix: inside the `query` branch of `databricks__alter_view`, reapply persisted column comments after `alter_query`, gated on `config.persist_column_docs()` and `model.columns`. The pattern mirrors `databricks__persist_docs` (`dbt/include/databricks/macros/adapters/persist_docs.sql`). The existing `if column_comments` block is kept — it still handles the disjoint case where only a column description changed in `schema.yml` (no query change, so the new reapply path doesn't run). This fix becomes load-bearing once #1356 lands and `ALTER VIEW AS` runs on every invocation, but the bug exists today on any query change and is worth fixing standalone. ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests, or tests are not required/relevant for this PR - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt-databricks next" section. Signed-off-by: Can Bekleyici <can.bekleyici@deepl.com> Co-authored-by: Shubham Dhal <shubham.dhal@databricks.com>
2 participants
Resolves #1356
Note
Stacked on #1444 (resolves #1357), per maintainer request to split this work. please merge that PR first
Description
Views materialized with
view_update_via_alter(v2 materialization) were skippingALTER VIEW ASwhenever dbt's query diff reported no change, so changes in underlying tables (e.g. a new column appearing in aselect *source) never propagated to the view.Fix: force
ALTER VIEW AS <query>on every invocation by replacingQueryProcessorwith aViewQueryProcessorwhoseget_diffalways returns self. Drop the no-op branch in the view materialization (always alter, never skip).With #1357's reapply-after-
alter_queryblock now in place, the existingColumnCommentsProcessoris also removed fromViewConfigevery alter-view run wipes column comments and reapplies them unconditionally, so a separate diff path is no longer needed.Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.