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.

Sign up for GitHub

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

Jump to bottom

Add Snowflake JDBC Connector #2551

Closed
wants to merge 2 commits into from
Closed

Add Snowflake JDBC Connector #2551

wants to merge 2 commits into from

Conversation

pgagnon
Copy link
Member

@pgagnon pgagnon commented Jan 19, 2020

Crude implementation of a Snowflake JDBC Connector.

Closes #1863

@cla-bot cla-bot bot added the cla-signed label Jan 19, 2020
@pgagnon pgagnon marked this pull request as ready for review January 19, 2020 19:24
Praveen2112
Praveen2112 reviewed Jan 22, 2020
View reviewed changes
Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR.

presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClient.java Outdated Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClient.java Outdated Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClientModule.java Outdated Show resolved Hide resolved
ebyhr
ebyhr reviewed Jan 22, 2020
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for creating this PR. Could you add the document?

It seems write query fails if we omit the default database from connection-url. It's different behavior comparing to other JDBC based connector, so it would be better to mention it.

presto-main/etc/config.properties Outdated Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClient.java Outdated Show resolved Hide resolved
@malthe
Copy link

malthe commented Feb 12, 2020

Bump?

@pgagnon pgagnon force-pushed the snowflake branch 3 times, most recently from 0143df4 to f14928a Compare February 29, 2020 17:47
@pgagnon pgagnon force-pushed the snowflake branch 2 times, most recently from 1ba865d to f265f38 Compare March 9, 2020 01:15
@tooptoop4
Copy link
Contributor

@pgagnon is this a read-only connector or can it insert/update data back into snowflake? does it support xml/json snowflake functions (https://docs.snowflake.com/en/sql-reference/functions-semistructured.html) ?

@pgagnon
Copy link
Member Author

pgagnon commented Mar 19, 2020 via email

@ebyhr
Copy link
Member

ebyhr commented Mar 20, 2020

@tooptoop4 The answer to the second question is no.

@pgagnon pgagnon requested review from Praveen2112 and ebyhr April 12, 2020 15:56
Praveen2112
Praveen2112 reviewed Apr 14, 2020
View reviewed changes
Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR. Can we add a test for config ?

presto-snowflake/pom.xml Outdated Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClient.java Outdated Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClientModule.java Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClientModule.java Outdated Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeClientModule.java Outdated Show resolved Hide resolved
presto-snowflake/src/main/java/io/prestosql/plugin/snowflake/SnowflakeConfig.java Outdated Show resolved Hide resolved
@tooptoop4 tooptoop4 mentioned this pull request Apr 18, 2020
Closed
@JeffryMAC
Copy link

any chance the connector will be released soon?

@pgagnon
Copy link
Member Author

pgagnon commented Aug 1, 2020

any chance the connector will be released soon?

Sorry I have been out of pocket for a while. I have received a couple of inquiries about it over the last week, so I will rebase it and address reviewer comments over the weekend.

@JeffryMAC
Copy link

@pgagnon thx. looking forward to this

Praveen2112
Praveen2112 approved these changes Sep 17, 2020
View reviewed changes
Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR. Looks good to me. Can you squash all the commits.

@findepi your insights ?

@pgagnon pgagnon force-pushed the snowflake branch 3 times, most recently from d16283e to 64e12af Compare October 5, 2020 15:25
@findepi
Copy link
Member

findepi commented Oct 6, 2020

Please make sure there is an issue to cover SF testing.

@pgagnon pgagnon mentioned this pull request Oct 6, 2020
Closed
@kokosing
Copy link
Member

Do you have any idea how to provide test coverage for that? Now it sounds very easy to have a regression unnoticed here. What are your thoughts?

@Praveen2112
Copy link
Member

I think Snowflake doesn't have a free account or a test account. Trial period is for 30 days. We can verify it manually but it is a tedious process (like we do for Redshift connector)

@kokosing
Copy link
Member

We can verify it manually

We could, but currently there are no tests that we could execute against manually created testing environment.

@Praveen2112
Copy link
Member

We can add Integration and DistributedQueriesTest test but we have to skip those tests in the GHA.

@tooptoop4
Copy link
Contributor

@pgagnon would you be able to add aggregate pushdown like 193872a#diff-e4b92558819e35928d8d20100000291dda1e15cf907c1d2f06ba4015b0afd6cc ?

@ravigbi
Copy link

ravigbi commented Dec 21, 2020

Checking in to see if there is any movement on this pull request. Looking forward to trying it out.

@pgagnon
Copy link
Member Author

pgagnon commented Dec 23, 2020

Checking in to see if there is any movement on this pull request. Looking forward to trying it out.

We're sort of blocked on a proper testing strategy right now, but I'll make sure to rebase soon so that you can at least play with the branch.

@ravigbi
Copy link

ravigbi commented Dec 23, 2020

Checking in to see if there is any movement on this pull request. Looking forward to trying it out.

We're sort of blocked on a proper testing strategy right now, but I'll make sure to rebase soon so that you can at least play with the branch.

That would be great. Thanks.

@ravigbi
Copy link

ravigbi commented Jan 19, 2021

Checking in to see if there is any movement on this pull request. Looking forward to trying it out.

We're sort of blocked on a proper testing strategy right now, but I'll make sure to rebase soon so that you can at least play with the branch.

That would be great. Thanks.

Hi @pgagnon. Not to be pushy but any update on the rebase?

@findepi findepi mentioned this pull request Mar 11, 2021
Closed
@lucasveljacic
Copy link

Hi @pgagnon! First of all, thanks for being working in this connector. We are really looking forward to use it!
I'm a data eng at centro.net and we have data in our SF databases and want to combine it with other data marts (HIVE and Mysql so far).

I've read this thread and, if I understood correctly, the PR is blocked due to some tests that can't be run as SF does not provide something like a docker image to code decent ITs. We run into this problem last year when we're developing a service and realized there wasn't a way to mock SF. We work it out by hitting the real SF from our Jenkins nodes (pointing to a dev DB). Not an ideal solution, but is working. However, it's true this tests won't be possible without a SF account.

I wonder if I could help in any way, maybe by running those manual tests mentioned by @Praveen2112 or helping you coding something as well, I'd be glad to help.

@chittshota
Copy link

@pgagnon Thanks for working on this feature.

@Praveen2112 @findepi Thanks a lot for the reviews on the initial PR. Could we see how to break the stalemate around testing ? While working on hadoop/hdfs project, we faced sometimes similar'ish challenges, and the way we sometimes adopted was to reflect the state properly in the documentation by using words such "Feature is in experimental phase". What are your thoughts about it ? With a plethora of downstream datasources, we may at times run into these sort of issues specific to certain connectors and hence a good way would be to reflect the state in the respective connector doc page. Happy to chime in and help in whatever way to get this across the finish line.

@lucasveljacic Cc

@yuuteng yuuteng mentioned this pull request Dec 22, 2021
Closed
@ebyhr ebyhr removed their request for review March 25, 2022 01:40
@rnpaiva
Copy link

rnpaiva commented Aug 4, 2022

Do you have plans to merge it ?

@csimplestring
Copy link

any plan to merge?

@mosabua
Copy link
Member

mosabua commented Oct 12, 2022 •
edited
Loading

Closing after discussion of @bitsondatadev with @pgagnon in favor of #10387.

@mosabua mosabua closed this Oct 12, 2022
@findepi findepi mentioned this pull request Dec 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanchitkashyap sanchitkashyap sanchitkashyap left review comments

@ebyhr ebyhr ebyhr left review comments

@findepi findepi findepi left review comments

@Praveen2112 Praveen2112 Praveen2112 approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Snowflake connector