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

Add case insensitive identifier matching to Phoenix connector #872

Merged
merged 2 commits into from Jun 1, 2019

Conversation

3 participants
@vincentpoon
Copy link
Member

commented May 31, 2019

This provides a solution for #858

@electrum

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Thanks for fixing this issue so quickly!

Subclassing BaseJdbcConfig is not ideal, as that will bind config parameters like connection-password which won't be used by Phoenix. Instead, we should add a BaseJdbcClient constructor to directly take the two case insensitive config values (leaving the existing one with BaseJdbcConfig that delegates to the new one). Then we would simply add the two config options to PhoenixConfig.

It would also be helpful to add a TestPhoenixConfig like we have for TestBaseJdbcConfig and other config classes.

Otherwise, this looks good.

vincentpoon added some commits May 31, 2019

@vincentpoon vincentpoon force-pushed the vincentpoon:phoenix_case_insensitive branch from d548ade to 5137bb4 Jun 1, 2019

@vincentpoon

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

@electrum thanks, updated to address your comments

@dain

dain approved these changes Jun 1, 2019

Copy link
Member

left a comment

Looks good

@electrum
Copy link
Member

left a comment

Thanks! I’ll merge this after Travis finishes.

@electrum electrum merged commit 83235e8 into prestosql:master Jun 1, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@electrum electrum added this to the 313 milestone Jun 1, 2019

@electrum electrum referenced this pull request Jun 1, 2019

Closed

Release notes for 313 #850

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.