-
Notifications
You must be signed in to change notification settings - Fork 122
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
Support for setting table properties as part of a model configuration #49
Conversation
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.
The best PRs have way more test code than program code, looks great! Just a couple small changes.
CHANGELOG.md
Outdated
@@ -1,5 +1,8 @@ | |||
## dbt-databricks 1.0.2 (Release TBD) | |||
|
|||
### Features | |||
- Support to set tblproperties when creating tables ([#33](https://github.com/databricks/dbt-databricks/issues/33), [#49](https://github.com/databricks/dbt-databricks/pull/49)) |
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.
How about "Support for setting table properties as part of a model configuration"
self.assertTablesEqual("set_tblproperties", "expected") | ||
|
||
results = self.run_sql( | ||
'describe extended {schema}.{table}'.format( |
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.
nit: you can use "SHOW TBLPROPERTIES" it's shorter and doesn't need to be parsed.
{%- if tblproperties is not none %} | ||
tblproperties ( | ||
{%- for prop in tblproperties -%} | ||
{{ prop }} = '{{ tblproperties[prop] }}' {% if not loop.last %}, {% endif %} |
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.
it might be better to treat the prop as a string literal and wrap it in single quotes 'delta.appendOnly' just in case the user puts a reserved keyword in there.
{%- if tblproperties is not none %} | ||
tblproperties ( | ||
{%- for prop in tblproperties -%} | ||
{{ prop }} = '{{ tblproperties[prop] }}' {% if not loop.last %}, {% endif %} |
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.
for tableproperties[prop], technically the value can be a boolean, string, or numeric type. I think wrapping the value in single quotes is okay because it will try to be implicitly casted but we should double check.
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.
Gr8!
Thanks! merging. |
…#49) resolves #33 ### Description Supports to set `tblproperties` when creating tables and views. In the models config, set `tblproperties` as follows. ``` {{ config( materialized='incremental', tblproperties={ 'delta.autoOptimize.optimizeWrite' : 'true', 'delta.autoOptimize.autoCompact' : 'true' } ) }} ```
Hey! I discovered that though properties are applied for the table creation step they are not applied incrementally if you add the properties later. Is this expected for this change, or I had something wrong with my set up? I was thinking of creating an Issue for this. |
resolves #33
Description
Supports to set
tblproperties
when creating tables and views.In the models config, set
tblproperties
as follows.