-
Notifications
You must be signed in to change notification settings - Fork 242
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
UnionExec array and nested array support #3359
Conversation
Signed-off-by: Ryan Lee <[email protected]>
15459fd
to
5140585
Compare
build |
Curious, this depends on a cudf feature that hasn't been merged but the tests pass. This makes me think the tests aren't sufficiently exercising the code, otherwise a test should have failed without the requisite cudf feature. |
Yeah, this looks dangerous to me too 😃 |
Converting this to a draft so this doesn't get accidentally merged by a passerby until this mystery is solved. |
build |
I can turn this test error on and off with a very clear change within cudf, but for whatever reason the followup build still passed. The followup build was started before the PR was merged, but it was pretty close timing. If the blossom CI was delayed pulling the snapshot it may have picked up a cudf snapshot with the fix in it. Looking into it tomorrow, but with locally tests demonstrating cause and effect I think this is good to reopen/merge... |
Confirmed while testing with different versions of the snapshot jars, the new test correctly hits the dependent interleave column functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve the plugin side changes assuming it has been verified that this patch fails with older versions of cudf
Verified offline with different snapshot builds. Fails with |
Resolves #1459
Depends on rapidsai/cudf#9130 for unionByName list of struct support. Approved but not merged yet.
Adds type support for arrays and nested arrays for
union
,unionAll
,unionByName
. Also includes better UnionExec testing for nested types.