-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Use merge pattern for Archival queries #1478
Use merge pattern for Archival queries #1478
Conversation
aa1815c
to
e6f7284
Compare
c5707df
to
7d490d4
Compare
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 looks pretty good! Just a few comments.
@@ -584,22 +585,22 @@ def valid_archive_target(self, relation): | |||
dbt.exceptions.raise_compiler_error(msg) | |||
|
|||
@available.parse_none | |||
def expand_target_column_types(self, temp_table, to_relation): | |||
def expand_target_column_types(self, from_relation, to_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.
I like it, but this is a breaking change - we should be sure to document it.
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.
do you think we should instead
- deprecate this method
- use
expand_column_types
directly henceforth?
this method was just a weirdo adapter method that converts a string into a Relation, then calls expand_column_types
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.
Nah, I think this is fine to just document for that reason.
archive, | ||
node_path, | ||
self.all_projects.get(archive.package_name), | ||
archive_config=archive_config) | ||
|
||
# TODO : Add tests for this | ||
to_return[node_path] = set_archive_attributes(parsed_node) |
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.
did you get these tests done?
@@ -9,6 +9,6 @@ | |||
updated_at='updated_at', | |||
) | |||
}} | |||
select * from `{{database}}`.`{{schema}}`.seed | |||
select * from `{{target.database}}`.`{{schema}}`.seed |
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.
why target.database
and not database
? Or alternatively, why schema
and not target.schema
?
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.
I changed the archive parsing code to set database
and schema
from the target_database
and target_schema
configs. This meant that {{database}}
would interpolate to the target database for the archive, which can be different than the database where the seed lives. Working on a test that exercises this, will remove the extraneous changes though - most of the target.database
additions i made here aren't actually necessary
@@ -40,6 +41,8 @@ def run_select_and_check(self, rel, sql): | |||
self.query_state[rel] = 'bad' | |||
|
|||
except Exception as e: | |||
logger.info("Caught exception: {}".format(e)) |
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 is debug stuff, right? Do you want to remove it?
Archival + Merge
This PR rounds out the changes for the 0.14.0 implementation of Archival.
Archive changes:
merge
statement where supported (closes Implement archival with a "merge" statement #1339)create table as ( ... )
statementvalid_to
type in archive table (closes Standardize timestamp column types in the Archival materialization #938)ParsedNode
database
andschema
fields correctlyOther changes:
expand_target_column_types
take a relation instead of a stringpytest
configBreaking changes:
The
expand_target_column_types
adapter method now takes a relation, not a string. This is also used in the dbt-utils implementation of insert_by_period