-
Notifications
You must be signed in to change notification settings - Fork 73
Added custom materialization for materialized view #198
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| kind: Features | ||
| body: Support Materialized View materialization | ||
| time: 2022-12-15T11:20:09.053826Z | ||
| custom: | ||
| Author: Jay-code0 | ||
| PR: "198" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| {% materialization materialized_view, adapter="trino" %} | ||
| {%- set target_relation = this %} | ||
| {%- set existing_relation = load_relation(this) -%} | ||
| {%- set config_grace_period = config.require('grace_period') %} | ||
| {%- set config_max_import_duration = config.require('max_import_duration') %} | ||
| {%- set config_drop_any_existing = config.require('drop_any_existing') %} | ||
| {%- set config_cron = config.get('cron') %} | ||
| {%- set config_refresh_interval = config.get('refresh_interval') %} | ||
|
|
||
|
|
||
| {{ run_hooks(pre_hooks) }} | ||
|
|
||
| {% if existing_relation is none %} | ||
|
|
||
| {{ log("No existing materialized view found, creating materialized view...", info=true) }} | ||
| {%- set build_sql = build_materialized_view(target_relation, config_grace_period, config_max_import_duration, config_cron, config_refresh_interval) %} | ||
|
|
||
| {% else %} | ||
| {%- if existing_relation.is_view %} | ||
| {%-set existing_type = 'view'%} | ||
| {%- elif existing_relation.is_table %} | ||
| {%-set existing_type = 'table'%} | ||
| {%- elif existing_relation.is_cte %} | ||
| {%-set existing_type = 'CTE'%} | ||
| {% endif %} | ||
|
|
||
| {%- if existing_type is not none and config_drop_any_existing == 'False' %} | ||
| {{ log("Found a " ~ existing_type ~ " with same name.", info=true) }} | ||
| {%- set build_sql = build_materialized_view(target_relation, config_grace_period, config_max_import_duration, config_cron, config_refresh_interval) %} | ||
|
|
||
| {% elif existing_type is not none and config_drop_any_existing == 'True' and existing_type != 'table'%} -- theres an issue with starburst thinking that the materialised view is a table in the information schema, hence the logic on this line | ||
| {%- set drop_existing_sql = "DROP " ~ existing_type ~ " IF EXISTS " ~ existing_relation %} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes me wonder if this should not be more generally applied. For example a MV can be replaced by a table.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is something i need to look into further at some point, theres an issue where it thinks an existing mat view is a table coming from starburst, hence its excluding table on this line, so causes an error due to recieving the wrong type, the error flow looks like this
So this currently ignores table types and will display an error in console informing user a table already exists and they need to manually handle it (drop, move etc..), otherwise itll run fine if its a materialized view incorrectly typed, due to the create or replace part of the SQL statement further on
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are all good cases that should be integration tested.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See following PR trinodb/trino#15350 where support in system table tables will be improved with the type.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| {%- set build_sql = build_materialized_view(target_relation, config_grace_period, config_max_import_duration, config_cron, config_refresh_interval) %} | ||
| {%- call statement('drop_existing') -%} | ||
| {{ drop_existing_sql }} | ||
| {% endcall %} | ||
|
|
||
| {% else %} | ||
| {{ log("Found something with same name, trying to create materialized view anyway...", info=true) }} | ||
| {%- set build_sql = build_materialized_view(target_relation, config_grace_period, config_max_import_duration, config_cron, config_refresh_interval) %} | ||
| {% endif %} | ||
|
|
||
| {% endif %} | ||
|
|
||
| -- Sends the sql command to starburst | ||
| {%- call statement('main') -%} | ||
| {{ build_sql }} | ||
| {% endcall %} | ||
|
|
||
| {{ run_hooks(pre_hooks) }} | ||
|
|
||
| {{ return({'relations': [target_relation]}) }} | ||
|
|
||
| {% endmaterialization %} | ||
|
|
||
| ---------------- Macros ------------------- | ||
|
|
||
| {%- macro build_materialized_view(target_relation, config_grace_period, config_max_import_duration, config_cron, config_refresh_interval) -%} | ||
| {% if config_cron is not none and config_refresh_interval is not none %} | ||
| {{ log("Found config for CRON and Refresh Interval, error will be thrown as only 1 is allowed", info=true) }} | ||
| {{ exceptions.raise_compiler_error("Invalid config: only CRON or refresh interval allowed - " ~ config_cron ~ " - " ~ config_refresh_interval) }} | ||
|
|
||
| {% else %} | ||
| {% if config_cron is not none and config_refresh_interval is none %} | ||
| {%- set schedule_to_use ="cron = \'" ~ config_cron ~ "\'," %} | ||
| {% else %} | ||
| {%- set schedule_to_use ="refresh_interval = \'" ~ config_refresh_interval ~ "\'," %} | ||
| {% endif %} | ||
|
|
||
| {%- set sqlcode= "CREATE OR REPLACE MATERIALIZED VIEW " ~ target_relation ~ " WITH ( | ||
| " ~ schedule_to_use ~ | ||
| "grace_period = \'" ~ config_grace_period ~ "\', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the single quotes are escaped? I would think it is not required.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its so they land correct in starburst, with the escape it looks like this without the single quotes it'll produce something like Ive chose this format due to it working consistently and noticed issues with different values when the code is run manually on starburst, for example a value of 5.33h, so thought its safer keeping the '' in every query sent
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My point is within a double quoted string, I don't think it's needed to escape single quotes. We should definitely include the single quotes based on the type of the property. |
||
| max_import_duration = \'" ~ config_max_import_duration ~ "\' | ||
| ) AS" ~ sql | ||
| %} | ||
|
|
||
| {{sqlcode}} | ||
|
|
||
| {% endif %} | ||
| {%- endmacro -%} | ||
|
|
||
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.
config flags should use boolean type and not strings.