[CT-2819] default__alter_relation_add_remove_columns macro does not use quoting with case sensitive Snowflake relation #256

McKnight-42 · 2024-07-01T22:14:51Z

resolves #250
docs dbt-labs/docs.getdbt.com/#

Problem

Try to fix issue around case sensitive column quoting in macro default__alter_relation_add_remove_columns

Solution

update the dbt-adapters default implementation test against postgres and other adapted as needed to maintain compaitiablity, update dbt-snowflake implementation of macro

create test in dbt-tests-adapters and inherit in adapters as needed.

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

github-actions · 2024-07-01T22:15:06Z

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 contributing guide.

McKnight-42 · 2024-07-02T21:19:26Z

NOTES: usage across adapters

Adapters that use own implementation of alter_relation_add_remove_columns (defined in dot-adapters)
- Dbt-snowflake
- Dbt-redshift
- Dbt-spark

Adapters that would use default if at all (not referenced in either adapter directly)
- Dbt-postgres
- Dbt-bigquery

…mcknight/ct-2819

… (course of original issue)

…mcknight/ct-2819

…class

dbt-tests-adapter/dbt/tests/adapter/incremental/fixtures.py

mikealfare · 2024-07-16T20:50:16Z

dbt/include/global_project/macros/adapters/columns.sql

@@ -123,11 +123,11 @@
     alter {{ relation.type }} {{ relation.render() }}

            {% for column in add_columns %}
-               add column {{ column.name }} {{ column.data_type }}{{ ',' if not loop.last }}
+               add column {{ adapter.quote(column.name) }} {{ column.data_type }}{{ ',' if not loop.last }}


Do we always want to quote? Or is there a config that we need to condition on, similar to the quote policy for relation names?

I think adapter.quote leverages the same quote policy as relation names

@mikealfare are you thinking something like this (for relations) or this (for columns)?

In this particular case, I think we need to match the quoting from here (which would mean always quoting via adapter.quote()). I forget the exact details -- and I could be wrong! -- but there's some notes under the "Root Cause" toggle in #250 (comment).

Side note: here are the primary resources I've been using when researching quoting or case-sensitive issues:

Quoting for database catalogs, schemas, tables, and columns docs.getdbt.com#3518

Rationalize quoting configs + properties dbt-core#2986

@dbeatty10 oh these are really cool.

@colin-rogers-dbt adapter.quote() will always quote.

@dbeatty10 If the objective is to always quote the column in this case, that's fine. I just know we've run into issues before where we quote something that doesn't get quoted, or vice versa, and then it fails a condition in a filter where it's supposed to pass. I haven't seen it with columns, so maybe it doesn't apply here.

mikealfare · 2024-07-16T20:51:47Z

dbt-tests-adapter/dbt/tests/adapter/incremental/test_incremental_on_schema_change.py

+        assert "Job" in logs
+
+
+class TestIncrementalCaseSenstivityOnSchemaChange(BaseIncrementalCaseSenstivityOnSchemaChange):


We don't need this class. The reason TestIncrementalOnSchemaChange is there is to preserve backwards compatibility. These tests used to run when testing dbt-postgres. Now that that's in its own repo and explicitly implements tests from dbt-tests-adapter, we no longer need to create these classes here.

…mcknight/ct-2819

dbeatty10 · 2024-07-31T17:49:01Z

dbt-tests-adapter/dbt/tests/adapter/incremental/fixtures.py

+
+    -- 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, 'john' as Name, 'engineer' as "Job"


I wonder if using adapter.quote and/or string_literal would enable this test to work across more adapters without modification?

Suggested change

select {{ dbt.current_timestamp() }} as inserted_at, 'john' as Name, 'engineer' as "Job"

select {{ dbt.current_timestamp() }} as inserted_at, {{ dbt.string_literal("john") }} as Name, {{ dbt.string_literal("engineer") }} as {{ adapter.quote("Job") }}

dbeatty10 · 2024-07-31T17:50:59Z

dbt-tests-adapter/dbt/tests/adapter/incremental/fixtures.py

@@ -303,3 +303,34 @@

 from source_data
 """
+
+_MODELS__SRC_ARTISTS = """


Nit: The content of the model looks like "jobs" now instead of "artists". So ideally they'd match instead of being different.

dbeatty10 · 2024-07-31T17:56:00Z

dbt-tests-adapter/dbt/tests/adapter/incremental/test_incremental_on_schema_change.py

+    def test_run_incremental_check_quoting_on_new_columns(self, project):
+        select = "src_artists dim_artists"
+        run_dbt(["run", "--models", select, "--full-refresh"])
+        run_dbt(["show", "--inline", "select * from {{ ref('dim_artists') }}"])
+        res, logs = run_dbt_and_capture(
+            ["show", "--inline", "select * from {{ ref('dim_artists') }}"]
+        )
+        assert "Job" not in logs
+        run_dbt(["run", "--vars", "{'version': 1}"])
+        run_dbt(["show", "--inline", "select * from {{ ref('dim_artists') }}"])
+        res, logs = run_dbt_and_capture(
+            ["show", "--inline", "select * from {{ ref('dim_artists') }}"]
+        )
+        assert "Job" in logs


