Skip to content

[TASK-383] Error Handling fix CPP, for pointer returning methods#409

Merged
luoyuxia merged 3 commits into
apache:mainfrom
fresh-borzoni:fix-errors-pointers-cpp
Mar 3, 2026
Merged

[TASK-383] Error Handling fix CPP, for pointer returning methods#409
luoyuxia merged 3 commits into
apache:mainfrom
fresh-borzoni:fix-errors-pointers-cpp

Conversation

@fresh-borzoni

@fresh-borzoni fresh-borzoni commented Mar 2, 2026

Copy link
Copy Markdown
Member

Summary

closes #383

Replaces CXX Result<*mut T> bridge signatures with FfiPtrResult struct to preserve server error codes that were previously lost when CXX converted Rust errors into rust::Error (string-only)

@fresh-borzoni

fresh-borzoni commented Mar 2, 2026

Copy link
Copy Markdown
Member Author

@luoyuxia @zhaohaidao @leekeiabstraction PTAL 🙏

Copilot AI left a comment

Copy link
Copy Markdown

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 updates the C++ ↔ Rust CXX bridge for pointer-returning APIs to preserve server-side error codes by returning an intermediate FfiPtrResult (instead of Result<*mut T> which collapses errors to string-only rust::Error).

Changes:

  • Introduces FfiPtrResult { result: FfiResult, ptr: ... } in the Rust CXX bridge and updates pointer-returning Rust APIs to use it.
  • Updates C++ wrappers (connection/table/writer creation + write calls) to decode FfiResult and extract the returned pointer on success.
  • Adjusts WriteResult destruction to use an explicit Rust-side deleter (delete_write_result).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
bindings/cpp/src/lib.rs Adds FfiPtrResult, helper constructors, updates multiple Rust FFI exports to return it, and adds delete_write_result.
bindings/cpp/src/connection.cpp Switches connection/admin/table getters to consume FfiPtrResult and preserve error codes.
bindings/cpp/src/table.cpp Switches writer/scanner/lookuper creation and write calls to consume FfiPtrResult; updates WriteResult drop path.
bindings/cpp/src/ffi_converter.hpp Adds ptr_from_ffi helper to convert FfiPtrResult.ptr to typed pointers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread bindings/cpp/src/lib.rs Outdated
Comment thread bindings/cpp/src/ffi_converter.hpp
Comment thread bindings/cpp/src/connection.cpp
Comment thread bindings/cpp/src/lib.rs Outdated
@fresh-borzoni

Copy link
Copy Markdown
Member Author

Addressed comments

@fresh-borzoni fresh-borzoni changed the title [TASK-383] Error Handling fix, CPP for pointer returning methods [TASK-383] Error Handling fix CPP, for pointer returning methods Mar 2, 2026

@leekeiabstraction leekeiabstraction left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TY for the PR, left a comment. PTAL

Comment thread bindings/cpp/src/ffi_converter.hpp
@fresh-borzoni

Copy link
Copy Markdown
Member Author

@leekeiabstraction Ty for the review
Answered comment, PTAL 🙏

@luoyuxia luoyuxia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@fresh-borzoni Thanks. Merging..

@luoyuxia luoyuxia merged commit 257eb7d into apache:main Mar 3, 2026
2 checks passed
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.

Fix error propagation in CPP for pointer returining methods

4 participants