JingsongLi commented on PR #946: URL: https://github.com/apache/paimon-rust/pull/946#issuecomment-5831415133
Requirement fit: **SUPPORTED**, with a narrower performance claim. Exposing the existing split-limit hint to Go/C has real value, but `plan_snapshot_from_lists` reads the manifest entries before the positive-limit accumulator runs; the PR should not claim that it avoids all manifest/statistics I/O. Implementation: **FINDINGS** at `e173fde0`. **[P1] Reject negative Go limits before the unsigned cast** (`bindings/go/read_builder.go:83-87`). `WithLimit(-1)` currently returns success and passes `uintptr(-1)` to Rust. In `LimitPushdownAccumulator::push`, `self.limit as i64` then becomes `-1`, so the first split with a known row count satisfies the limit and later splits are omitted. A caller passing a negative config/sentinel gets silently incomplete scan results. Please reject `limit < 0` (or use an unsigned public type), and add a Go→C→scan regression with multiple known-count splits. Verification: the PR C test `test_read_builder_with_limit_prunes_plan_splits` passed. I added a temporary Rust probe with `usize::MAX` and two known-count splits; it failed on the first `push`, confirming early truncation. I removed the probe after reproduction, leaving the review checkout clean. `git diff --check main...HEAD` passed; all 14 PR CI jobs are green, but none covers the negative input. -- 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]
