-
Notifications
You must be signed in to change notification settings - Fork 144
fix: Alter destination table along with mv on schema changes #534
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
base: main
Are you sure you want to change the base?
Conversation
Corporate CLA signed. |
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.
💡 To request another review, post a new comment with "/windsurf-review".
{%- set on_schema_change = incremental_validate_on_schema_change(config.get('on_schema_change'), default='ignore') -%} | ||
{{ log('on_schema_change strategy for destination table of MV: ' + on_schema_change, info=True) }} | ||
{%- if on_schema_change != 'ignore' -%} | ||
{%- set column_changes = adapter.check_incremental_schema_changes(on_schema_change, existing_relation, sql) -%} | ||
{% if column_changes %} | ||
{% do clickhouse__apply_column_changes(column_changes, existing_relation) %} | ||
{% set existing_relation = load_cached_relation(this) %} | ||
{% endif %} | ||
{%- endif %} |
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.
The code should check if existing_relation
exists before attempting to apply schema changes. Without this check, the code might try to apply schema changes to a non-existent relation.
{%- set on_schema_change = incremental_validate_on_schema_change(config.get('on_schema_change'), default='ignore') -%} | |
{{ log('on_schema_change strategy for destination table of MV: ' + on_schema_change, info=True) }} | |
{%- if on_schema_change != 'ignore' -%} | |
{%- set column_changes = adapter.check_incremental_schema_changes(on_schema_change, existing_relation, sql) -%} | |
{% if column_changes %} | |
{% do clickhouse__apply_column_changes(column_changes, existing_relation) %} | |
{% set existing_relation = load_cached_relation(this) %} | |
{% endif %} | |
{%- endif %} | |
{%- set on_schema_change = incremental_validate_on_schema_change(config.get('on_schema_change'), default='ignore') -%} | |
{{ log('on_schema_change strategy for destination table of MV: ' + on_schema_change, info=True) }} | |
{%- if existing_relation is not none and on_schema_change != 'ignore' -%} | |
{%- set column_changes = adapter.check_incremental_schema_changes(on_schema_change, existing_relation, sql) -%} | |
{% if column_changes %} | |
{% do clickhouse__apply_column_changes(column_changes, existing_relation) %} | |
{% set existing_relation = load_cached_relation(this) %} | |
{% endif %} | |
{%- endif %} |
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.
The default behavior in case of on_schema_change
non existent is to ignore destination table change and just update the MV (current implementation)
@koletzilla Please take a look at this while I add/update tests for multiple MV and RMV |
CCLA has been signed by Twilio. Is there somebody who can validate it? |
I validated, all ok. |
Summary
Alter destination table along with materialized views on update.
Currently when we update a model
materialized='materialized_view'
it only updates underlying materialized view but not the destination table it is attached to. This issue is also brought up before.This PR provides the ability to update the destination table along with Materialized view. It uses on_schema_change
Checklist