Conversation
|
@leekeiabstraction @zhaohaidao PTAL 🙏 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the C++ bindings to make ScanRecord a value type by wrapping the underlying FFI data in std::shared_ptr instead of using raw pointers. Previously, ScanRecord contained a RowView that borrowed from ScanRecords, creating dangling reference issues when users tried to store records beyond the lifetime of the ScanRecords container. The fix enables users to safely accumulate records across multiple Poll() calls, matching the Rust API's semantics.
Changes:
- Changed
RowViewto holdshared_ptrinstead of raw pointers for both the FFI data and column map - Replaced manual memory management in
ScanRecordswith defaulted constructors/destructors - Modified
LogScanner::Poll()to wrap the FFI pointer inshared_ptrwith a custom deleter that callsrust::Box::from_raw()
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| bindings/cpp/include/fluss.hpp | Changed RowView and ScanRecords to use shared_ptr for memory management; defaulted special member functions |
| bindings/cpp/src/table.cpp | Removed manual Destroy() logic; updated Poll() to create shared_ptr with custom deleter |
| website/docs/user-guide/cpp/api-reference.md | Updated documentation to reflect value-type semantics and reference counting |
| website/docs/user-guide/cpp/data-types.md | Revised lifetime documentation and examples to show safe accumulation patterns |
| website/docs/user-guide/cpp/example/log-tables.md | Added examples for continuous polling and accumulating records across polls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
luoyuxia
left a comment
There was a problem hiding this comment.
@fresh-borzoni Thanks. LGTM!
Summary
ScanRecord in C++ held a raw pointer to the underlying scan data, making it a borrowed view that dangled when ScanRecords was destroyed. Users couldn't store or accumulate records across Poll() calls, unlike Rust (Arc) where records are self-contained.
Fixed by wrapping the opaque ScanResultInner in shared_ptr with a custom deleter that calls rust::Box::from_raw(). ScanRecord is now a value type: copyable, storable, safe to accumulate. Zero-copy preserved (same FFI pointer path), no Rust-side changes needed.
Updated docs to replace borrowing warnings with value-type semantics and added an accumulation example to log-tables guide.