Skip to content
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

[Java][C] sliced RecordBatch offset info is lost when imported from c-data #41682

Open
hellishfire opened this issue May 16, 2024 · 6 comments
Open

Comments

@hellishfire
Copy link

hellishfire commented May 16, 2024

Describe the bug, including details regarding any error messages, version, and platform.

Reproduced on latest arrow release (16.0)

When importing a sliced RecordBatch from c to java

On c side:

auto sliced_record_batch = original_record_batch->Slice(/*offset=*/8, /*length=*/2);
arrow::ExportRecordBatch(sliced_record_batch, arrow_array_ptr);

On java side:

ArrowArray arrowArray = ArrowArray.allocateNew(allocator);
Data.importIntoVectorSchemaRoot(allocator, arrowArray, vectorSchemaRoot, null);

The imported vectorSchemaRoot maintains the correct length(which is 2), but the offset info (which is 8) is not respected, hence the content of the imported vectorSchemaRoot points to the first 2 rows of the original_record_batch, while the desired content is sliced_record_batch.

I'm not familiar with arrow code, but it seems that the offset info is actually present in org.apache.arrow.c.ArrowArray.Snapshot, but org.apache.arrow.c.ArrayImporter ignores the offset in org.apache.arrow.c.ArrayImporter.doImport(ArrowArray.Snapshot)

Component(s)

Java

@lidavidm
Copy link
Member

Java doesn't really have a concept of slicing, so either this needs to copy the data (at least for things that can't easily be sliced, like the validity buffer) or it should error on import.

@hellishfire
Copy link
Author

hellishfire commented May 16, 2024

Java doesn't really have a concept of slicing, so either this needs to copy the data (at least for things that can't easily be sliced, like the validity buffer) or it should error on import.

I took a quick look at the slice() method of VectorSchemaRoot, which slices the data buffer, and slice or copy the validity buffer

@lidavidm
Copy link
Member

Yes, but that's different in intent than the C++ version (which just tracks an offset). Here it means we can't do a 0 copy import.

@hellishfire
Copy link
Author

Yes, but that's different in intent than the C++ version (which just tracks an offset). Here it means we can't do a 0 copy import.

I understand that, but IMO data correctness comes before performance, so a copy of validity buffer is tolerable in this case?

@lidavidm
Copy link
Member

CC @vibhatha

We also have to traverse recursively since child arrays can have their own offset

@vibhatha
Copy link
Collaborator

I will try to reproduce this and evaluate the approach suggested by @lidavidm. I will probably need some time to try this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants