[Bug] Incremental compilation error alter queries not using quoted columns

[ChristopheDuong]

Is there an existing issue for this?

• [X] I have searched the existing issues

Current Behavior

I've created a repository with multiple tests that summarizes compilation errors with SQL queries generated by dbt when running incremental models at: https://github.com/airbytehq/testing_dbt

One of the issues occurs when we rename columns of an incremental model (with on_schema_change: sync_all_columns) while including special characters in the new column name. Therefore the columns should be referred to with quoting.

from https://github.com/airbytehq/testing_dbt/blob/2d8f87b721e13122dd5fd9fa8088ee7734f9017e/first_project/models/change_column_names.sql renamed to https://github.com/airbytehq/testing_dbt/blob/2d8f87b721e13122dd5fd9fa8088ee7734f9017e/second_project/models/change_column_names.sql

• Table creation works properly while quoting identifiers.

  create temporary table "change_column_names__dbt_tmp204510736609"
  as (
  select
  'replacing underscore with space' as "new column name"
  now() as insert_time
  );

  https://github.com/airbytehq/testing_dbt/blob/2d8f87b721e13122dd5fd9fa8088ee7734f9017e/output/postgres-3-schema-change-FAILED-dbt.log#L279

• But alter table queries fails when using incremental materialization:

  alter table "postgres"."tests"."change_column_names"
     add column new column name text,
     drop column column_name,

  https://github.com/airbytehq/testing_dbt/blob/2d8f87b721e13122dd5fd9fa8088ee7734f9017e/output/postgres-3-schema-change-FAILED-dbt.log#L1555

Expected Behavior

The expected SQL query should be:

 alter table "postgres"."tests"."change_column_names"
     add column "new column name" text,
     drop column column_name,

Steps To Reproduce

1. in https://github.com/airbytehq/testing_dbt/
2. ./run_test.sh

Relevant log output

No response

Environment

- OS: mac Os & docker (fishtownanalytics/dbt)
- Python: 3.8.12
- dbt: 0.21.0

What database are you using dbt with?

postgres, redshift, snowflake, other (mention it in "Additional Context")

Additional Context

The error occurs with:

• postgres
• redshift
• snowflake
• sqlserver

[jtcohen6]

@ChristopheDuong Thanks again for the thoughtful write-up and reproduction cases!

I think this one is even more straightforward than dbt-labs/dbt-core#4422. Because these column values are being returned to us from the database, and never from user-space code, they should have the exactly correct casing + quoting. We don't need to worry about case-insensitivity on Snowflake, for instance. So I think we could safely wrap these column names in quotes, always, just like we do for alter_column_type.

Given that, these lines in default__alter_relation_add_remove_columns:

https://github.com/dbt-labs/dbt-core/blob/ed5df342ca5d99c5e6971ee6d11c8cf3e6e263b3/core/dbt/include/global_project/macros/adapters/columns.sql#L77-L83

Become:

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

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

The same goes for snowflake__alter_relation_add_remove_columns, taking a cue (and some confidence) from snowflake__alter_column_type. That will require a follow-on PR in the dbt-snowflake repo.

(I don't know if this is required in dbt-bigquery + dbt-spark, since I don't think they support column identifiers containing spaces or special characters to begin with.)

The fix here should be very quick. It's verifying the fix that requires slightly more effort.

In order to test this change, and feel good about it everywhere, I think the order of operations needs to look like:

1. Pre-work: Convert the test cases for on_schema_change to the new testing framework; currently they're here, and scattered throughout each database plugin. They should move into the "adapter zone" of test cases, where they can be more easily extended to each adapter plugin.
2. New test case: Add a test case that features an incremental model with columns that have spaces, special characters, etc., and need to be quoted. Ensure it fails for all/some on_schema_change modes.
3. Fix: Add adapter.quote, as shown above. Ensure the test cases passes for all on_schema_change modes.
4. Merge + extend: Merge the test cases and fixes into this repo. Inherit + run the test cases in other plugins, for relevant databases, with fixes as needed (dbt-snowfalke for sure).

Given that, I'm going to hold off on labeling this good first issue until we have that "pre work" (test conversion) done. At that point, the right scaffolding will be in place to support a more-tactical community contribution.

[github-actions[bot]]

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please remove the stale label or comment on the issue, or it will be closed in 7 days.

[github-actions[bot]]

Although we are closing this issue as stale, it's not gone forever. Issues can be reopened if there is renewed community interest; add a comment to notify the maintainers.

[bniedzie]

I wanted to follow up on this - has there been any update on this bug and the relevant pre-work? Thanks for any information you can provide!

[DTchebotarev]

@jtcohen6 I believe I have a fix here with the changes you outlined including tests. https://github.com/dbt-labs/dbt-core/pull/8914

[dbeatty10]

Re-opening due to PR in https://github.com/dbt-labs/dbt-core/pull/8914

[felipecoxanet]

Hi, any one have any update on this bug? thanks

[dbeatty10]

@felipecoxanet we weren't able to review the associated PR prior to a recent refactor, so now we'd need it opened up again in a new repo. We're not able to prioritize doing that ourselves right now, but we'd welcome someone from the community taking it on.

See more here: https://github.com/dbt-labs/dbt-core/pull/8914#issuecomment-2048366265

[maxpowis-bp]

+1