Skip to content

GH-939: Remove reflection for gRPC buffers #954

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.

Sign up for GitHub

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

Jump to bottom
Open
xborder wants to merge 4 commits into
base: main
Choose a base branch
from
Open

GH-939: Remove reflection for gRPC buffers #954

xborder wants to merge 4 commits into from

Conversation

@xborder
Copy link
Contributor

@xborder xborder commented Jan 11, 2026

What's Changed

  • The branch removes the previous reflection-based access to internal gRPC buffers and replaces it with supported gRPC interfaces (Detachable, HasByteBuffer) to safely transfer ownership of direct buffers to Arrow without copying.
  • gRPC direct ByteBuffer streams are detached and wrapped as ForeignAllocation for zero‑copy reads
  • Fall back to safe buffer copies when gRPC does not expose a direct ByteBuffer or data is fragmented
  • Added test coverage

Closes #939.

@github-actions

This comment has been minimized.

Sign in to view
@xborder xborder changed the title GH-939: Remove relection GH-939: Remove reflection for gRPC buffers Jan 11, 2026
@xborder xborder marked this pull request as draft January 11, 2026 00:47
@lidavidm lidavidm added the enhancement PRs that add or improve features. label Jan 11, 2026
@xborder xborder marked this pull request as ready for review January 11, 2026 15:02
@github-actions github-actions bot added this to the 19.0.0 milestone Jan 11, 2026
lidavidm
lidavidm reviewed Jan 12, 2026
View reviewed changes
flight/flight-core/src/main/java/org/apache/arrow/flight/ArrowMessage.java Outdated
Comment on lines 463 to 466
if (detachedByteBuffer == null || !detachedByteBuffer.isDirect()) {
closeQuietly(detachedStream);
return null;
}
Copy link
Member

@lidavidm lidavidm Jan 12, 2026

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?

Copy link
Contributor Author

@xborder xborder Jan 12, 2026

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

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

Reviewers

@lidavidm lidavidm lidavidm left review comments

@laurentgo laurentgo Awaiting requested review from laurentgo laurentgo is a code owner

@wgtmac wgtmac Awaiting requested review from wgtmac wgtmac is a code owner

@jbonofre jbonofre Awaiting requested review from jbonofre jbonofre is a code owner

Assignees

No one assigned

Labels

enhancement PRs that add or improve features.

Projects

None yet

Milestone

19.0.0

Development

Successfully merging this pull request may close these issues.

[FlightRPC] Remove sketchy gRPC reflection code

2 participants

@xborder @lidavidm