Fix: Spark-compatible tmp_table_suffix for incremental materializations #759
+5
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generate Spark-compatible tmp_table_suffix for incremental materializations with iceberg
Generate a unique tmp table suffix if required for incremental materializations. Generate Spark-compatible tmp_table_suffix for incremental materializations.
1 task
nicor88
reviewed
Nov 22, 2024
| {% 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 %} |
There was a problem hiding this comment.
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.
nicor88
reviewed
Nov 22, 2024
| -- Generate a unique tmp table suffix if required | ||
| {% if unique_tmp_table_suffix == True %} | ||
| {% set raw_suffix = adapter.generate_unique_temporary_table_suffix() %} | ||
| {% set tmp_table_suffix = raw_suffix.replace('-', '_') %} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
#754 was merged, therefore please remove this, as not needed anymore
|
#754 was merged, please readapt this PR based on my comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses the issue related to unique_tmp_table_suffix when working with Spark. While the proposed changes in issue #668 worked for part of the scenarios, they did not account for Spark’s restrictions on table and view names. Specifically:
• Spark does not allow hyphens (-) in table/view names, causing failures when creating temporary tables.
• This PR modifies the logic to replace hyphens in the generated suffix with underscores (_), ensuring Spark compatibility.
Additionally:
• Ensures unique_tmp_table_suffix is respected and functional when set to True for all table types and strategies.
• Supports concurrent runs of the same model by avoiding naming conflicts.
Models used to test - Optional
Checklist