-
Notifications
You must be signed in to change notification settings - Fork 128
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
Views V2 #954
base: 1.10.latest
Are you sure you want to change the base?
Views V2 #954
Conversation
@@ -138,7 +139,9 @@ class DatabricksConfig(AdapterConfig): | |||
target_alias: Optional[str] = None | |||
source_alias: Optional[str] = None | |||
merge_with_schema_evolution: Optional[bool] = None | |||
safe_table_create: Optional[bool] = None | |||
use_safer_relation_operations: Optional[bool] = None | |||
incremental_apply_config_changes: Optional[bool] = None |
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.
This was previously missing
@@ -918,3 +923,30 @@ def _describe_relation( | |||
results["information_schema.tags"] = adapter.execute_macro("fetch_tags", kwargs=kwargs) | |||
results["show_tblproperties"] = adapter.execute_macro("fetch_tbl_properties", kwargs=kwargs) | |||
return results | |||
|
|||
|
|||
class ViewAPI(RelationAPIBase[ViewConfig]): |
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.
We need to use something similar to MV/ST and Incremental to check for changes so that we can update views via ALTER.
@@ -91,6 +91,10 @@ def is_materialized_view(self) -> bool: | |||
def is_streaming_table(self) -> bool: | |||
return self.type == DatabricksRelationType.StreamingTable | |||
|
|||
@property |
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.
this was missing from a previous PR
def get_changeset(self, existing: Self) -> Optional[DatabricksRelationChangeSet]: | ||
"""Get the changeset that must be applied to the existing relation to make it match the | ||
current state of the dbt project. If no changes are required, this method should return | ||
None. | ||
""" | ||
changes: dict[str, DatabricksComponentConfig] = {} |
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.
Defining a base since the incremental and view versions of this method are very similar
|
||
def get_changeset(self, existing: Self) -> Optional[DatabricksRelationChangeSet]: | ||
changeset = super().get_changeset(existing) | ||
if changeset and "comment" in changeset.changes: |
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.
For some reason there is no mechanism in Databricks to alter the comment on a view ;(. Therefore, even if you want me to update the view via alter, if you update the description, we'll need to replace the view.
@@ -56,7 +56,7 @@ | |||
|
|||
{% macro databricks__get_columns_in_query(select_sql) %} | |||
{{ log("Getting column information via empty query")}} | |||
{% call statement('get_columns_in_query', fetch_result=True, auto_begin=False) -%} | |||
{% call statement('get_columns_in_query', fetch_result=True) -%} |
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.
auto_begin doesn't do anything in Databricks
@@ -4,7 +4,7 @@ | |||
{% set comment = column['description'] %} | |||
{% set escaped_comment = comment | replace('\'', '\\\'') %} | |||
{% set comment_query %} | |||
alter table {{ relation.render()|lower }} change column {{ api.Column.get_name(column) }} comment '{{ escaped_comment }}'; | |||
COMMENT ON COLUMN {{ relation.render()|lower }}.{{ api.Column.get_name(column) }} IS '{{ escaped_comment }}'; |
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.
This form works with tables and views
@@ -15,3 +15,17 @@ | |||
|
|||
{%- endif -%} | |||
{%- endmacro %} | |||
|
|||
{% macro execute_multiple_statements(statements) %} |
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.
Refactoring out of MV/ST because now we have a third place that we need this pattern.
@@ -15,7 +15,7 @@ | |||
{% if adapter.behavior.use_materialization_v2 %} | |||
{{ log("USING V2 MATERIALIZATION") }} | |||
{#-- Set vars --#} | |||
{% set safe_create = config.get('safe_table_create', True) | as_bool %} | |||
{% set safe_create = config.get('use_safer_relation_operations', True) | as_bool %} |
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.
renamed this flag since now I want to use it for views too.
@@ -32,7 +32,7 @@ | |||
|
|||
-- get config options | |||
{% set on_configuration_change = config.get('on_configuration_change') %} | |||
{% set configuration_changes = get_materialized_view_configuration_changes(existing_relation, config) %} | |||
{% set configuration_changes = get_configuration_changes(existing_relation) %} |
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.
Was able to make this macro more generic too.
@@ -1,10 +1,9 @@ | |||
{%- macro create_backup(relation) -%} | |||
-- get the standard backup name | |||
{% set backup_relation = make_backup_relation(relation, relation.type) %} | |||
-- get the standard backup name |
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.
working on consistent tab size of 2
{% endmacro %} | ||
|
||
{% macro databricks__get_replace_sql(existing_relation, target_relation, sql) %} | ||
{# /* use a create or replace statement if possible */ #} | ||
{# /* if safe_relation_replace, prefer renaming */ #} |
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.
Updated this macro per replace_flow.md. Now works for views as well as MV/ST
Description
Next step in the V2 materialization saga, this time Views. The big deal with views is adding the ability to update previously deployed views with ALTER rather than replacement, so that they keep the same grants, and UC id (which is how create or replace already works for tables). The other change is that now replace.sql is doing more heavy lifting, simplifying what would otherwise be a fairly complicated materialization.
Probably need more tests, but want to get the ideas out in front of reviewers
Checklist
CHANGELOG.md
and added information about my change to the "dbt-databricks next" section.