Skip to content

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.

@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
Comment on lines 463 to 466
if (detachedByteBuffer == null || !detachedByteBuffer.isDirect()) {
closeQuietly(detachedStream);
return null;
}
Copy link
Member

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

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

Copy link
Member

@lidavidm lidavidm left a comment

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

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved CI

@lidavidm
Copy link
Member

CC @ejona86 in case you do want to look over this

@lidavidm
Copy link
Member

@xborder it seems this doesn't actually work...

@xborder
Copy link
Contributor Author

xborder commented Jan 15, 2026

@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

@jbonofre
Copy link
Member

@xborder thanks for the update. Let me take a look.

@xborder
Copy link
Contributor Author

xborder commented Jan 15, 2026

@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

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

Labels

enhancement PRs that add or improve features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FlightRPC] Remove sketchy gRPC reflection code

3 participants