GH-939: Remove reflection for gRPC buffers by xborder · Pull Request #954 · apache/arrow-java (original) (raw)
What's Changed
- Replace reflection-based gRPC buffer handling with FlightDataParser reading from InputStream/detached ByteBuffer, enabling zero‑copy ArrowBuf slices for app_metadata and data_body.
- Simplify ArrowMessage.frame to use the shared parser and fallback path; remove GetReadableBuffer and add grpc-core runtime/module updates.
- Add TestArrowMessageParse coverage for duplicate fields, zero‑copy allocation, and non‑detach fallback behavior.
Closes #939.
This comment has been minimized.
GH-939: Remove relection GH-939: Remove reflection for gRPC buffers
Comment on lines +463 to +466
| if (detachedByteBuffer == null || !detachedByteBuffer.isDirect()) { | | -------------------------------------------------------------------- | | closeQuietly(detachedStream); | | return null; | | } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we detached the stream, if we give up here, aren't we erroneously going to discard the actual data? My understanding of Detachable is that the original stream is now invalid, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. Actually this is redundant, the isDirect is already checked at the beginning of the method so I just removed it
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All builds fail:
Error: Non-test scoped test only dependencies found:
Error: io.grpc:grpc-core:jar:1.78.0:compile
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved CI
CC @ejona86 in case you do want to look over this
@xborder it seems this doesn't actually work...
@lidavidm Seems like a chicken and the egg problem. grpc-core can't be a compile dependency because it is detected as unused by CI although it is still required in runtime (AddWritableBuffer still needs it for reflection).
Marking it with a runtime scope causes the tests I created to fail because they require the dependency in compile time.
I've changed the tests to use a mock for the dependency it had on grpc-core. This seemed to have solved the issues from the two previous runs but probably diminishes the relevance of these tests
@xborder thanks for the update. Let me take a look.
@lidavidm I think there's something I overlooked. frame() is iterating over the stream, if I detach it when reading APP_METADATA_TAG it invalidates the same stream it is iterating. Let me rework on this and I'll keep you posted
@jbonofre I tried to reproduce this on a Windows 11 VM and couldn't. Could this be a flaky test?
Scope detached FlightData buffers to a message-local allocator and retain them into downstream allocators as messages are consumed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})