[C++] Add borrowing ingest_proto_records overload - #727
Open
zlata-stefanovic-db wants to merge 1 commit into
Open
[C++] Add borrowing ingest_proto_records overload#727zlata-stefanovic-db wants to merge 1 commit into
zlata-stefanovic-db wants to merge 1 commit into
Conversation
Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
zlata-stefanovic-db
marked this pull request as ready for review
August 12, 2026 14:33
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Closes #724.
Stream::ingest_proto_recordsonly acceptedconst std::vector<std::vector<std::uint8_t>>&, so callers whose encoded records live anywhere else — an arena, a ring buffer, their own record type — had to copy every payload into that container before each call. As #724 shows, that copy is imposed by the parameter type, not by the memory model: the FFI borrows the bytes as parallel pointer/length arrays, andmake_proto_batchalready copies nothing.The overload. Adds
ingest_proto_records(const ProtoRecordView*, std::size_t)alongside the existing one.zerobus::ProtoRecordView(inzerobus/record.hpp) is a non-owning{data, size}aggregate defaulting to{nullptr, 0}. Purely additive — no existing signature changes, norust/ffi/zerobus.hchange, so Go and Java are untouched and no FFI release is needed.Named
ProtoRecordView, notProtoRecord. The issue proposedProtoRecord, but sitting next to the owningUnackedRecordin the same header that name implies it owns its bytes, which is the one thing a caller must not assume. Renaming later would be breaking, so it seemed worth settling now.No
const std::vector<ProtoRecordView>&convenience overload. It looks harmless but would makestream.ingest_proto_records({})ambiguous against the existing vector-of-vectors overload — silently breaking source that compiles today. Callers passviews.data(), views.size().Validates instead of trusting. A null array with a non-zero count, or any view with a null pointer and a non-zero size, throws a
ZerobusExceptionnaming the offending record's index rather than being dereferenced inside Rust.{nullptr, 0}stays valid — it is an empty record, matching how single empty records are already handled. This follows the guarding the rest of the file already does (ensure_open,checked_offset,checked_c_str).The vector overload does not delegate through the new one, per the issue: that would turn its single pass into two.
JSON is deliberately left alone. #724 notes
ingest_json_recordsas the identical gap, but it is not symmetric: its FFI takes NUL-terminatedconst char*, so astring_viewwould have to be copied to terminate it, andstd::stringis already the zero-copy shape. Worth its own decision rather than bundling.Docs updated per
cpp/CLAUDE.md:NEXT_CHANGELOG.md,cpp/README.md,cpp/examples/proto/README.md, andcpp/examples/proto/batch.cpp, which now sends a second batch out of a contiguous arena instead of re-ingesting the same rows.How is this tested?
make test(15/15),make lint(no warnings),make fmt-checkclean. Not run against a live endpoint.The issue notes the batch path had no coverage, and it could not have any:
make_proto_batchwas file-local in an anonymous namespace, and aStreamonly exists with a live server. So the adaptation moved tocpp/src/detail/proto_batch.hpp— deliberately free ofzerobus.h, since it is pure pointer bookkeeping — andcpp/tests/proto_batch_test.cppcovers it in the repo's dependency-free style.The load-bearing assertion is pointer identity: the built arrays must point at the caller's bytes, not at copies. That is the whole premise of the issue, and a regression to copying would still pass every behavioural test, so nothing else would catch it. Also covered: both null guards, the empty-record sentinel, and that the two overloads produce byte-identical arrays for equivalent input. It runs under the existing ASan and TSan CI jobs.
Two cases from the issue's list are not hermetically testable and are not included: "empty batch" and "closed stream" both need a
Streaminstance, which requires a server. The empty-batch no-op and theensure_openordering are exercised by the gatedintegration_testonly.