JunRuiLee commented on PR #771:
URL: https://github.com/apache/paimon-rust/pull/771#issuecomment-5613688251
Thanks for the scope review — restructured along it. Summary of where
everything went, and one item I could not act on.
### #771 is back on the one-shot boundary
Down from 38 files / +4,298−383 to **30 files / +2,236−343**. The staged
read API is withdrawn to #804 (draft), which is up now so it can be seen rather
than only described.
I took both of your options rather than one, because I think they compose:
this PR returns to option 1's boundary, and option 2 becomes the follow-up.
That way the answer to your 2026-09-03 comment on this PR — *"Why not use
VectorRead to read and use TableScan and TableRead to read real data?"* — is
still on the table, with its own contract discussion, instead of being dropped
to shrink the diff.
One correction to the framing, only so the history reads straight. That API
was added here **in response to** that comment, not in disregard of #755's
scope: #755 step 4 specifies the one-shot boundary this PR now has, and the
staged form it defers is the distributed one — *"candidate-only search, global
merge/rerank, and deferred materialization"* across workers, not an in-process
two-step read. The reason to move it out holds regardless, and it is #755's
own: the materialize contract should not be frozen before there is a consumer
for it.
#804 also keeps the one-shot terminal rather than replacing it, which is
what the earlier revision did. Once this PR is in, that Rust method and its C
ABI symbol are published surface, so removing them would be a breaking change
rather than the cleanup it would have been while the branch was unmerged. That
is also what makes #804 additive (+2,168/−30) instead of a removal plus a
replacement — roughly the other half of the review surface you were seeing.
### Independent fixes, split out
- **#802** — a deletion-vector position bound by its own source file
- **#803** — Lumina's empty-index error precedence
- **#801** — the scalar batch arity check
Each is on `main`, independent of the others and of this PR, with its own
tests. #802 and #803 were mutation-checked: reverting each fix individually
fails exactly its own test.
### The Lumina result-path item — I think this one may be a misread, and I
would rather ask than invent a PR
The review lists *"Existing Lumina result-path changes for NaN ordering,
sentinel-filled label buffers, and tie-breaking."* I cannot find those changes
on this branch. Against the merge base (`b761a74`):
```
$ git diff b761a74 HEAD -- crates/paimon/src/lumina/reader.rs \
| grep -E '^[+-]' | grep -iE 'nan|sentinel|new_label_buffer|row_id\.cmp'
| wc -l
0
```
All four are pre-existing, at `b761a74:crates/paimon/src/lumina/reader.rs`:
| symbol | line |
|---|---|
| `const SENTINEL` | 28 |
| `fn compare_scores` (the NaN arms) | 116–117 |
| `fn new_label_buffer` | 136 |
| `ScoredRow` tie-break on `row_id` | 166, 172 |
`git diff` on that file does touch the surrounding lines, because
`search_lumina` and `search_lumina_batch` gained a parameter for the
dense-filter budget, so those functions appear in the diff with the comparison
and buffer helpers nearby as context. My guess is that is what it looked like.
If you meant something else — another file, or a change I have miscategorised —
point me at it and I will split it out the same way.
### Test convergence
Both items done:
- `bucket_split_decoder_rejects_corrupt_bytes` is now a decoder assertion.
The entry point takes serialized bytes and decodes them at that boundary, so
that is where corrupt input is refused; it no longer drives a read behind a
query that cannot execute either way.
- Projection/filter rejection is asserted once, at the Rust level. The C
suite is argument marshalling, corrupt bytes, and one happy path over the Java
fixture — plus handle and pointer safety in #804, where the new handles are.
`vector_search_append_filter_returns_invalid_input` still fails under `-p
paimon-c`. It fails identically on `origin/main`; unrelated to this PR, and I
have left it alone.
--
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]
