-
Notifications
You must be signed in to change notification settings - Fork 227
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
Update adaptors.sql to use tblproperties
macro
#848
Update adaptors.sql to use tblproperties
macro
#848
Conversation
see discussion #847 |
typo in name of macro
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.
@etheleon : Thanks for the contribution! I added a suggestion to align the code with the other clauses:
@@ -69,6 +69,7 @@ | |||
{{ clustered_cols(label="clustered by") }} | |||
{{ location_clause() }} | |||
{{ comment_clause() }} | |||
{{ dbt_spark_tblproperties_clause() }} |
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.
@etheleon : Could you rename the dbt_spark_tblproperties_clause
to spark__tblproperties_clause
and create a similar pattern as for the other clauses?
{% macro tblproperties_clause() %}
{{ return(adapter.dispatch('tblproperties_clause', 'dbt')()) }}
{%- endmacro -%}
{% macro spark__tblproperties_clause() %}
...
{%- 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.
sure, I've submitted my commit
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.
@JCZuurmond if you have time for this review
* changed macro `dbt_spark_tblproperties_clause()` to `spark__tblproperties_clause()` * create new macro `tblproperties_clause()` which references the above
f017a0b
to
3bd8916
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.
LGTM! Thanks for fixing this
@Fleid: Could you merge this PR? |
@@ -69,6 +69,7 @@ | |||
{{ clustered_cols(label="clustered by") }} | |||
{{ location_clause() }} | |||
{{ comment_clause() }} | |||
{{ tblproperties_clause() }} |
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 had the same problem and fixed it in a similar way locally. My question is then: why add it here (seed.sql
) and not in adapters.sql
directly under macro spark__create_table_as
?
It seems it would be a more generic change, no?
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.
You are right, @martelli. It makes more sense to put it in the spark__create_table_as
. I assumed it was there, did not read correctly that the change was done in the `seed.sql.
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.
@JCZuurmond Ok. Since I had the fix ready, I've completed a parallel pull request. If this one gets updated and landed, I'll discard mine.
@colin-rogers-dbt can you review this? 🙇 |
tblproperties
macro
@JCZuurmond are we doing this one? |
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.
Have we tested this manually? I don't think our existing tests cover this case. At the very least we should open an issue to update our automated tests.
@etheleon can you add a changelog entry? |
hey @colin-rogers-dbt working on it 👍
output from
|
resolves #865
resolves #190
docs dbt-labs/docs.getdbt.com/#
Problem
tblproperties
clause macro is not used when creating tables. Add macro to adaptor.sqlSolution
Add macro
Checklist