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

Added case_sensitive_cols argument to generate_base_model macro #63

Merged
merged 4 commits into from
Jun 22, 2022

Conversation

graciegoheen
Copy link
Contributor

@graciegoheen graciegoheen commented Jun 3, 2022

This is a:

  • bug fix PR with no breaking changes — please ensure the base branch is main
  • new functionality — please ensure the base branch is the latest dev/ branch
  • a breaking change — please ensure the base branch is the latest dev/ branch

Description & motivation

I added an argument case_sensitive_cols to generate_base_model macro to allow this macro to still work for source tables with case sensitive column names.

Checklist

  • I have verified that these changes work locally (Snowflake)
  • 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

@graciegoheen graciegoheen requested review from b-per and dbeatty10 June 3, 2022 20:33
@graciegoheen graciegoheen self-assigned this Jun 3, 2022
Copy link
Contributor

@b-per b-per left a comment

Choose a reason for hiding this comment

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

LGTM!
Just a quick comment about describing what the new parameter actually does

README.md Outdated Show resolved Hide resolved
@b-per
Copy link
Contributor

b-per commented Jun 7, 2022

Oh, and I didn't see that it fails for Redshift. Not sure why this is the case though.

@graciegoheen
Copy link
Contributor Author

@graciegoheen
Copy link
Contributor Author

Column names in Google BigQuery are not able to be case sensitive... I updated the logic to remove the "" wrapping that was throwing an error for BigQuery only. So the generate_base_model macro will still use the CaseSensitive source column names but won't be quoted. Example in BigQuery:

...
    select
        My_Integer_Col,
        My_Bool_Col

    from source
...

Example in other data warehouse:

...
    select
        "My_Integer_Col",
        "My_Bool_Col"

    from source
...

@graciegoheen graciegoheen requested a review from b-per June 7, 2022 16:14
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.

It would be awesome if there's some property of the adapter itself that we can use for handling columns that should/shouldn't be quoted. That would enable us to just apply whatever that logic is rather than requiring logic like if target.type == "bigquery".

Here's some examples of the kind of thing I'm thinking of:

Since this is working and for sake of expediency, I didn't research if any of these (or alternatives) would be applicable to this case though.

We can circle back later and attempt to generalize it if we so need or desire.

@dbeatty10 dbeatty10 merged commit 5d36cd6 into main Jun 22, 2022
jeremyholtzman pushed a commit that referenced this pull request Apr 10, 2023
* Added case_sensitive_cols argument to generate_base_model macro

* added all_args test

* enable_case_sensitive_identifier for redshift

* removed "" wrapping for bigquery
@gwenwindflower gwenwindflower deleted the feature/add_case_sensitive_cols branch February 28, 2024 23:01
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.

3 participants