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 ignore_case for Snowflake #308

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

cakkinep
Copy link
Contributor

Description & motivation

Snowflake external tables by default do not ignore case of columns of underlying external File, this poses a challenge when defining to exactly define the column names to be able to successfully create external tables and have the column values populated.

This PR adds the capability to use GET_IGNORE_CASE function to lookup the column value instead of standard value: reference.

Checklist

  • [ *] I have verified that these changes work locally
  • [* ] I have updated the README.md (if applicable)
  • [* ] I have added an integration test for my fix/feature (if applicable)

@cakkinep
Copy link
Contributor Author

@dataders need you help with this integration test failure, its not related to any of the changes i made.

@jeremyyeo
Copy link
Collaborator

@cakkinep - I'll have a look to see what's up with those CI errors.

@jeremyyeo
Copy link
Collaborator

Some reason some of those external tables (like PEOPLE_JSON_PARTITIONED) had been recreated as tables from some other process perhaps - can't say for sure. Will drop them and rerun ci.

@cakkinep
Copy link
Contributor Author

Yeah, saw that in the errors, couldn't do much as I didn't have access to the CI environment and resources it is running against. Thanks for working on the fix.

@cakkinep
Copy link
Contributor Author

cakkinep commented Aug 6, 2024

@robby-rob can you please check if this PR will solve for what you were looking for in #289 ?

@cakkinep
Copy link
Contributor Author

cakkinep commented Aug 9, 2024

@jeremyyeo , @dataders , tagging you both for a review to get this merged. Please let me know if i may have missed anything .

@cakkinep
Copy link
Contributor Author

@jeremyyeo please let me know if you are not the right person to request a review and get this MR move forward.

@robby-rob-slalom
Copy link

@robby-rob can you please check if this PR will solve for what you were looking for in #289 ?

This appears to solve what I was working on. Is there a case for allowing the ignore_case option when not using the infer_schema option? I believe I added that in when #257 was merged, but not sure how important it is.

@cakkinep
Copy link
Contributor Author

This appears to solve what I was working on. Is there a case for allowing the ignore_case option when not using the infer_schema option? I believe I added that in when #257 was merged, but not sure how important it is.

@robby-rob-slalom
This change handles ignore case for both infer_schema and non infer case too.

Thanks for your feedback.


{% if infer_schema %}
{% set query_infer_schema %}
select * from table( infer_schema( location=>'{{external.location}}', file_format=>'{{external.file_format}}') )
select * from table( infer_schema( location=>'{{external.location}}', file_format=>'{{external.file_format}}', ignore_case=>true) )

Choose a reason for hiding this comment

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

May want to have the ignore_case option here use the ignore_case variable, defaulting to blank or false to match Snowflake defaults.

Something similar to this commit

Copy link
Contributor Author

@cakkinep cakkinep Aug 26, 2024

Choose a reason for hiding this comment

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

I am not using column name from the results of this ignore_case=>true, instead i am explicitly using GET_IGNORE_CASE function on the inferred columns to prevent any issues , my observation is when using ignore_case=>true snowflake is converting the columns to upper case by default ( snowflake's interpretation of ignoring case) and using them in the value:: reference, this would not get the correct value.

By explicitly using the function , i am able to make it truly ignore case.

corresponding code on line 59 here - https://github.com/dbt-labs/dbt-external-tables/pull/308/files#diff-bab29748b029dbac246005b734e423bdc39eb7735844083cd6902e1afbcab7f7R59

@cakkinep
Copy link
Contributor Author

@dataders , @jeremyyeo , can you please look into this or get attention of someone who can look into merging this MR?

@cakkinep
Copy link
Contributor Author

cakkinep commented Sep 4, 2024

@jeremyyeo , @dataders , can you please review and get this merged?

@cakkinep
Copy link
Contributor Author

@jeremyyeo , @dataders if you are not the right people to tag for getting this reviewed and merged, please tag those who are responsible.

@cakkinep
Copy link
Contributor Author

How can we get this merged? @dataders @jeremyyeo

@cakkinep
Copy link
Contributor Author

@jeremyyeo can you please review and let me know if anything else needs to be done to get this merged?

Copy link
Collaborator

@dataders dataders left a comment

Choose a reason for hiding this comment

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

@cakkinep, I'm going to merge this and cut a beta release. Please shout if this does not do the trick. cheers

@dataders dataders merged commit 5f344e6 into dbt-labs:main Oct 25, 2024
3 checks passed
@dataders dataders mentioned this pull request Oct 28, 2024
3 tasks
dataders added a commit that referenced this pull request Oct 28, 2024
dataders added a commit that referenced this pull request Oct 28, 2024
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.

4 participants