-
Notifications
You must be signed in to change notification settings - Fork 153
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
[CT-2819] default__alter_relation_add_remove_columns macro does not use quoting with case sensitive Snowflake relation #1277
base: main
Are you sure you want to change the base?
Conversation
…for case sensitive columns
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the dbt-bigquery contributing guide. |
since bigquery naturally accepts case sensitiviy without needed quoting I made a test for that. use cases I've seen that would require a column to be quoted are if a space or special characters are needed but trying to add those thus far has lead to failure for string literals in the model. suggestions? |
"dim_artists.sql": _MODELS__DIM_ARTISTS, | ||
} | ||
|
||
def test_incremental_case_sensitivity(self, project): |
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.
I could be missing it, but this looks like the same test as in the base. Can we inherit from the new test case and just overwrite models
? Also, this should just be a test class. We don't need the base class and the test class since this is in the concrete adapter.
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.
I had pulled the dbt-adapter ones into its own base case and test class so that the location I had them originally as part of BaseIncrementalOnSchemaChangeSetup
but didn't want to always pull it in since bigquery requires its own default version
{% else %} | ||
-- add a non-zero version to the end of the command to get a different version: | ||
-- --vars "{'version': 1}" | ||
select {{ dbt.current_timestamp() }} as inserted_at, 'engineer' as Job, |
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.
Is Name
purposely skipped here? I noticed you had it in the dbt-tests-adapter
version.
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.
yeah for bigquery when ran it seemed to count the column as already existing and would fail. removing and running seems fine even if I changed the casing of name to Name it still failed. for column already existing happy to look into it further.
my thought was that since bigquery checks column casing natively it sees them as the same thing so nothing new to add and that seems to borq the query
dbt-labs/dbt-adapters#250
docs dbt-labs/docs.getdbt.com/#
Problem
user raised issue around "While working with DBT incremental config: on_schema_change='append_new_columns'
The append new columns flag is not able to capture the correct case-sensitive column name and add it to the incremental table causing the run to fail."
they stated they were using snowflake.
Solution
add a new test to dbt-adapters-tests to check that column quoting case sensitivity is expressed correctly, update all macros in adapters as needed if they do not use the default implementation and test default implementing macros to see if we need to update the dbt-adapters macro as well.
Todo:
Checklist