Copilot commented on code in PR #236:
URL: https://github.com/apache/fluss-rust/pull/236#discussion_r2762417891
##########
bindings/cpp/src/table.cpp:
##########
@@ -265,6 +265,15 @@ Result LogScanner::Subscribe(const
std::vector<BucketSubscription>& bucket_offse
return utils::from_ffi_result(ffi_result);
}
+Result LogScanner::SubscribePartition(int64_t partition_id, int32_t bucket_id,
int64_t start_offset) {
+ if (!Available()) {
+ return utils::make_error(1, "LogScanner not available");
+ }
+
Review Comment:
`SubscribePartition` passes `partition_id` straight through to the Rust FFI.
On the Rust side, `-1` currently has a special meaning (it routes to the
non-partition `subscribe` path), so calling `SubscribePartition(-1, ...)` will
not do what the API name implies. Consider validating `partition_id` here
(e.g., reject negative values) to prevent surprising behavior at the C++ API
boundary.
```suggestion
if (partition_id < 0) {
return utils::make_error(2, "partition_id must be non-negative");
}
```
##########
bindings/cpp/src/lib.rs:
##########
@@ -790,6 +819,15 @@ impl LogScanner {
}
}
+ fn subscribe_partition(
+ &self,
+ partition_id: i64,
+ bucket_id: i32,
+ start_offset: i64,
+ ) -> ffi::FfiResult {
Review Comment:
`subscribe_partition` forwards `partition_id` into `do_subscribe`, where
`-1` is treated as a sentinel meaning “no partition” and will call `subscribe`
instead of `subscribe_partition`. This makes `partition_id = -1` behave
unexpectedly for callers and leaks an internal sentinel into the public FFI
API. Add an explicit validation in `subscribe_partition` (e.g., reject
`partition_id < 0` with a clear error) and/or refactor `do_subscribe` to take
`Option<PartitionId>` so the sentinel value is not part of the value space.
```suggestion
) -> ffi::FfiResult {
if partition_id < 0 {
return err_result(1, "partition_id must be
non-negative".to_string());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
