84 iceberg format support using snowflake #97
Merged
Conversation
This file contains hidden or 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
…g RELY for join elimination
There was a problem hiding this comment.
Pull Request Overview
Adds broad support for Snowflake Iceberg tables, enhances global configuration options, fixes quoting and mixed‐case handling, and bumps version to 1.0.5.
- Trim quotes from generated constraint names across all adapters.
- Introduce
ICEBERGprefix for Snowflake DDL whenis_iceberg_formatis true and refine constraint lookup/caching. - Switch all
var(...)calls to string‐based truth checks, adddbt_constraints_always_norely, and update integration tests and documentation.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| macros/*__create_constraints.sql (all adapters) | Added ` |
| macros/snowflake__create_constraints.sql | Added ICEBERG DDL prefix, improved cache updates, semi-structured detection |
| macros/create_constraints.sql | Changed var checks to string matching, added new global vars |
| integration_tests/models/schema.yml | New not_null tests for array/object/variant columns |
| integration_tests/models/dim_part.sql | Inject semi-structured columns for Snowflake and TitleCasePartKey |
| integration_tests/macros/clone_table.sql | Conditional macro guard and log verbosity updates |
| integration_tests/dbt_project.yml | Added global constraint flags including dbt_constraints_always_norely |
| dbt_project.yml | Bumped version to 1.0.5, added new global vars |
| README.md | Documented dbt_constraints_always_norely |
Comments suppressed due to low confidence (1)
macros/create_constraints.sql:275
- The new
dbt_constraints_always_norelyflag logic isn’t covered by integration tests. Consider adding a test case to verify that RELY is skipped when this variable is true.
{%- set dbt_constraints_always_norely = var('dbt_constraints_always_norely', "false")|string|lower == "true" %}
Comment on lines
+24
to
+32
| description: "Test that we don't create not null contraints on array columns" | ||
| tests: | ||
| - not_null | ||
| - name: test_object_col | ||
| description: "Test that we don't create not null contraints on object columns" | ||
| tests: | ||
| - not_null | ||
| - name: test_variant_col | ||
| description: "Test that we don't create not null contraints on variant columns" |
There was a problem hiding this comment.
Typo in description: 'contraints' should be 'constraints'.
Suggested change
| description: "Test that we don't create not null contraints on array columns" | |
| tests: | |
| - not_null | |
| - name: test_object_col | |
| description: "Test that we don't create not null contraints on object columns" | |
| tests: | |
| - not_null | |
| - name: test_variant_col | |
| description: "Test that we don't create not null contraints on variant columns" | |
| description: "Test that we don't create not null constraints on array columns" | |
| tests: | |
| - not_null | |
| - name: test_object_col | |
| description: "Test that we don't create not null constraints on object columns" | |
| tests: | |
| - not_null | |
| - name: test_variant_col | |
| description: "Test that we don't create not null constraints on variant columns" |
| {%- for row in results.rows -%} | ||
| {%- do upper_column_list.append(row["column_name"]|upper) -%} | ||
| {%- if row['null?'] == 'false' -%} | ||
| {%- if row['null?'] == true -%} |
There was a problem hiding this comment.
Comparing row['null?'] to boolean true may fail if run_query returns the string 'true'/'false'. Consider normalizing to string comparison or casting.
Suggested change
| {%- if row['null?'] == true -%} | |
| {%- if row['null?'] == 'true' -%} |
| {%- set ddl_prefix_for_alter = 'ICEBERG' if table_relation.is_iceberg_format else '' -%} | ||
| {%- set modify_statements= [] -%} | ||
| {%- for column in columns_list -%} | ||
| {%- set modify_statements = modify_statements.append( "COLUMN " ~ column ~ " SET NOT NULL" ) -%} |
There was a problem hiding this comment.
In Jinja, append mutates the list and returns None. Assigning its return will break modify_statements. Instead, call modify_statements.append(...) without reassigning.
Suggested change
| {%- set modify_statements = modify_statements.append( "COLUMN " ~ column ~ " SET NOT NULL" ) -%} | |
| {%- do modify_statements.append( "COLUMN " ~ column ~ " SET NOT NULL" ) -%} |
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.
Adds support for Snowflake Iceberg tables
Adds dbt_constraints_always_noreply variable to globally disable using RELY for join elimination
Updates the version in the dbt_project.yml to 1.0.5
Misc integration test changes for recent minor enhancements
Fixes support for mixed case columns on all DB by selectively trimming
Adds support for text and boolean values for all variables
Fixes metadata caching for Snowflake