Skip to content

[C++] Add borrowing ingest_proto_records overload - #727

Open
zlata-stefanovic-db wants to merge 1 commit into
mainfrom
cpp-proto-record-view
Open

[C++] Add borrowing ingest_proto_records overload#727
zlata-stefanovic-db wants to merge 1 commit into
mainfrom
cpp-proto-record-view

Conversation

@zlata-stefanovic-db

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

Closes #724.

Stream::ingest_proto_records only accepted const 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, and make_proto_batch already copies nothing.

The overload. Adds ingest_proto_records(const ProtoRecordView*, std::size_t) alongside the existing one. zerobus::ProtoRecordView (in zerobus/record.hpp) is a non-owning {data, size} aggregate defaulting to {nullptr, 0}. Purely additive — no existing signature changes, no rust/ffi/zerobus.h change, so Go and Java are untouched and no FFI release is needed.

Named ProtoRecordView, not ProtoRecord. The issue proposed ProtoRecord, but sitting next to the owning UnackedRecord in 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 make stream.ingest_proto_records({}) ambiguous against the existing vector-of-vectors overload — silently breaking source that compiles today. Callers pass views.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 ZerobusException naming 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_records as the identical gap, but it is not symmetric: its FFI takes NUL-terminated const char*, so a string_view would have to be copied to terminate it, and std::string is 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, and cpp/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-check clean. Not run against a live endpoint.

The issue notes the batch path had no coverage, and it could not have any: make_proto_batch was file-local in an anonymous namespace, and a Stream only exists with a live server. So the adaptation moved to cpp/src/detail/proto_batch.hpp — deliberately free of zerobus.h, since it is pure pointer bookkeeping — and cpp/tests/proto_batch_test.cpp covers 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 Stream instance, which requires a server. The empty-batch no-op and the ensure_open ordering are exercised by the gated integration_test only.

Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
@zlata-stefanovic-db zlata-stefanovic-db self-assigned this Aug 12, 2026
@zlata-stefanovic-db
zlata-stefanovic-db marked this pull request as ready for review August 12, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] Batch ingest API forces callers to copy every record

1 participant