Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of exact head f1d9951b8729aee5d8158e3e9d626183a32e55ac against base d9570d1ea9f7744c08df6a74a188c8619aa37ac2 found five blocking issues: precision is lost before exact TopK; indexed scoring depends on RecordBatch fragmentation; an omitted metric can mix cosine and L2 across splits; unbounded query-subvector count expands into unbounded ANN plan width; and unsupported stored null/non-finite elements can be scored plausibly instead of rejected.
I reviewed the complete authoritative 53-file change set, applicable AGENTS/guides, current thread/comment history, and pinned Lance/DataFusion production paths through two bounded convergence rounds. The final live head/base and duplicate fences were rechecked before submission. No additional review focus was supplied. This was static analysis only: I did not run builds or tests or edit repository source. The PR description reports focused tests passed and says the full SQL regression suite was not run.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for two remaining correctness issues on exact head cd8255c.
Critical checkpoints:
- Scoring correctness: direct distance accumulation, complete indexed-child reduction, null/empty handling, refinement ordering, filters, split offsets, and final global TopN were traced. Finite zero-norm cosine data still turns one derived NaN into a whole-stream failure (inline P1).
- Schema/identity: FE JSON -> Thrift v2 -> BE -> Lance-C shape, type, metric, row identity, and work budgets are consistent. The new public C API nevertheless accepts nested Lance field paths that its replacement scorer cannot resolve (inline P2).
- Compatibility/lifecycle: schema-version fencing, smooth-upgrade scheduling, scanner ownership, error/cancellation propagation, patch/cache order, C/C++ ABI, and pinned fixture formats were checked; no additional blocker remained.
- Index and resource safety: mixed indexed/unindexed fragments, metric mismatch fallback, candidate/refinement caps, and offset expansion converge correctly after the existing fixes.
- Coverage/artifacts: duplicate fixtures and catalog compatibility-mode discovery were checked. Existing threads for normalization, batch fragmentation, default metrics, fan-out, and actual null/non-finite primitives were treated as hard duplicate fences and revalidated at this head.
No additional user review focus was supplied. This was a static-only review: the task contract prohibited builds and tests. Two convergence rounds completed, all final-round subagents returned NO_NEW_VALUABLE_FINDINGS, and every candidate was accepted, deduplicated, or dismissed with evidence.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
|
run buildall |
|
/review |
|
Synchronized the follow-up from lance-format/lance-c#83 in ee71f0e:
Local validation: 61 native library tests, 13 native multi-vector integration tests and 52 BE reader tests passed against the rebuilt library. The patch applies cleanly with zero fuzz; shared implementation/test equivalence with the upstream PR was checked. Full local self-review completed before committing. Full SQL regression remains for CI. |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Static review of exact head ee71f0e67229922e28c5a2ee9725306cb532316a against base d9570d1ea9f7744c08df6a74a188c8619aa37ac2 found one remaining correctness issue: documented-valid finite L2/dot operands can overflow the pinned f32 distance kernels, and the replacement scorer turns that row into a whole-batch execution error.
Correctness: traced the FE JSON matrix through Thrift, split planning, BE validation, the C ABI, and Lance exact/indexed/refined/hybrid plans. The prior fixes for score precision, child-stream batching, metric consistency, work bounds, actual invalid stored values, zero-norm cosine, and nested paths are present. The inline P2 below remains distinct.
Compatibility: the optional schema-versioned Thrift fields, direct/stale-request revalidation, resolved metric propagation, and smooth-upgrade source fence are aligned across FE and BE.
Lifecycle and safety: checked scanner ownership, typed query-buffer lifetime, checked multiplication/narrowing, cancellation/error propagation, result materialization, and scan-stat callback wiring; no additional issue was substantiated.
Performance and observability: the 128-subvector and 100000 candidate/refinement budgets are enforced at FE, BE, and C boundaries; indexed child streams are fully reduced before final ranking, and the scanner callback remains connected.
Tests and packaging: reviewed the native, FE, BE, and SQL cases, fixture generator/promotion, patch ordering, and all 91 changed paths. The three BE/Docker fixture pairs are byte-identical. Per the review environment, this was static-only: no builds or tests were run.
Convergence: two normal full-review passes and the separate risk pass returned NO_NEW_VALUABLE_FINDINGS in Round 2. One provisional nullable-Struct concern was dismissed after verifying that pinned Lance pushes ancestor validity into persisted children. The user supplied no additional focus points.
## Summary Expose one multi-vector query through `lance_scanner_nearest_multivector` and the C++ `Scanner::nearest_multivector` wrapper. The API copies a row-major matrix for a `List<FixedSizeList<float16|float32|float64>>` column, preserving the distinction between one multi-vector query and independent batch queries without changing existing API signatures. Rows rank by `sum(query_vector, min(stored_vector, distance))`. L2 is the default across indexed and appended fragments; cosine indexes use exact candidate refinement by default. Null/empty outer rows do not rank. Undefined cosine pairs from zero-norm vectors are ignored; rows without a defined match for any query subvector do not rank. Nested vector columns retain Lance field-path parsing, including backtick-quoted child names. Shape/type checks, finite-value validation and limits on query-vector count and candidate work reject unsupported inputs before expensive plan expansion. The pinned Lance execution plan also needs corrections before its TopK operators: accumulating distances directly preserves small L2 differences, and reducing each ANN child across its complete stream prevents batch boundaries from changing scores and dropping the winning row. A scoped scoring adapter implements those corrections, validates actual stored elements, and restores final distance order before limit/offset after fragment-scoped payload reads. Strict row batching remains outside the final sort and result window, using the batch size resolved by Lance. ANN candidate selection remains approximate. The adapter is isolated so it can be removed when equivalent corrections are available in the Lance dependency. ## Validation - Latest full Rust test run: 389 tests passed; the two opt-in C/C++ compile/run tests were ignored in this run. They passed during earlier feature validation, and this follow-up does not change the headers or ABI. - 14 multi-vector integration tests cover types, metrics, malformed/invalid values, work limits, small-distance Top1, batch-size-independent indexed Top1, partial index coverage, zero-norm cosine rows/queries, and nested/quoted paths. - Strict batching reproducer returned `[1, 2, 1]` before the fix and now returns `[2, 2]`. Added offset/limit, empty/tail windows, default sizing and a plan-level test preserving the resolved strict size. - Nested projection tests check schema, payload values, row counts and distance order across 32 combinations of ordinary/quoted paths, projections, index use and appended fragments. - `cargo fmt --check` and `cargo clippy --all-targets -- -D warnings` passed. - Completed full local self-review before committing. Related consumer feature: apache/doris#68028.
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 19th, 2026 8:24 AM. The selected account is excluded until 2026-09-19T08:24:00Z. Please trigger /review again; another configured account may be available. |
| } | ||
| TExternalSearchRequest searchRequest = new TExternalSearchRequest() | ||
| .setSchemaVersion(1) | ||
| .setSchemaVersion(queryVector.isSetNumVectors() ? 2 : 1) |
There was a problem hiding this comment.
no need change to version to 2 now
There was a problem hiding this comment.
Updated in 80a6aab. Both ordinary and multi-vector requests now use schema version 1; the optional num_vectors field identifies the matrix payload. Removed the multi-vector-specific upgrade fence and its compatibility-only test/documentation. Existing additional-type checks are unchanged. Validation: 31 focused FE tests, 53 Lance BE tests, FE Checkstyle and clang-format 16 passed. The two previously failing SQL suites also passed locally against the CI build of d38fba4; the protocol change was compiled and tested separately with the current FE/BE sources.
| // Pin the planner's default on every split; Lance otherwise inherits an index metric. | ||
| vectorParams.setMetric(params.containsKey(METRIC) ? parseMetric(params.get(METRIC)) : TVectorMetric.L2); | ||
| LanceVectorQuery.validateMultiVectorBudget(queryVector, common.topK(), common.offset(), | ||
| params.containsKey(REFINE_FACTOR) ? parsePositiveInt(params.get(REFINE_FACTOR), REFINE_FACTOR) : 1); |
There was a problem hiding this comment.
REFINE_FACTOR must greater than 0 ?
There was a problem hiding this comment.
Yes. An explicitly supplied refine_factor must be positive: the pinned Lance indexed-search planner rejects Some(0) with 'Refine factor cannot be zero'. Doris already enforced this through parsePositiveInt before this PR. A factor of 1 still re-ranks candidates using original vectors; it does not disable refinement. Multi-vector queries default to 1 to refine scores and validate the stored values.
| public final class LanceVectorQuery { | ||
| // Keep aligned with the BE and Lance-C limits: each subvector expands into an ANN branch. | ||
| public static final int MAX_QUERY_VECTORS = 128; | ||
| public static final long MAX_QUERY_VECTOR_CANDIDATES = 100_000; |
There was a problem hiding this comment.
To limit this in Doris? maybe no need?
There was a problem hiding this comment.
The limit is a resource guard, not an algorithmic requirement. Lance creates an ANN plan branch for each query subvector and overfetches candidates in each branch, so a small input matrix can still expand into substantial planning and execution work. FE validation rejects oversized requests before dispatch; BE and the merged lance-c #83 API also enforce the same bounds. Removing only the Doris check would therefore defer the rejection to the backend. I agree that 128 subvectors and the 100,000 candidate budget are policy choices and that using the same budget for exact and indexed searches is conservative. We can revisit the thresholds and separate the execution paths with workload evidence, but I would retain the aligned checks in this PR rather than remove the protection.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static-only review completed for exact head 80a6aaba1e1e7641d3a5f885c75f123896949c83 against base d9570d1ea9f7744c08df6a74a188c8619aa37ac2. No new non-duplicate inline finding was substantiated.
Critical checkpoints:
- Coverage: reviewed the complete authoritative 11,234-line diff and all 186 changed paths. The 150 binary fixture entries were accounted for through their generators, manifests, and consuming tests.
- Scoring and execution: traced exact, indexed, refined, and hybrid multi-vector paths; full child-stream reduction; metric selection; row-ID reconciliation; filters; split overfetch; final global ordering/windowing; and strict batching. No additional correctness or lifecycle defect survived recheck.
- Schema and compatibility: FE JSON matrix encoding, optional Thrift
num_vectors, schema version 1, BE byte/shape/type validation, Lance-C ABI checks, L2 defaulting, old-caller behavior, and candidate/refinement budgets are aligned. - Dependency and packaging: reviewed the ordered fail-fast Lance patch stack, cache marker behavior, pinned dependency integration, fixture provenance, docs, and native/FE/BE/regression coverage.
- Duplicate fence: current-head thread
4022237009already reports the remaining finite-distance-overflow/whole-batch failure, so it was not reposted. - User focus: no additional review focus was supplied.
This result is complete but static-only and summary-only: the review contract prohibited builds and tests, and no repository source was edited. All three required Round 1 reviewers returned NO_NEW_VALUABLE_FINDINGS; every risk and candidate was accepted, duplicate-fenced, or dismissed with concrete evidence.
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: #67741; consumer integration: #68028 Add merged [lance-format/lance-c#83](lance-format/lance-c#83) to the Lance-C third-party patch chain on master. This provides the C/C++ multi-vector query API and its upstream scoring, validation and regression tests as a prerequisite for Doris multi-vector search integration. The patch records upstream commit `0a30ee6c5a9d1455ceb36f4745acb79e53a00461`. Overlapping context is adapted to the existing patch chain while retaining PR #79's scalar-segment execution path and tests. The multi-vector implementation and integration tests are byte-identical to the merged upstream commit. The release archive and dependency versions remain unchanged from #67741. Apply PR #83 with a separate completion marker so source trees already carrying the existing patches also receive the new API, and repeat runs do not reapply it. Rebuild Lance-C before consuming the new API. This PR changes only third-party patch integration and does not expose new Doris SQL syntax. ### Validation - Applied the actual Lance-C patch-driver block to clean v0.1.9 sources and to sources already patched through #67741; both produce identical source trees. Repeat execution leaves the sources unchanged. All patches apply with `--fuzz=0`. - Compared the resulting source tree with the previously validated integrated chain and checked the multi-vector implementation/tests against the merged upstream commit. - `cargo test --locked`: 410 tests passed, including 14 multi-vector integration tests; three opt-in C/C++ compile/run and static-transport tests were ignored in this run. - `cargo fmt --check` and `bash -n thirdparty/download-thirdparty.sh` passed. - Completed local self-review of patch provenance, conflict adaptation, existing API behavior, ownership/resource bounds and cached-source application before committing. A complete Doris build and SQL regression run are not part of this local dependency-only validation. ### Release note None ### Check List (For Author) - Test - [x] Unit Test - [x] Manual test: patch application, cached-source upgrade, repeat execution and source equivalence - Behavior changed: - [x] Yes: the third-party library gains the upstream multi-vector API; existing API signatures remain unchanged. - Does this need documentation? - [x] No: upstream API documentation is included in the patch. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
3 participants
What problem does this PR solve?
Issue Number: close #66494
vector_searchaccepts one query matrix for a LanceList<FixedSizeList<T, D>>column on branch-4.1, supporting Float16, Float32 and Float64. A matrix such as[[1,0],[0,1]]ranks logical rows containing variable numbers of subvectors. The row score issum(q in query, min(v in row, distance(q, v))); the TVF filter precedes candidate selection, and TopK/offset count rows.The Lance-C adapter computes scores before candidate truncation, preserving small L2 differences and aggregating each ANN query child across all output batches. Indexed candidates undergo exact refinement, and fragment-scoped results regain distance order before limit/offset. ANN candidate selection remains approximate. Omitted metrics resolve consistently to L2 on every split, including partially indexed datasets. The pinned Lance version supports cosine multi-vector indexes; L2/dot use exact search when no compatible index exists.
FE, BE and C API checks bound query width to 128 subvectors and candidate work to 100,000 for both
num_vectors * (top_k + offset)andrefine_factor * (top_k + offset). Actual null/non-finite stored elements encountered by scoring fail execution; empty/null outer rows do not rank. Undefined cosine pairs from zero-norm vectors are ignored, and rows without a defined match for any query subvector are filtered. The adapter resolves nested vector paths and backtick-quoted child names consistently with Lance. The shared result-window code also preserves Lance's strict batching stage after sorting and slicing; the older Doris C binding does not expose a strict-batching setter, so that path is covered by a native execution-plan test without adding an API. Requests retain schema version 1; the optionalnum_vectorsfield distinguishes matrices from ordinary single-vector payloads.The vendored patch adds the matrix API without changing existing C signatures. Following #67741, the third-party build now applies the community patch chain on Lance-C 0.1.9, including the Lance v11 upgrade and merged lance-format/lance-c#83. PR #83's patch records its upstream commit and adapts overlapping context to retain PR #79's scalar-segment execution path; its multi-vector implementation and integration tests match upstream. Rebuild Lance-C: previously installed libraries lack the required API. Re-extract sources carrying the older patch chain; the build rejects stale patch markers instead of silently retaining an old library.
Validation
Completed a full local self-review and corrected the issues found before committing.
The complete
test_lance_multivector_coverageandtest_lance_vector_search_index_typessuites passed locally (2 suites, 0 failures, 0 skips) against the CI build of d38fba4. The schema-version change was compiled and tested separately in the focused FE/BE harnesses; those results are not a full Doris build of the final revision. The complete external regression set remains for CI. Exact equality in the representative index fixtures uses all partitions and sufficient candidate overfetch; it is not a general ANN recall guarantee.Release note
Support multi-vector Lance columns and query matrices in
vector_search, with documented row-level distance, filtering and TopK semantics.Check List (For Author)
docs/lance-multivector-search.md.Check List (For Reviewer who merge this PR)