-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fix print_dbt_project_evaluator_issues
for BigQuery
#503
base: main
Are you sure you want to change the base?
Conversation
Fix table reference for BigQuery compatibility
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.
Thanks! I added a comment to remove some of the variables no longer needed when getting the relation from the graph
object.
@@ -33,7 +33,7 @@ | |||
{% set db_schema = model_database ~ "." ~ model_schema if model_database else model_schema %} | |||
|
|||
{% set sql_statement %} | |||
select * from {{db_schema}}.{{name_model_checked}} | |||
select * from {{ model_details['relation_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.
I think that we can use the .
notation here and remove the variables from above as they are no longer needed.
select * from {{ model_details['relation_name'] }} | |
select * from {{ model_details.relation_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.
Yes, you're absolutely right. Fixed that.
…ot-notation Remove unused vars and use dot notation
This is a:
Link to Issue
Closes #502.
Description & motivation
The
print_dbt_project_evaluator_issues
macro doesn't reference the resource correctly on BigQuery. Specifically, it misses the backticks inselect ... from
project.dataset.table``. This PR fixes it.Unsure how to test this without re-working the macro quite substantially. AFAICT, no existing tests either. Open to discussion.
Integration Test Screenshot
See attached logs:
Checklist