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

Add quote_identifiers parameter to unpivot to handle case-sensitive column names #792

Merged
merged 10 commits into from
Jul 11, 2024
Merged

Add quote_identifiers parameter to unpivot to handle case-sensitive column names #792

merged 10 commits into from
Jul 11, 2024

Conversation

error418
Copy link
Contributor

@error418 error418 commented May 9, 2023 •
edited by dbeatty10
Loading

resolves #216

Description & motivation

Added quote_identifiers parameter to unpivot to handle case-sensitive column names.

This is like #135 was for pivot.

Checklist

  • This code is associated with an Issue which has been triaged and accepted for development.
  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery (tested by @Ishankoradia - see PR comments)
    • Postgres
    • Redshift
    • Snowflake
  • I followed guidelines to ensure that my changes will work on "non-core" adapters by:
    • dispatching any new macro(s) so non-core adapters can also use them (e.g. the star() source)
    • using the limit_zero() macro in place of the literal string: limit 0
    • using dbt.type_* macros instead of explicit datatypes (e.g. dbt.type_timestamp() instead of TIMESTAMP
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@error418 error418 force-pushed the feat/unpivot-quote-columns branch 2 times, most recently from 876fba7 to 32ccf3d Compare May 15, 2023 15:59
@error418 error418 marked this pull request as draft May 15, 2023 16:00
@error418 error418 force-pushed the feat/unpivot-quote-columns branch 8 times, most recently from 605dcaa to d2de2de Compare May 15, 2023 17:36
@error418 error418 marked this pull request as ready for review May 15, 2023 17:42
joellabes
joellabes previously requested changes May 16, 2023
View reviewed changes
Copy link
Contributor

@joellabes joellabes left a comment

Choose a reason for hiding this comment

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

Thanks @error418! The changes to the macro look good; I have a few nitpicks around the tests though ⛏️

integration_tests/models/sql/test_unpivot_quote.sql Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
integration_tests/models/sql/test_unpivot_quote.sql Outdated Show resolved Hide resolved
@error418 error418 force-pushed the feat/unpivot-quote-columns branch from d2de2de to 9f0ff03 Compare May 16, 2023 14:33
@error418 error418 marked this pull request as draft May 16, 2023 14:34
@error418 error418 force-pushed the feat/unpivot-quote-columns branch 2 times, most recently from 97a00ed to bcb1b1f Compare May 16, 2023 14:47
@error418 error418 marked this pull request as ready for review May 16, 2023 14:58
@error418 error418 force-pushed the feat/unpivot-quote-columns branch from bcb1b1f to 03467be Compare May 17, 2023 12:51
@error418
Copy link
Contributor Author

rebased to current main and resolved merge conflicts in CHANGELOG.md

@error418 error418 requested a review from joellabes May 17, 2023 15:14
@error418
Copy link
Contributor Author

error418 commented Jul 3, 2023

Anything missing to get this merged?

@error418 error418 mentioned this pull request Sep 26, 2023
Closed
@github-actions GitHub Actions
Copy link

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 31, 2023
@error418
Copy link
Contributor Author

ping

@github-actions github-actions bot removed the Stale label Jan 1, 2024
@Ishankoradia Ishankoradia mentioned this pull request Apr 11, 2024
Merged
@Abhishek-N
Copy link

hey @error418 @joellabes, facing a similar issue and need to have this merged. can you merge this asap please?

@error418
Copy link
Contributor Author

@Shadowsong27 @Ishankoradia @Abhishek-N at the moment I am waiting for a review by @joellabes (or any maintainer of dbt-utils).

@Ishankoradia thank you for testing the changes for BigQuery

Should there be any remaining issues with this PR I am happy to provide the necessary fixes.

@dbeatty10 dbeatty10 added the bug Something isn't working label Apr 18, 2024
@pnd-mnicolau
Copy link

bump

@alejobs
Copy link

alejobs commented Apr 30, 2024

What's missing here? I need this fix to be deployed. Thanks.

@dbeatty10 dbeatty10 added the case_sensitivity Issues related to case-sensitivity behavior label May 13, 2024
@dbeatty10 dbeatty10 added the quoting Issues related to quoting behavior label Jun 4, 2024
@MiguelMadero
Copy link

What's missing here? I need this fix to be deployed. Thanks.

@error418
Copy link
Contributor Author

@joellabes could you please provide some information about the status of this pull request?

This is stalling for a while now without any feedback from the maintainers, which sadly becomes a bit frustrating.

Thanks

@dbeatty10 dbeatty10 self-requested a review June 25, 2024 21:21
@dbeatty10
Copy link
Contributor

Hi all,

This is our highest priority PR to review right now, and I'll be diving deeper into it this week.

After taking an initial look, the heart of the implementation looks good.

The main thing I'd like to follow-up on prior to approving + merging is the testing:

  • It looks like there is special configuration in the CI tests so that Snowflake and Redshift pass

So I'd like to:

  • Make sure that the test is running for Redshift.
  • Align how Snowflake is handled across similar tests. i.e., either standardize on prior art or standardize on the adapter.quote() approach in this PR

@dbeatty10
Copy link
Contributor

@error418 I think I figured out the root cause of the Redshift test failures in CI.

As you noted, the default behavior in Redshift is to lowercase all column names even if a table is created with quoted mixed-case column names.

It looks like the solution is to set enable_case_sensitive_identifier to true. I'm going to try adding that configuration to the CI cluster or workgroup's parameter group and see if that allows this test to pass for Redshift.

@dbeatty10 dbeatty10 mentioned this pull request Jun 27, 2024
Open
1 task
@dbeatty10 dbeatty10 changed the title feat(unpivot): add quote identifier parameter to handle case sensitive columns Add quote_identifiers parameter to unpivot to handle case-sensitive column names Jun 27, 2024
@dbeatty10 dbeatty10 added enhancement New feature or request and removed bug Something isn't working labels Jun 27, 2024
@dbeatty10 dbeatty10 dismissed joellabes’s stale review July 11, 2024 19:51

Addressed the tests ⚒️

dbeatty10
dbeatty10 reviewed Jul 11, 2024
View reviewed changes
macros/sql/unpivot.sql
@@ -12,11 +12,11 @@ Arguments:
value_name: Destination table column name for the pivoted values
#}

{% macro unpivot(relation=none, cast_to='varchar', exclude=none, remove=none, field_name='field_name', value_name='value') -%}
{{ return(adapter.dispatch('unpivot', 'dbt_utils')(relation, cast_to, exclude, remove, field_name, value_name)) }}
{% macro unpivot(relation=none, cast_to='varchar', exclude=none, remove=none, field_name='field_name', value_name='value', quote_identifiers=False) -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

In an ideal world, I would rather have the default be quote_identifiers=True so that it matches with pivot. But agree with your choice here to default it to False instead so we don't have any breaking behavior change.

dbeatty10
dbeatty10 approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

We're ready to rock @error418 🎸

The tests in Redshift are passing after setting enable_case_sensitive_identifier to true in the cluster’s parameter group via the Amazon Redshift console.

@dbeatty10 dbeatty10 added this pull request to the merge queue Jul 11, 2024
Merged via the queue into dbt-labs:main with commit 9237ba9 Jul 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbeatty10 dbeatty10 dbeatty10 approved these changes

@joellabes joellabes Awaiting requested review from joellabes

Assignees
No one assigned
Labels
case_sensitivity Issues related to case-sensitivity behavior enhancement New feature or request quoting Issues related to quoting behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable quoting column names for unpivot macro
9 participants
@error418 @Shadowsong27 @Ishankoradia @Abhishek-N @pnd-mnicolau @alejobs @MiguelMadero @dbeatty10 @joellabes