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

change relation matching to case-insensitive #141

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

bachng2017
Copy link
Contributor

@bachng2017 bachng2017 commented Oct 1, 2022

Overview

This is a resolve for #140

The implement will override the compare function in the BaseRelation to use case-insensitive match as default.

Checklist

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • README.md updated and added information about my change
  • I have run changie new to create a changelog entry

@findinpath
Copy link
Collaborator

@bachng2017 build is failing because of code style settings.

Please do create a changelog entry

Your submission should be accompanied by corresponding tests.
Take inspiration from https://github.com/starburstdata/dbt-trino/blob/4e0ac14261cda131199ebac898d4d7a68cd40ed1/tests/functional/adapter/persist_docs/test_persist_docs.py (you only need one model for the purpose of your test).

@bachng2017
Copy link
Contributor Author

@findinpath we fixed code style and added a test. Should the checks run automatically ?

@bachng2017
Copy link
Contributor Author

ic.

First-time contributors need a maintainer to approve running workflows

@findinpath
Copy link
Collaborator

@bachng2017 because you are a 1st time commiter the build workflow needs to be approved manually.

Pls squash the commits and use the same commit mesaage guidelines as in Trino development

@findinpath findinpath requested review from mdesmet and hovaesco October 2, 2022 18:05
@mdesmet
Copy link
Contributor

mdesmet commented Oct 2, 2022

Issue with Starburst tests will be resolved by #139.

Copy link
Contributor

@mdesmet mdesmet left a comment

Choose a reason for hiding this comment

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

LGTM % comments

We should add a follow up story to remove this override when Trino supports case insensitive quoted identifiers.

dbt/adapters/trino/relation.py Outdated Show resolved Hide resolved
dbt/adapters/trino/relation.py Outdated Show resolved Hide resolved
@bachng2017
Copy link
Contributor Author

thanks @mdesmet for the review and fixes :)

bachng2017 added a commit to bachng2017/dbt-trino that referenced this pull request Oct 3, 2022
@bachng2017
Copy link
Contributor Author

@findinpath , is there any guideline for this ? Should I rebase all the commit into 1 ?

Pls squash the commits and use the same commit mesaage guidelines as in Trino development

@findinpath
Copy link
Collaborator

We should add a follow up story to remove this override when Trino supports case insensitive quoted identifiers.

@mdesmet can you pls reference a Trino open issue if there is any?

@findinpath
Copy link
Collaborator

@bachng2017 https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

@findinpath findinpath requested a review from mdesmet October 3, 2022 09:07
@hovaesco hovaesco merged commit 537e38e into starburstdata:master Oct 4, 2022
@hovaesco
Copy link
Contributor

hovaesco commented Oct 4, 2022

Thanks for your contribution!

@mdesmet
Copy link
Contributor

mdesmet commented Oct 4, 2022

Follow up issue created: #144

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