feat: Allow different schema for tmp tables created during table materialization #664
Conversation
pierrebzl
changed the title
|
@pierrebzl is this ready for review? If so we can move it from the draft state, and I can start to review it. |
Yes, sorry I was away for the past 2 weeks. |
pierrebzl
requested review from
jessedobbelaere,
Jrmyy,
mattiamatrix,
nicor88 and
svdimchenko
as code owners
svdimchenko
added
the
enable-functional-tests
|
functional test seems to be failed, pls take a look there |
| {%- set old_relation = adapter.get_relation(database=database, schema=schema, identifier=identifier) -%} | ||
| {%- set old_tmp_relation = adapter.get_relation(identifier=identifier ~ '__ha', | ||
| schema=schema, | ||
| database=database) -%} | ||
| {%- if temp_schema is not none and old_tmp_relation is not none-%} |
There was a problem hiding this comment.
I don't fully get this check, why do you need to include a check for old_tmp_relation not being none here? If old_tmp_relation is none, we still want to create the new tmp table in the tmp schema.
| @@ -36,6 +36,14 @@ | |||
| {%- endcall %} | |||
| {%- endmacro %} | |||
|
|
|||
| {% macro set_table_relation_schema(relation, schema) %} | |||
There was a problem hiding this comment.
could you add some comments to this method?
| {% macro set_table_relation_schema(relation, schema) %} | ||
| {%- if temp_schema is not none -%} | ||
| {%- set relation = relation.incorporate(path={"schema": schema}) -%} | ||
| {%- do create_schema(relation) -%} |
There was a problem hiding this comment.
I'm not 100% convinced by this. pretty_much create_schema will be called by every model where we setup the temp_schema. It will be good too find a a better way to handle that.
| @@ -36,6 +36,14 @@ | |||
| {%- endcall %} | |||
| {%- endmacro %} | |||
|
|
|||
| {% macro set_table_relation_schema(relation, schema) %} | |||
| {%- if temp_schema is not none -%} | |||
There was a problem hiding this comment.
| {%- if temp_schema is not none -%} | |
| {%- if schema is not none -%} |
or could you explain how do you get temp_schema here ?
There was a problem hiding this comment.
First of of all thanks for the contribution, I left some comments, please have a look.
Also, I would like to know if this feature works also for hive tables with ha=true, maybe consider to add a test for those, and be sure to include them in the implementation.
Description
As described in #662, this intend to extend
temp_schemaoption table materialization that requires to create transient temporary tables.After this PR, with
temp_schemaset on all your models that use table maaterialization, you should not see any tmp tables created inside model target schemas.Models used to test - Optional
Checklist