-
Notifications
You must be signed in to change notification settings - Fork 901
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
Remove legacy Arrow interop APIs #16590
Remove legacy Arrow interop APIs #16590
Conversation
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.
Approved CMake changes
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.
I'm not 100% sure what is happening, but we are getting failures trying to use this in Spark when we write data to python processes for python UDF processing.
E Caused by: ai.rapids.cudf.CudfException: writer failed to write table with the following error: Invalid: Tried to write record batch with different schema
E at ai.rapids.cudf.Table.writeArrowIPCArrowChunk(Native Method)
I did some more debugging and the issue appears to happen when after writing batches with non-null values out, we then try to write a batch with only null values in it.
That is when the error happens. We end up doing this somewhat regularly when we do groupby aggregations using python UDFS. Spark has a kind of crappy way of doing UDFS in python where each grouping key gets a separate arrow batch written out for it. This happens when we generate a group by where all of the values we are computing the agg for ended up being null (in this case it is a single row). I think the problem is that the schema translation either expects to never see nulls if it didn't see it in the first batch, or that it has some kind of issue with the batch being all nulls. Not really sure. All I know is the schema discovery is not happy with this situation. |
OK got it, thanks for tracking this case down. If you have time to write a minimal C++ repro that would be amazing, but no worries if not; I will try and create one myself by EOD today. |
OK, I pushed a fix. We've had a few issues crop up around the new API around the nullability of columns when converting from cudf if the cudf column has no nulls. We may want to rethink defaults, but when I tried fiddling with them here I kept finding edge cases in other scenarios where things broke, so I suspect that the defaults in other libraries like arrow are not entirely consistent. For now, I've just patched the implementation for the JNI since we presumably want to rewrite this to avoid libarrow altogether. I'll open a new issue to discuss the default behavior, but I think this solution ought to be OK for now. |
Opened #16621 for further discussion. |
I just tested and I am still getting the same error. I'll do some more debugging and see what I can come up with for this. |
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.
That did it
Thanks for the quick turnaround on testing! |
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.
A small question otherwise looks good to me.
/merge |
I noticed from #16590 (comment) that there was one other file where `#pragma once` was not at the top. This PR fixes that. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #16636
This reverts commit 6c4905d.
Description
Contributes to #15193.
Checklist