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(go/adbc/driver/snowflake): workaround snowflake metadata-only limitations #1790

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

zeroshade
Copy link
Member

Workaround to fix #1454 until snowflake addresses snowflakedb/gosnowflake#1110 with a better solution (hopefully by having the server actually return Arrow...)

@zeroshade zeroshade requested review from lidavidm and joellubi April 29, 2024 17:55
@github-actions github-actions bot added this to the ADBC Libraries 1.0.0 milestone Apr 29, 2024
@zeroshade
Copy link
Member Author

@davidhcoe I've added a test that simulates the case where you running into an issue in order to confirm that it fixes the problem. But it would still be great to get you to test this with your SHOW TABLES example to confirm that this fixes that scenario too

@davidhcoe
Copy link
Contributor

@davidhcoe I've added a test that simulates the case where you running into an issue in order to confirm that it fixes the problem. But it would still be great to get you to test this with your SHOW TABLES example to confirm that this fixes that scenario too

I will try it either later tonight or early tomorrow at the latest.

Comment on lines +514 to +517
// the "JSON" data returned isn't valid JSON. Instead it is a list of
// comma-delimited JSON lists containing every value as a string, except
// for a JSON null to represent nulls. Thus we can't just use the existing
// JSON parsing code in Arrow.
Copy link
Member

Choose a reason for hiding this comment

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

oh joy

go/adbc/driver/snowflake/record_reader.go Show resolved Hide resolved
@lidavidm
Copy link
Member

Ok, we can wait for @davidhcoe to give this a spin and then merge

@zeroshade
Copy link
Member Author

@lidavidm I emphasized on the linked snowflake issue that this is a workaround and short-term solution. If snowflake ever decides to change their JSON format (since they aren't using valid JSON) or something, they'll break us. but I can't find a better workaround / solution to this unless snowflake fixes it on their server side.

@davidhcoe
Copy link
Contributor

Works great. Two thumbs up. Thank you @zeroshade.

@lidavidm
Copy link
Member

@lidavidm I emphasized on the linked snowflake issue that this is a workaround and short-term solution. If snowflake ever decides to change their JSON format (since they aren't using valid JSON) or something, they'll break us. but I can't find a better workaround / solution to this unless snowflake fixes it on their server side.

Yup. If it breaks, we can redirect people to Snowflake as there's only so much we can do here.

@lidavidm lidavidm merged commit cdb2c62 into apache:main Apr 30, 2024
39 checks passed
@zeroshade zeroshade deleted the snowflake-json-only branch April 30, 2024 16:39
cocoa-xu pushed a commit to meowcraft-dev/arrow-adbc that referenced this pull request May 8, 2024
…itations (apache#1790)

Workaround to fix apache#1454 until snowflake addresses
snowflakedb/gosnowflake#1110 with a better
solution (hopefully by having the server actually return Arrow...)
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.

go/adbc/driver/snowflake: driver cannot support query "SHOW TABLES"
3 participants