Prefer DEFAULT to NULL when inserting into, actually validate append #607
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
benc-db
requested review from
andrefurlan-db and
rcypher-databricks
as code owners
nimeshpatni
reviewed
Mar 21, 2024
| @@ -60,7 +60,7 @@ | |||
| {%- if dest_col in source_columns -%} | |||
| {%- do common_columns.append(dest_col) -%} | |||
| {%- else -%} | |||
| {%- do common_columns.append('NULL') -%} | |||
| {%- do common_columns.append('DEFAULT') -%} | |||
There was a problem hiding this comment.
Hi,
This seems to be a breaking change, "NULL" (String value) or "DEFAULT" (String value) won't solve the purpose. For all the unmatched columns, we will end up having multiple "DEFAULT" (String) values as columns that can raise conflict/ambiguous column error.
Default value works when we don't pass the column in the argument of insert statement. Could you please try this simple resolution:
{%- set common_columns_csv = set(source_columns).intersection(dest_columns) | sort | join(', ') -%}
There was a problem hiding this comment.
They're not inserted as strings, but as literals.
Example from test run:
insert into table `peco`.`test17110372962014884605_test_incremental_on_schema_change`.`incremental_append_new_columns_remove_one` (`id`, `field1`, `field2`, `field3`, `field4`)
select `id`, `field1`, DEFAULT, `field3`, `field4` from `incremental_append_new_columns_remove_one__dbt_tmp`
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
A github user pointed out that technically using NULL could lead to different behavior than *, where DEFAULT would be honored for missing columns, so I have adjusted
insert intoto use DEFAULT instead. Also, noticed that at some point I lost my config that made the append tests actually test append, so added that back. The second half of the 'append columns' test for the append strategy validates that DEFAULT is used and does not break.Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.