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-2178] Handling column names having / while altering the table #63

Open
2 tasks done
preethi8p opened this issue Feb 24, 2023 · 7 comments
Open
2 tasks done
Assignees
Labels
type:bug Something isn't working as documented

Comments

@preethi8p
Copy link

preethi8p commented Feb 24, 2023

Is this a new bug in dbt-snowflake?

  • I believe this is a new bug in dbt-snowflake
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

We had few columns with "/" in the column names. During incremental runs, when dbt runs the alter statement to either add or drop a column it is failing with the below error in the global macro columns.sql
ERROR_MESSAGE
SQL compilation error:
syntax error line 3 at position 12 unexpected '/'.
image

Fix
Modified global macro to add proper quoting to the column names during alter table statement.

Expected Behavior

We expect that the alter statements to add or drop columns, must run successfully without any failure. Basically if we have added a column in the previous layer(say load layer), and during incremental load in the next layer(raw layer) it should run the alter statement and add the column to the raw table. Please note it failed because it has a character "/" in the column name.

Steps To Reproduce

  1. Add a column to a table in load layer with a name like "KOND/CC" (with a /)
  2. Do an incremental run so that this column should be added in the next layer which uses this table as source.
  3. Alter statement to add the column is triggered and it fails with error mentioned above
  4. As a fix, we need to include the column within double quotes in the macro "snowflake__alter_relation_add_remove_columns" as we are using Snowflake.
  5. Upon fix it will run the alter statement successfully and add that column to the table.

Relevant log output

Log output with error:
2023-02-16T17:23:47.3708425Z �[0m17:23:47  �[33mDatabase Error in (models/raw/cust.sql �[0m 
2023-02-16T17:23:47.3714555Z �[0m17:23:47    001003 (42000): SQL compilation error:
2023-02-16T17:23:47.3720917Z �[0m17:23:47    syntax error line 3 at position 12 unexpected '/'.
2023-02-16T17:23:47.3727341Z �[0m17:23:47    syntax error line 3 at position 16 unexpected '/'.

Log output after fix:
2023-02-20T12:39:08.9562443Z �[0m12:39:08  121 of 1023 OK created incremental model qa_raw.cust .......... [�[32mSUCCESS 0�[0m in 193.62s]

Environment

- OS: Windows 10
- Python: 3.8.6
- dbt-core: 1.3.1
- dbt-snowflake:1.3.0

Additional Context

No response

@preethi8p preethi8p added the type:bug Something isn't working as documented label Feb 24, 2023
@github-actions github-actions bot changed the title Handling column names having "/" while altering the table [CT-2178] Handling column names having "/" while altering the table Feb 24, 2023
@jtcohen6
Copy link
Contributor

Linking:

@preethi8p
Copy link
Author

preethi8p commented Feb 24, 2023

Have raised a Pull Request for the issue - Update columns.sql dbt-labs/dbt-core#7025

@github-actions
Copy link
Contributor

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 comment on the issue or else it will be closed in 7 days.

@preethi8p
Copy link
Author

I hope this was accepted as a valid issue, so is this PR completed? How do I track? If yes, then please provide me the link to verify the same else please let me know the next step.

@dbeatty10 dbeatty10 changed the title [CT-2178] Handling column names having "/" while altering the table [CT-2178] Handling column names having / while altering the table Feb 5, 2024
@dbeatty10 dbeatty10 changed the title [CT-2178] Handling column names having / while altering the table [CT-2178] Handling column names having / while altering the table Feb 5, 2024
@dbeatty10 dbeatty10 transferred this issue from dbt-labs/dbt-snowflake Feb 5, 2024
@dbeatty10
Copy link
Contributor

dbeatty10 commented Feb 5, 2024

This was reported multiple times and covering a plurality of adapters:

Acceptance criteria

Implementation detail was originally described here: dbt-labs/dbt-core#7025 (comment)

In short:

  1. Use {{ adapter.quote() }} as described in dbt-labs/dbt-adapters#160 (comment)
  2. Add test case(s) to the on_schema_change tests for a column that contains a special character (/, ", etc.) or is a reserved keyword, e.g. from.
  3. Inherit these test cases within each adapter (dbt-snowflake, etc).

More detail

After doing a git grep "column\.name" dbt/, here are three places I noticed in the code base to update:

  1. add column {{ column.name }} {{ column.data_type }}{{ ',' if not loop.last }}
  2. drop column {{ column.name }}{{ ',' if not loop.last }}
  3. alter table {{ relation }} add column "{{ column.name }}" {{ column.data_type }};

Since the 3rd one is related to snapshots, ideally there would be at least one snapshot-specific functional test that needs a quoted identifier to handle a special character or reserved keyword.

Copy link
Contributor

github-actions bot commented Oct 8, 2024

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 comment on the issue or else it will be closed in 7 days.

@github-actions github-actions bot added the Stale Mark an issue or PR as stale, to be closed label Oct 8, 2024
@preethi8p
Copy link
Author

Its been too long I have opened this issue, now I don't even remember where to track back. Now I am not clear on all the comments, so please let me know if this can be published or not ? And what should be my next step towards this? TIA.

@github-actions github-actions bot removed the Stale Mark an issue or PR as stale, to be closed label Oct 9, 2024
@adrianburusdbt adrianburusdbt self-assigned this Oct 28, 2024
mikealfare pushed a commit that referenced this issue Dec 2, 2024
mikealfare pushed a commit that referenced this issue Jan 20, 2025
…ackup & dist/sort (#63)

* set BACKUP parameter before dist/sort keys
* updated changelog
* added test v1
* Finalize backup tests
* update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working as documented
Projects
None yet
4 participants