-
Notifications
You must be signed in to change notification settings - Fork 696
Tpcc import with arenas (#17333) #20756
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds optional Protobuf Arena support to TValueBuilder
, enabling arena-allocated Ydb::Value
instances, and updates the TPCC import code to use a single google::protobuf::Arena
per thread for all bulk upserts.
- Extended
TValueBuilderImpl
andTValueBuilderBase
to accept an optionalArena
pointer and to allocate from it. - Modified all TPCC
Load*
functions to take agoogle::protobuf::Arena&
, pass it toTValueBuilder
, apply it inBulkUpsertSettings
, and reset after each call. - Updated constructors in
value.cpp
/value.h
and added the necessary protobuf includes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
ydb/public/sdk/cpp/src/client/value/value.cpp | Added Arena member, heap vs. arena allocation logic |
ydb/public/sdk/cpp/include/ydb-cpp-sdk/client/value/value.h | Changed builder constructors to take optional arena |
ydb/library/workload/tpcc/import.cpp | Updated TPCC import to use and reset a shared arena |
Comments suppressed due to low confidence (3)
ydb/public/sdk/cpp/src/client/value/value.cpp:2825
- [nitpick] Member 'Arena' lacks a trailing underscore and is inconsistent with other members (e.g., TypeBuilder_). Consider renaming to 'Arena_'.
google::protobuf::Arena* Arena;
ydb/public/sdk/cpp/src/client/value/value.cpp:2826
- [nitpick] Member 'ProtoValueHeap' does not follow the trailing underscore convention of other private members. Consider renaming to 'ProtoValueHeap_'.
Ydb::Value ProtoValueHeap;
ydb/public/sdk/cpp/include/ydb-cpp-sdk/client/value/value.h:533
- New arena‐enabled constructor path is not covered by existing tests. Consider adding unit tests for both heap and arena cases to ensure BuildValue behaves correctly.
TValueBuilderBase(google::protobuf::Arena* arena = nullptr);
🟢 |
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Changelog entry
...
Changelog category
Description for reviewers
...