[opt](lance) Support distributed execution for vector_search - #66841
[opt](lance) Support distributed execution for vector_search#66841zhangstar333 wants to merge 3 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Requesting changes for five correctness and rolling-upgrade issues: the deployed Lance Thrift fields are renumbered; a failed one-shot row-id task is retained after its caller wakes; V3 row-location bytes are silently misdecoded by an old materializer; the removed negative-fragment guard can turn an old-FE range into a successful empty scan; and new fragment-scoped searches have no old-BE capability fallback.
Critical checkpoints:
- Distributed search: the same-version fragment-local
top_k + offsetbound, global OFFSET/LIMIT, outer-WHERE placement, flat/indexed modes, filters, ties, deletes, and empty/tail fragments were traced and are consistent. Mixed-version wire and physical-execution compatibility is not. - Row identity and lazy materialization: same-version uint64 propagation, snapshot/storage identity, grouping, deduplication, input-order restoration, hidden-row-ID injection, aliases/nested columns, and direct take were checked. The V3 consumer compatibility issue remains blocking.
- Reader/FFI lifecycle: dataset/scanner/Arrow ownership, virtual-only projection, returned-row validation, and error cleanup were checked. The deserialized fragment boundary regression remains blocking.
- Concurrency: the new worker defer avoids the waiter's hang, but an error result violates the scheduler callback's completion contract and retains the task/closure.
- Coverage: the changed tests cover same-version distributed search and row fetch paths, but not the mixed-version Thrift/layout/capability cases, the old-FE high-bit fragment path, or failed-task cleanup.
No additional user focus was supplied. The review converged after three full FE, BE/third-party, and cross-layer rounds. Static review only; builds and tests were not run because the review environment explicitly forbids them.
| scan_status = status; | ||
| } | ||
| } | ||
| return status; |
There was a problem hiding this comment.
[P1] Mark this one-shot scheduler task complete even when the fetch fails. SimplifiedScanTask stores a std::function<bool()>, so returning an error Status here converts to false. ScannerSplitRunner then never sets its completion future, it is not auto-rescheduled, and the one-shot wrapper removes its task handle only for a true result. The new defer wakes the waiter, so the caller returns scan_status while the executor still retains an unfinished split/closure capturing this stack frame by reference; repeated read errors leak those scheduler tasks for the process lifetime. Record the error in scan_status, but return true from the scheduler callback after the single attempt.
| std::array<uint8_t, 7> reserved_before_backend_id {}; | ||
| int64_t backend_id; | ||
| uint32_t file_id; | ||
| std::array<uint8_t, 4> reserved_before_row_id {}; |
There was a problem hiding this comment.
[P1] Gate this V3 row-location encoding on the materializing BE's capability. During a rolling upgrade, a new scan BE can send these bytes through the distributed TopN to an old materialization BE. The old release code raw-casts every value as the 24-byte V2 layout without checking its version or length. This 32-byte layout preserves the backend/file offsets but puts zero padding at V2's row-id offset 20, so the old BE silently decodes every V3 value as row ID 0 and may splice the first row's deferred columns into a different selected row. Please negotiate V3 support (or use an old-reader-safe representation) before emitting it.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)