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

[BUG] Column Escaping Doesn't Work with NOT Well #114

Closed
ahuangpod opened this issue Apr 11, 2022 · 6 comments · Fixed by #132
Closed

[BUG] Column Escaping Doesn't Work with NOT Well #114

ahuangpod opened this issue Apr 11, 2022 · 6 comments · Fixed by #132
Assignees
Labels
bug Something isn't working
Milestone

Comments

@ahuangpod
Copy link

Describe the bug
Now dbtvault wraps all the columns with quotes, but not would be in the quotes as well.

Versions

dbt: 1.0.4
dbtvault: 0.8.2

To Reproduce

 {{
     dbtvault.stage(
         source_model = "source_model",
         hashed_columns = {
             "hk_a": "id",
             "a_hashdiff": {
                 "is_hashdiff": true,
                 "columns": [
                     "a",
                 ]
             }
         },
         derived_columns = {
             "derived_column_a": "not column_a",
         },
     )
 }}

The compiled SQL would be like:

derived_columns AS (
    SELECT

    "not column_a" AS "eff_flag",

   FROM source_data
),

Expected behavior
The not should be outside the quotes like:

derived_columns AS (
    SELECT

    not "column_a" AS "eff_flag",

   FROM source_data
),

Screenshots
If applicable, add screenshots to help explain your problem.

Log files
If applicable, provide dbt log files which include the problem.

Additional context
Add any other context about the problem here.

@ahuangpod ahuangpod added the bug Something isn't working label Apr 11, 2022
@ahuangpod
Copy link
Author

Do we have any updates on this?

BTW, here is the macro:

 {%- macro default__escape_column_name(column) -%}

     {%- set escape_char_left  = var('escape_char_left',  '"') -%}
     {%- set escape_char_right = var('escape_char_right', '"') -%}

     {%- set escaped_column_name = escape_char_left ~ column | replace(escape_char_left, '') | replace(escape_char_right,  '') | trim ~ escape_char_right -%}

     {%- do return(escaped_column_name) -%}

 {%- endmacro -%}

Thank you.

@DVAlexHiggs
Copy link
Member

Hi, thank you for your report! Sorry for the delayed response.

There's a few other cases I can think of where the quoting will cause problems and we'll want to make sure our existing tests cover those.

@DVAlexHiggs
Copy link
Member

DVAlexHiggs commented Jun 23, 2022

We're thinking of implementing this by removing quoting from derived columns by default, and adding it as a configuration option per derived column.

This would allow more or less normal functionality and provide the means for people to solve issues with reserved words and spaces if needed in hopefully, less common cases (this was the original reason we introduced escaping/quoting in the first place)

Thoughts on this?

@DVAlexHiggs
Copy link
Member

DVAlexHiggs commented Jun 24, 2022

Something like this:

derived_columns:
  BOOKING_TYPE: {
    source_column: "TYPE",
    escape: true
  }
 # OR
 derived_columns:
  BOOKING_TYPE: "TYPE"

@ahuangpod
Copy link
Author

Thank you, I think that looks good to me. Making it configurable would be useful.

@DVAlexHiggs DVAlexHiggs added this to the 0.8.4 milestone Jul 6, 2022
DVAlexHiggs added a commit that referenced this issue Jul 10, 2022
* Revert as_constant() to original code

* escape_column_names() macro to skip column constant values

* Expected data not required for eponymous unit test

* WIP derive_columns() macro code changes

Not yet complete

* WIP hash() macro code changes

Not yet complete

* Revised hash() and hash_columns() unit tests

Any computed columns must be configured as derived columns first

* WIP derive_columns() macro code and tests changes

* Testing and debugging

* Testing and debugging

Explicitly coded escaping within derived column calculation

* Create a test harness escape() function

* Revised code based on parse_hashdiff()

Tested on Snowflake so far.
Apologies, yesterday's code was wrong wrong wrong.

* Passing for Snowflake and MS SQL Server

For MSSQL pipenv / Pipfile must have EITHER dbt-core ==1.1.0 and dbt-sqlserver ==1.1.0 OR dbt-core ==1.0.6 and dbt-sqlserver ==1.0.0, no other version combinations work.

* Minor changes

Co-authored-by: Alex Higgs <alex.higgs@data-vault.com>
@DVTimWilson DVTimWilson linked a pull request Jul 28, 2022 that will close this issue
@DVAlexHiggs
Copy link
Member

Fix released in v0.9.0. Let us know if you continue to have issues. Thanks!

DVAlexHiggs added a commit that referenced this issue Dec 17, 2022
* Revert as_constant() to original code

* escape_column_names() macro to skip column constant values

* Expected data not required for eponymous unit test

* WIP derive_columns() macro code changes

Not yet complete

* WIP hash() macro code changes

Not yet complete

* Revised hash() and hash_columns() unit tests

Any computed columns must be configured as derived columns first

* WIP derive_columns() macro code and tests changes

* Testing and debugging

* Testing and debugging

Explicitly coded escaping within derived column calculation

* Create a test harness escape() function

* Revised code based on parse_hashdiff()

Tested on Snowflake so far.
Apologies, yesterday's code was wrong wrong wrong.

* Passing for Snowflake and MS SQL Server

For MSSQL pipenv / Pipfile must have EITHER dbt-core ==1.1.0 and dbt-sqlserver ==1.1.0 OR dbt-core ==1.0.6 and dbt-sqlserver ==1.0.0, no other version combinations work.

* Minor changes

Co-authored-by: Alex Higgs <alex.higgs@data-vault.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants