Skip to content

feat(go/adbc/sqldriver): read from union types #2637

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

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

murfffi
Copy link
Contributor

@murfffi murfffi commented Mar 24, 2025

Implements reading from union types and adds a unit test. All tests pass, except those with C shared object dependencies. I did not build the code outside go/adbc - let me know if needed for this PR.

In addition to the added unit test, I tested the scenario from the issue with DuckDB.

Closes #2636

@murfffi murfffi requested a review from zeroshade as a code owner March 24, 2025 16:18
@github-actions github-actions bot added this to the ADBC Libraries 18 milestone Mar 24, 2025
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on the response here, this looks good to me in general, but can we add a test which contains more than one type in the union? With results from both types and ensure that we get the correct values for each index?

Also, can we please add a test for DenseUnion in addition to the test with SparseUnion?

@murfffi
Copy link
Contributor Author

murfffi commented Mar 28, 2025

Thanks for taking a look! Will add the tests.

Are you okay with not supporting nested unions? I could have added a loop for that but I thought the added complexity wasn't worth the questionable value of nested unions.

@zeroshade
Copy link
Member

Incrementally adding support should be fine. so not supporting nested unions for now is fair. Besides, it just means it's a good reason to use the Arrow interfaces directly instead 😄

@murfffi
Copy link
Contributor Author

murfffi commented Mar 29, 2025

Sorry for the delay on the response here, this looks good to me in general, but can we add a test which contains more than one type in the union? With results from both types and ensure that we get the correct values for each index?

Also, can we please add a test for DenseUnion in addition to the test with SparseUnion?

Added. Forgot to ask if squashed commits are preferred.

Implements reading from union types and adds a unit test.
All tests pass.

Closes apache#2636
@zeroshade
Copy link
Member

Squashed commits aren't necessary since we're going to squash it anyways when we merge it.

@murfffi
Copy link
Contributor Author

murfffi commented Apr 7, 2025

Any comments on the latest changes?

@zeroshade
Copy link
Member

Sorry, been super crazy on my end last week. This looks good! thanks!!

@zeroshade zeroshade merged commit 854d31e into apache:main Apr 7, 2025
36 checks passed
@murfffi murfffi deleted the feat2636 branch April 7, 2025 14:55
@murfffi
Copy link
Contributor Author

murfffi commented Apr 7, 2025

Thanks!

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/sqldriver: read from union types
2 participants