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.

Sign up for GitHub

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

Jump to bottom

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

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
68bfa34
initial add of test and update to columns.sql
McKnight-42 Jul 1, 2024
d6ad15c
update test to try and check against postgres
McKnight-42 Jul 3, 2024
3c922d4
minor update to tests and push up
McKnight-42 Jul 8, 2024
8d8bad6
Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …
McKnight-42 Jul 8, 2024
b221b09
revert macro update for testing
McKnight-42 Jul 9, 2024
2765bf0
readd adapter.quote
McKnight-42 Jul 9, 2024
de1893a
update fixtures and tests (remove breakpoints) test against snowflake…
McKnight-42 Jul 9, 2024
7fdf639
pre-commit fixes and changelog
McKnight-42 Jul 9, 2024
ade5d98
Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …
McKnight-42 Jul 11, 2024
0cb0583
pre-commit fix
McKnight-42 Jul 11, 2024
52cb472
pull new test out of old OnSchemaChange, to create own bass and test …
McKnight-42 Jul 16, 2024
f70650b
remove uneeded test class
McKnight-42 Jul 17, 2024
098fcf5
Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …
McKnight-42 Jul 17, 2024
50fb497
Merge branch 'main' into mcknight/ct-2819
McKnight-42 Jul 31, 2024
5bc06b6
Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …
McKnight-42 Jul 31, 2024
29bd5ae
updating test and model names based on feedback, pushing to test in o…
McKnight-42 Jul 31, 2024
7868857
Merge branch 'mcknight/ct-2819' of https://github.com/dbt-labs/dbt-ad…
McKnight-42 Jul 31, 2024
1d83c86
updating test and model names based on feedback, pushing to test in o…
McKnight-42 Jul 31, 2024
13f3f35
update query to use adapter.quote
McKnight-42 Aug 1, 2024
253686a
Merge branch 'main' of https://github.com/dbt-labs/dbt-adapters into …
McKnight-42 Aug 1, 2024
ed01f64
modify fixture to take into account databases that don't append if co…
McKnight-42 Aug 1, 2024
35710fe
swap double-quotes for single-quotes
McKnight-42 Aug 2, 2024
ecacae7
update run_dbt_and_capture command
McKnight-42 Aug 5, 2024
27d3cab
update run_dbt_and_capture command
McKnight-42 Aug 5, 2024
002bba5
swap command flag
McKnight-42 Aug 5, 2024
8670ca2
minor reformat
McKnight-42 Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20240709-145439.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: add new test to test case sensitivity for column quoting in incremental models that use append_new_columns
time: 2024-07-09T14:54:39.526077-05:00
custom:
Author: McKnight-42
Issue: "250"
31 changes: 31 additions & 0 deletions dbt-tests-adapter/dbt/tests/adapter/incremental/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,3 +303,34 @@

from source_data
"""

_MODELS__SRC_ARTISTS = """
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{{
config(
materialized='table',
)
}}

{% if var("version", 0) == 0 %}

select {{ dbt.current_timestamp() }} as inserted_at, 'john' as name

{% 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, 'john' as Name, 'engineer' as "Job"
mikealfare marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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") }}


{% endif %}
"""

_MODELS__DIM_ARTISTS = """
{{
config(
materialized='incremental',
on_schema_change='append_new_columns',
)
}}

select * from {{ ref("src_artists") }}
"""
26 changes: 25 additions & 1 deletion dbt-tests-adapter/dbt/tests/adapter/incremental/test_incremental_on_schema_change.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import pytest

from dbt.tests.adapter.incremental import fixtures
from dbt.tests.util import check_relations_equal, run_dbt
from dbt.tests.util import check_relations_equal, run_dbt, run_dbt_and_capture


class BaseIncrementalOnSchemaChangeSetup:
Expand Down Expand Up @@ -84,3 +84,27 @@ def test_run_incremental_fail_on_schema_change(self, project):

class TestIncrementalOnSchemaChange(BaseIncrementalOnSchemaChange):
pass


class BaseIncrementalCaseSenstivityOnSchemaChange:
@pytest.fixture(scope="class")
def models(self):
return {
"src_artists.sql": fixtures._MODELS__SRC_ARTISTS,
"dim_artists.sql": fixtures._MODELS__DIM_ARTISTS,
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

4 changes: 2 additions & 2 deletions dbt/include/global_project/macros/adapters/columns.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@McKnight-42 McKnight-42 Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbeatty10 oh these are really cool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

{% endfor %}{{ ',' if add_columns and remove_columns }}

{% for column in remove_columns %}
drop column {{ column.name }}{{ ',' if not loop.last }}
drop column {{ adapter.quote(column.name) }} {{ ',' if not loop.last }}
{% endfor %}

{%- endset -%}
Expand Down
Loading