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 casing comparisons on dbt-created relations (#1555) #1558

Merged
merged 3 commits into from
Jun 21, 2019

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Jun 19, 2019

Fixes #1555

This might fix #1557 - hard to say as I couldn't figure out how to reproduce this one. I agree it's probably related though.

When dbt creates a relation in the db, add a special flag to the relation (defaults to False)
When checking a node name match:

  • if that flag is present and quoting is disabled, do a lowercase compare
  • otherwise remain case sensitive

Jacob Beck added 3 commits June 19, 2019 09:44
When dbt creates a relation in the db, add a special flag
When checking a node name match:
 - if that flag is present and quoting is disabled, do a lowercase compare
 - otherwise remain case sensitive
@beckjake beckjake marked this pull request as ready for review June 19, 2019 17:52
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

This looks good to me! This definitely fixes #1555, though I also am unable to reproduce #1557. That's ok, let's merge this as-is.

My only feedback is around the edges here: i think we'll need to update the spark plugin too? https://github.com/fishtown-analytics/dbt-spark/blob/master/dbt/adapters/spark/relation.py

Can you think of any other changes outside of the dbt-core codebase that would be required here? Like to new plugin setup script maybe?

@beckjake
Copy link
Contributor Author

I just checked and (contrary to my initial assumption) we shouldn't need to update the adapter plugin script, the script assumes you don't need to override Relation (phew!)

@beckjake
Copy link
Contributor Author

Third party plugins that override Relation will need to make changes though, yeah - I guess spark is one of those. It's a pretty minor change...

@drewbanin
Copy link
Contributor

ok, that's good. Let's merge this PR, then tackle dbt-spark separately. I'll make an issue over there to re-pin to 0.14.0 and can include this change in the description

@beckjake beckjake merged commit 2636969 into dev/wilt-chamberlain Jun 21, 2019
@beckjake beckjake deleted the fix/created-relations-casing branch June 21, 2019 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants