feat(go/adbc/sqldriver): read from union types#2637
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
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?
|
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. |
|
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 😄 |
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
|
Squashed commits aren't necessary since we're going to squash it anyways when we merge it. |
|
Any comments on the latest changes? |
|
Sorry, been super crazy on my end last week. This looks good! thanks!! |
|
Thanks! |
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 apache#2636
2 participants
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