GH-42198: [C++] Fix GetRecordBatchPayload crashes for device data

[zeroshade]

Rationale for this change

Ensuring that creating IPC payloads works correctly for non-CPU data by utilizing CopyBufferSliceToCPU.

What changes are included in this PR?

Adding calls to CopyBufferSliceToCPU to the Ipc Writer for base binary types and for list types, to avoid calls to value_offset in those cases.

Are these changes tested?

Yes. Tests are added to cuda_test.cc

Are there any user-facing changes?

No.

• GitHub Issue: [C++] GetRecordBatchPayload crashes on non-cpu String and List arrays #42198

[github-actions]

⚠️ GitHub issue #42198 has been automatically assigned in GitHub to PR creator.

[lidavidm]

Seems there are failing tests? (And flaky macOS builds)

[paleolimbot]

+1 on some failing tests (maybe because of tests that serialize a string, binary, or list with an offset?)

I am assuming this is the Arrow C++ version of what I'm trying to optimize in apache/arrow-nanoarrow#509 ? I haven't measured how slow the previous version was or the effect of copying the required buffer values asynchronously, but my current mental model of GPU--host behaviour would suggest that waiting for two calls to MemoryManager::CopyBufferSliceToCPU() to complete synchronously is suboptimal (although better than crashing!).

[paleolimbot]

Does this need to take into account the array-level offset?

[zeroshade]

Since we create the zero-based value offsets, it turns out we can just grab the final offset after the conversion and avoid the subtraction as that will be the total byte length that we need. So we only need to perform a single copy there.

[jorisvandenbossche]

Using GetRecordBatchPayload with non-CPU device buffers should work just fine.

Personally I wouldn't necessarily expect that (e.g. I would expect this to crash with sliced nested arrays?), but so if this does actually work, it would be good to document that in the docstring of GetRecordBatchPayload (it might also be good to add a general test for that in cuda_test.cc (because I don't think this is actually tested right now?) and not only of the corner cases fixed in this PR).

[zeroshade]

@paleolimbot You were right that I didn't handle the offset properly, I've fixed that now. Since the two different offsets aren't contiguous, we'd have to do the copies separately anyways. It would definitely be slightly more optimal to do two async mem copies before sync'ing on them but as you said, it's better than crashing 😄

@jorisvandenbossche currently, you are correct that it will crash with sliced arrays in some cases. I'm certain that there are edge cases I haven't fully covered yet, which is why I didn't update the docs with this change. For now this is only focusing on string/binary arrays and lists until we run into other cases.

[paleolimbot]

For now this is only focusing on string/binary arrays and lists until we run into other cases.

While you (and we) are here, would it make sense to at least add tests for a few sliced arrays?

[zeroshade]

@paleolimbot currently creating an IPC payload from sliced arrays will fail due to having to recompute the null count which won't work for device data. As a result, we can't add tests for this yet until we address / fix that problem.

[paleolimbot]

we can't add tests for this yet

Would it be appropriate to add a test to ensure that this fails instead of crashes?

[zeroshade]

@paleolimbot currently in debug mode it will abort on DCHECK(is_cpu()) and in release mode it'll crash from passing NULLPTR to CountSetBits. GetNullCount doesn't provide any way to return a Status currently or otherwise indicate a failure rather than crashing

[paleolimbot]

I still think it would be a good idea to have ipc::IpcPayload fail instead of crash (and add the test to ensure this happens) while we're all here; however, I'm happy to defer to somebody with more experience with the expectations in Arrow C++ around testing 🙂 .

[zeroshade]

I'll take a look and see if there's a relatively easy way to do that without some major refactoring. Ultimately the issue is that GetNullCount is never expected to fail, so it doesn't use arrow::Result but simply returning 0 would be (IMHO) worse than crashing as it would be invalid and inaccurate

[paleolimbot]

Agreed that a refactor is out of scope! I basically had in mind a quick check before calling null_count() (I assume the crash happens hereish https://github.com/apache/arrow/pull/42199/files#diff-1b1d9dca9fdea7624e22f017b8762c4919edf57c2cf43c15d59b8a5e8e1b38a5L158 )

[zeroshade]

That's exactly where the failure comes from, I'll add a check there and a test that triggers it and confirms with ASSERT_NOT_OK ensuring it doesn't crash.

[lidavidm]

Can we link this to a follow up ticket?

[zeroshade]

I created a ticket #43029 and added a comment here to link to it

[lidavidm]

Why don't we take into account offset #0 here anymore?

[zeroshade]

just above this we use GetZeroBasedValueOffsets and populate value_offsets with zero-indexed offsets. This means that if we grab the very last offset in that buffer (array.length() * sizeof(offset_type)) we have the full number of data bytes and we can save the extra copy to grab offset #0

[lidavidm]

I think a rebase should fix a lot of the CI flakes?

[zeroshade]

@lidavidm I did a rebase, but I don't think the integration CI fail is related to this change, at least I can't think of how it would be. Any ideas what's up with the Java integration testing CI there?

[lidavidm]

CC @vibhatha

[zeroshade]

If i run archery docker run conda-integration I'm able to replicate all but the C++/C++ failure. I don't know Java well enough to run the tests and work out the issue there though.

@vibhatha any chance you'd be able to help out here?

[lidavidm]

I'll try to take a look in between ADBC stuff since there's been no response

[lidavidm]

Archery is quite frustrating when trying to just debug a single case...

[lidavidm]

I guess there's no way to do things other than to run the entire suite then try to pick up the pieces

[lidavidm]

I think it's C++, anyways. Java doesn't generate the file in question, it's

./build/debug/arrow-stream-to-file < /tmp/tmppsyt4cv0/a21e3cc2_generated_primitive_zerolength.producer_file_as_stream > /tmp/tmppsyt4cv0/a21e3cc2_generated_primitive_zerolength.consumer_stream_as_file

that's generating the invalid file.

[lidavidm]

And if I revert this patch, the problem goes away. So the problem is in this patch, not Java.

[zeroshade]

I think it's C++, anyways. Java doesn't generate the file in question, it's

./build/debug/arrow-stream-to-file < /tmp/tmppsyt4cv0/a21e3cc2_generated_primitive_zerolength.producer_file_as_stream > /tmp/tmppsyt4cv0/a21e3cc2_generated_primitive_zerolength.consumer_stream_as_file

that's generating the invalid file.

You were able to replicate the failure of the case with this? If so, that would definitely make it easier to debug

[zeroshade]

thanks for digging into it and figuring out that reproducer @lidavidm, i was able to debug and fix it in my latest commit

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit e9f35ff.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 49 possible false positives for unstable benchmarks that are known to sometimes produce them.