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.

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

Closed
wants to merge 5 commits into from

Conversation

McKnight-42
Copy link
Contributor

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:

  • test with rest of adapters (see if dbt-adapter default version of macro needs to be changed or any other adapter override version of macro).
  • make sure new tests works in all adapters as needed
  • update requirements files either dev-requirements or project.toml files before merger

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@McKnight-42 McKnight-42 self-assigned this Jul 16, 2024
@cla-bot cla-bot bot added the cla:yes label Jul 16, 2024
Copy link
Contributor

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.

@McKnight-42
Copy link
Contributor Author

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):
Copy link
Contributor

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.

Copy link
Contributor Author

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,
Copy link
Contributor

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.

Copy link
Contributor Author

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

dev-requirements.txt Outdated Show resolved Hide resolved
@McKnight-42 McKnight-42 changed the title create bigquery version of test as it has different default behavior … [CT-2819] default__alter_relation_add_remove_columns macro does not use quoting with case sensitive Snowflake relation Aug 5, 2024
@McKnight-42 McKnight-42 removed their assignment Aug 9, 2024
@mikealfare mikealfare closed this Nov 9, 2024
@mikealfare mikealfare deleted the mcknight/2819 branch November 9, 2024 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants