Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Spark-compatible tmp_table_suffix for incremental materializations #759

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix: Spark-compatible tmp_table_suffix for incremental materializations #759

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5b2df7
Update incremental.sql
chenp-eleos Nov 21, 2024
dd2e3e6
Update incremental.sql
chenp-eleos Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions dbt-athena/src/dbt/include/athena/macros/materializations/models/incremental/incremental.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,14 @@
{% set temp_schema = config.get('temp_schema') %}
{% set target_relation = this.incorporate(type='table') %}
{% set existing_relation = load_relation(this) %}
-- If using insert_overwrite on Hive table, allow to set a unique tmp table suffix
{% if unique_tmp_table_suffix == True and strategy == 'insert_overwrite' and table_type == 'hive' %}
{% set tmp_table_suffix = adapter.generate_unique_temporary_table_suffix() %}
-- Generate a unique tmp table suffix if required
{% if unique_tmp_table_suffix == True %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove the check on strategy == 'insert_overwrite' and table_type == 'hive'?

for sure we can simplify the statement checks and do something like this:

{% if unique_tmp_table_suffix == True and ((strategy == 'insert_overwrite' and table_type == 'hive') or table_type == 'iceberg') %}

But I will avoid to don't check the strategy type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've some doubts about my comment, your idea might not be bad overall, but to guarantee that works as expected, consider to add some Integrations tests for different scenarios.

{% set raw_suffix = adapter.generate_unique_temporary_table_suffix() %}
{% set tmp_table_suffix = raw_suffix.replace('-', '_') %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this inside the generate_unique_temporary_table_suffix.
replacing - with _ make sense also for an athena/trino SQL prospective.

Also, remember to adapt the functional tests, otherwise they will fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#754 was merged, therefore please remove this, as not needed anymore

{% else %}
{% set tmp_table_suffix = '__dbt_tmp' %}
{% endif %}

{% if unique_tmp_table_suffix == True and table_type == 'iceberg' %}
{% set tmp_table_suffix = adapter.generate_unique_temporary_table_suffix() %}
{% endif %}


{% set old_tmp_relation = adapter.get_relation(identifier=target_relation.identifier ~ tmp_table_suffix,
schema=schema,
database=database) %}
Expand Down
Loading