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

feat(go/adbc/driver): Adbc Driver for Snowflake #586

Merged
merged 40 commits into from
Apr 28, 2023

Conversation

zeroshade
Copy link
Member

@zeroshade zeroshade commented Apr 13, 2023

Initial work to start creating a snowflake ADBC driver which we can eventually package up like we do for the Flight SQL driver. Currently only GetInfo and GetObjects are implemented, but it's a start!

Will have to add secrets to the repo eventually to allow tests to work.

This relies on snowflakedb/gosnowflake#781 getting merged first

@zeroshade zeroshade self-assigned this Apr 13, 2023
@github-actions
Copy link

⚠️ Please follow the Conventional Commits format in CONTRIBUTING.md for PR titles.

@zeroshade
Copy link
Member Author

requires snowflakedb/gosnowflake#769 in order to work properly

@lidavidm
Copy link
Member

Cool!

Let me know if you need help adding secrets (but the process should just be file INFRA Jira, then email to private@apache.org when you get the go-ahead, in case you haven't done it before)

@zeroshade
Copy link
Member Author

@lidavidm I've filed https://issues.apache.org/jira/browse/INFRA-24477 to get it added.

@zeroshade zeroshade marked this pull request as ready for review April 19, 2023 20:12
@zeroshade zeroshade requested a review from lidavidm April 19, 2023 20:12
@zeroshade
Copy link
Member Author

@lidavidm This is ready for review now, though the CI/tests aren't going to pass until I get the SNOWFLAKE_URI secret added as per the JIRA ticket I filed. In the meantime, the rest of the code can be reviewed and looked at. Though this still relies on some PRs for the gosnowflake lib that haven't gotten merged yet.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Those are some...interesting type conversions we have going on here.

LGTM in general. It seems the driver is mostly straightforward once you get the herculean effort of fixing up the underlying driver out of the way?

go/adbc/driver/snowflake/driver.go Show resolved Hide resolved
go/adbc/driver/snowflake/statement.go Outdated Show resolved Hide resolved
go/adbc/driver/snowflake/statement.go Show resolved Hide resolved
@zeroshade zeroshade changed the title WIP: feat(go/adbc/driver): Adbc Driver for Snowflake feat(go/adbc/driver): Adbc Driver for Snowflake Apr 21, 2023
@zeroshade zeroshade force-pushed the snowflake-driver branch 2 times, most recently from 57592f8 to ce07db3 Compare April 24, 2023 15:10
go/adbc/go.mod Outdated Show resolved Hide resolved
Comment on lines +259 to +260
When calling :cpp:`AdbcConnectionGetTableSchema`, the returned Arrow Schema
will contain metadata on each field:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should eventually standardize something like Flight SQL's scheme for encoding this sort of info...

.github/workflows/native-unix.yml Outdated Show resolved Hide resolved
LICENSE.txt Show resolved Hide resolved
license.tpl Show resolved Hide resolved
.github/workflows/native-unix.yml Outdated Show resolved Hide resolved
.github/workflows/integration.yml Outdated Show resolved Hide resolved
.github/workflows/integration.yml Outdated Show resolved Hide resolved
LICENSE.txt Outdated Show resolved Hide resolved
c/validation/adbc_validation.cc Outdated Show resolved Hide resolved
c/validation/adbc_validation.cc Outdated Show resolved Hide resolved
@zeroshade
Copy link
Member Author

@lidavidm My change on the snowflake repo has been merged and I've updated the go.mod here to point to that commit hash. They release on a monthly schedule so the next actual release which will include this change won't be until the end of May, at which point we can update the go.mod to point to that specific version tag.

So this is good to go!

@zeroshade
Copy link
Member Author

Actually, I should update the install and package files for this shouldn't I? or should that be a separate change?

ci/linux-packages/debian/rules Outdated Show resolved Hide resolved
@lidavidm
Copy link
Member

Actually, I should update the install and package files for this shouldn't I? or should that be a separate change?

Up to you - whatever is easier

@zeroshade zeroshade merged commit 50a9e89 into apache:main Apr 28, 2023
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.

None yet

2 participants