The run_dbt(["show" ... portions probably aren't necessary. They were just so people could see the state of the data within this reprex: #250 (comment)

Instead of the assertions here, you might be able to just rely upon expect_pass=True as the default for run_dbt and run_dbt_and_capture.

From the original bug report in #250, my understanding was that it actually produced an error.

…mcknight/ct-2819

…ther adapters

…apters into mcknight/ct-2819

…ther adapters

…mcknight/ct-2819

…lumn already exists

cboethigtrellance · 2024-12-02T19:43:24Z

Anything else required or need help with to get this over the line?

initial add of test and update to columns.sql

68bfa34

McKnight-42 self-assigned this Jul 1, 2024

cla-bot bot added the cla:yes label Jul 1, 2024

McKnight-42 added 6 commits July 3, 2024 16:19

update test to try and check against postgres

d6ad15c

minor update to tests and push up

3c922d4

Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …

8d8bad6

…mcknight/ct-2819

revert macro update for testing

b221b09

readd adapter.quote

2765bf0

update fixtures and tests (remove breakpoints) test against snowflake…

de1893a

… (course of original issue)

McKnight-42 requested review from colin-rogers-dbt and dbeatty10 July 9, 2024 19:40

McKnight-42 added 4 commits July 9, 2024 14:54

pre-commit fixes and changelog

7fdf639

Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …

ade5d98

…mcknight/ct-2819

pre-commit fix

0cb0583

pull new test out of old OnSchemaChange, to create own bass and test …

52cb472

…class

mikealfare reviewed Jul 16, 2024

View reviewed changes

McKnight-42 and others added 3 commits July 17, 2024 09:56

remove uneeded test class

f70650b

Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …

098fcf5

…mcknight/ct-2819

Merge branch 'main' into mcknight/ct-2819

50fb497

dbeatty10 reviewed Jul 31, 2024

View reviewed changes

colin-rogers-dbt mentioned this pull request Jul 31, 2024

[Bug] Appending new columns to an existing incremental does not respect the quote column property. dbt-labs/dbt-snowflake#1140

Closed

2 tasks

McKnight-42 added 7 commits July 31, 2024 14:28

Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …

5bc06b6

…mcknight/ct-2819

updating test and model names based on feedback, pushing to test in o…

29bd5ae

…ther adapters

Merge branch 'mcknight/ct-2819' of https://github.com/dbt-labs/dbt-ad…

7868857

…apters into mcknight/ct-2819

updating test and model names based on feedback, pushing to test in o…

1d83c86

…ther adapters

update query to use adapter.quote

13f3f35

Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …

253686a

…mcknight/ct-2819

modify fixture to take into account databases that don't append if co…

ed01f64

…lumn already exists

McKnight-42 added 5 commits August 2, 2024 14:24

swap double-quotes for single-quotes

35710fe

update run_dbt_and_capture command

ecacae7

update run_dbt_and_capture command

27d3cab

swap command flag

002bba5

minor reformat

8670ca2

McKnight-42 removed their assignment Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CT-2819] default__alter_relation_add_remove_columns macro does not use quoting with case sensitive Snowflake relation #256

[CT-2819] default__alter_relation_add_remove_columns macro does not use quoting with case sensitive Snowflake relation #256

McKnight-42 commented Jul 1, 2024

github-actions bot commented Jul 1, 2024

McKnight-42 commented Jul 2, 2024 •

edited

Loading

mikealfare Jul 16, 2024

colin-rogers-dbt Jul 16, 2024

dbeatty10 Jul 16, 2024

dbeatty10 Jul 16, 2024

McKnight-42 Jul 16, 2024 •

edited

Loading

mikealfare Jul 17, 2024

mikealfare Jul 16, 2024

dbeatty10 Jul 31, 2024

dbeatty10 Jul 31, 2024

dbeatty10 Jul 31, 2024

cboethigtrellance commented Dec 2, 2024

		assert "Job" in logs


		class TestIncrementalCaseSenstivityOnSchemaChange(BaseIncrementalCaseSenstivityOnSchemaChange):

	select {{ dbt.current_timestamp() }} as inserted_at, 'john' as Name, 'engineer' as "Job"
	select {{ dbt.current_timestamp() }} as inserted_at, {{ dbt.string_literal("john") }} as Name, {{ dbt.string_literal("engineer") }} as {{ adapter.quote("Job") }}

[CT-2819] default__alter_relation_add_remove_columns macro does not use quoting with case sensitive Snowflake relation #256

Are you sure you want to change the base?

[CT-2819] default__alter_relation_add_remove_columns macro does not use quoting with case sensitive Snowflake relation #256

Conversation

McKnight-42 commented Jul 1, 2024

Problem

Solution

Checklist

github-actions bot commented Jul 1, 2024

McKnight-42 commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McKnight-42 Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cboethigtrellance commented Dec 2, 2024

McKnight-42 commented Jul 2, 2024 •

edited

Loading

McKnight-42 Jul 16, 2024 •

edited

Loading