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

Fix - Quote columns in unpivot #626

Closed
wants to merge 15 commits into from
Closed

Conversation

GtheSheep
Copy link

@GtheSheep GtheSheep commented Jul 26, 2022

This is a:

  • documentation update
  • bug fix with no breaking changes
  • new functionality
  • a breaking change

Description & motivation

Closes #216

When unpivoting column names that require quotations (dates, strings with spaces, etc) unpivot.sql doesn't respect quote_columns=true or anything like that.

I wanted to open this to get input on best methodology before adding tests:

  • Should this be a new argument?
  • Should this respect the quote_columns config instead?
  • Other?

Checklist

  • 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
    • 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_utils.type_* macros instead of explicit datatypes (e.g. dbt_utils.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

@joellabes
Copy link
Contributor

Thanks for taking this on @GtheSheep!

I wanted to open this to get input on best methodology before adding tests:

  • Should this be a new argument?

Yes I think it should!

  • Should this respect the quote_columns config instead?

I don't think so - normally for dbt utils, we aren't trying to be clever and inspect the state of yaml files. I personally consider it a bit of an antipattern to hide configuration behaviour for macros inside of yaml, because it means you have to check two places to understand how it's going to work. (If there was a global quote_columns config ever added to dbt_project.yml, I'd probably change my tune).

Does that get you unstuck? Sorry for the delay in getting back to you 🐌

@GtheSheep
Copy link
Author

Hey @joellabes thanks for this, super helpful!

yeah I was thinking there was a global config for quote_columns but it seems to just be for seeds, my bad! But yep, this unblocks me, will keep it as close as possible to how this works for pivot.sql and get on with adding a test for this.

Thanks 😸

@joellabes
Copy link
Contributor

@GtheSheep Heads up that we've just done some surgery on the unpivot macro (most notably #671). Will be putting out a first beta of dbt utils 1.0 in the immediate future, which you'll probably want to pull into this branch. Would love to get these changes into 1.0 as well if possible, so that we can keep all the behavioural changes to a single release. Doesn't matter as much for this one because it's backwards compatible.

@seajhawk
Copy link

@GtheSheep & @joellabes -
I'm very excited to see this PR in progress!

Is there a known workaround I can use while the PR process continues?

@GtheSheep GtheSheep force-pushed the main branch 2 times, most recently from bfb8981 to 68be94e Compare November 7, 2022 00:41
@GtheSheep GtheSheep force-pushed the main branch 2 times, most recently from eaa3a7a to e0406d3 Compare December 31, 2022 17:02
@GtheSheep GtheSheep closed this Mar 6, 2023
@error418
Copy link
Contributor

error418 commented May 8, 2023

Any comment on why this PR was closed? Is this open to be picked up by someone else?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable quoting column names for unpivot macro
4 participants