[fix](lance) Select metric-compatible vector index - #67553
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
74eab3f to
df7c762
Compare
|
run buildall |
|
@zhangstar333 PTAL, thx |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review status: complete after 2 of at most 3 rounds; every Round 2 normal and risk-focused pass returned NO_NEW_VALUABLE_FINDINGS. Reviewed exact head df7c762.
Requesting changes for one test-compilation blocker and two explicit issue-completeness gaps.
Critical checkpoints:
- Build/test correctness: the ten added test fixtures omit the required IndexType constructor argument, so the modified test class cannot compile and the new assertions cannot run.
- Selection correctness: the production grouping and lexicographic first-safe-plan logic is internally consistent; rejected plans are isolated, one logical index's UUIDs are routed, and uncovered visible fragments retain flat-search splits.
- Metric and compatibility paths: omitted/DEFAULT L2 plus explicit L2/COSINE/DOT_PRODUCT/HAMMING agree across request parsing, metadata normalization, FE selection, and BE setup. NORMAL and FULL_TEXT paths are not changed by the new vector selector.
- Regression evidence: issue #67117's required physical same-column multi-index EXPLAIN plus nprobes/ef discriminator is still missing; the added tests exercise injected metadata only.
- Documentation: current English/Chinese 4.x docs and in-repository fixture comments still describe the removed one-index-only limitation instead of the new deterministic selection policy.
- Security, concurrency, lifecycle, and performance: no additional distinct regression was substantiated.
- User focus: no additional focus was provided; the full PR was reviewed.
No build was run, as this review runner explicitly prohibits builds. Existing review context was refreshed immediately before submission and contained no prior reviews or inline comments.
| Arrays.asList(new LanceFragmentInfo(1, 8, 8), new LanceFragmentInfo(2, 7, 7)), | ||
| Collections.singletonMap("vector", 9), | ||
| Arrays.asList( | ||
| new LanceIndexSegmentInfo(l2Segment, "a_l2", Collections.singletonList(9), |
There was a problem hiding this comment.
[P1] Supply the required index type in these fixtures
LanceIndexSegmentInfo has only the six-argument constructor ending in (IndexType, String), but this and the other nine added calls pass the metric as the fifth argument. fe-core test compilation therefore fails before any new regression assertion can run; the reported mvn validate phase only runs Checkstyle. Add IndexType.VECTOR before each "L2"/"COSINE" metric argument (including the null-bitmap case), then run a phase that reaches test compilation.
| assertSplit(splits.get(1), 2, 8, 88); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
[P2] Add the required physical multi-index regression
These cases construct LanceTableMetadata directly, so they bypass Dataset.describeIndices(), LanceMetadataLoader, scan-range UUID serialization, and lance-c execution. Issue #67117 explicitly requires a same-column two-index fixture that proves nonzero indexed planning in EXPLAIN and uses an nprobes/ef discriminator; the existing matrix fixture still deliberately keeps one index per vector column. Please add that production-path regression so this fix's core scenario is exercised beyond the helper-level plan.
| List<LanceIndexSegmentInfo> indexSegments, int fieldId) { | ||
| List<LanceIndexSegmentInfo> selectedSegments = new ArrayList<>(); | ||
| String selectedIndexName = null; | ||
| // A stable order keeps index selection independent of Lance metadata ordering. |
There was a problem hiding this comment.
[P2] Update the documented selection contract
This now selects the lexicographically first safely plannable metric match, but the current English and Chinese 4.x Lance catalog docs still say Doris considers only one vector index per column and recommend creating at most one. Issue #67117 also makes removing that guidance and documenting a deterministic policy an acceptance criterion, while the in-repo matrix fixture comments still describe the old first-index limitation. Please update those docs/comments (or link the companion documentation change) so users are not told the opposite of the new behavior.
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
### What problem does this PR solve? Issue Number: close apache#67117 Related PR: apache#67553 Problem Summary: The Lance fixture comment said that only the first vector index on a column was reachable. Metric-compatible selection now groups physical segments by logical index name and considers later compatible groups, so that explanation was stale. Describe the fixture matrix as deliberately using one index per column for independent compatibility cells and point same-column coverage to the FE metadata test. ### Release note None ### Check List (For Author) - Test: git diff --check; python3 -m py_compile docker/thirdparties/docker-compose/iceberg/scripts/lance_build_preinstalled_catalog.py - Behavior changed: No - Does this need documentation: Yes (separate website documentation follow-up needed)
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
|
The current CI failures ( Image change
Both runs tested the same commit FE UT (build 1043001) All 17 non-muted failures are in After that, the remaining Lance tests fail with check_coverage_fe (build 1042299) The container prints Other PRs are hitting the same Could someone with CI access take a look? Rolling the tag back to the |
@FANNG1 |
FE UT Coverage ReportIncrement line coverage |
|
@FANNG1 the code conflicts, could rebase upstream for merge |
Issue Number: close apache#67117 Related PR: apache#67039 Problem Summary: LanceScanNode previously committed to the first logical index returned for a vector field before checking its metric. A query whose requested metric matched a later same-column index therefore fell back to Flat Search. Group segments by logical index name, inspect groups in stable lexical name order, and select the first metric-compatible group that can safely produce indexed splits. Legacy or invisible groups no longer prevent a later safe candidate from being used. Vector searches can use a later same-column Lance index when it matches the requested metric instead of unnecessarily falling back to Flat Search. - Test: Maven validate -pl fe-core -am -DskipTests (passed); LanceScanNodeTest added. Targeted ./run-fe-ut.sh --run org.apache.doris.datasource.lance.source.LanceScanNodeTest was blocked before fe-core by generated Thrift source incompatibility from the local prebuilt thirdparty package. - Behavior changed: Yes (metric-compatible same-column indexes are now selected deterministically) - Does this need documentation: Yes (follow up in apache/doris-website#4082)
### What problem does this PR solve? Issue Number: close apache#67117 Related PR: None Problem Summary: The same-metric selection test previously covered one physical segment per logical index only. It could not prove that selecting the lexicographically first logical index preserves every one of its physical segments without mixing UUIDs from another same-metric index. Model the selected index with two segments and a later index covering all fragments, then assert the selected group\x27s UUIDs and the remaining flat fragment split. ### Release note None ### Check List (For Author) - Test: FE Checkstyle validation; targeted FE unit test blocked before fe-core by historical macOS thirdparty Thrift generation incompatibility - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#67117 Related PR: apache#67039 Problem Summary: The same-column vector index selection tests used the pre-full-text-search LanceIndexSegmentInfo constructor and no longer compiled after IndexType became required. They also exercised only injected FE metadata, leaving the real fixture, metadata loader, scan-range serialization, and backend index path untested. Supply IndexType.VECTOR to every affected fixture and add a two-index same-column Lance dataset with L2 created before cosine. The regression verifies that a cosine query plans indexed segments and that nprobes changes the result relative to Flat Search. ### Release note None ### Check List (For Author) - Test: Python syntax check passed for lance_build_preinstalled_catalog.py; git diff --check passed; targeted ./run-fe-ut.sh --run org.apache.doris.datasource.lance.source.LanceScanNodeTest blocked before fe-core because thirdparty/installed/bin/protoc is missing - Behavior changed: No - Does this need documentation: Yes (follow-up website documentation update required)
### What problem does this PR solve? Issue Number: close apache#67117 Related PR: apache#67039 Problem Summary: External Regression provisions its Lance warehouse from a prebuilt fixture rather than rebuilding lance_build_preinstalled_catalog.py from the PR checkout. The newly added same-column multi-index table was therefore absent in CI, causing test_lance_vector_search_metrics to fail before it could exercise index selection. Remove the unavailable external fixture and its dependent assertions. Keep the same-column selection coverage in LanceScanNodeTest, whose metadata fixtures are built in the FE test process, and clarify that the external matrix intentionally uses one index per column. ### Release note None ### Check List (For Author) - Test: git diff --check; python3 -m py_compile docker/thirdparties/docker-compose/iceberg/scripts/lance_build_preinstalled_catalog.py - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#67117 Related PR: apache#67553 Problem Summary: The Lance fixture comment said that only the first vector index on a column was reachable. Metric-compatible selection now groups physical segments by logical index name and considers later compatible groups, so that explanation was stale. Describe the fixture matrix as deliberately using one index per column for independent compatibility cells and point same-column coverage to the FE metadata test. ### Release note None ### Check List (For Author) - Test: git diff --check; python3 -m py_compile docker/thirdparties/docker-compose/iceberg/scripts/lance_build_preinstalled_catalog.py - Behavior changed: No - Does this need documentation: Yes (separate website documentation follow-up needed)
829c628 to
eb80283
Compare
|
run buildall |
### What problem does this PR solve? Issue Number: close #67117 Related PR: None Problem Summary: When multiple Lance vector indexes exist on the same column, Doris previously selected the first physical index segment before checking its metric. If that logical index used a different metric, Doris fell back to flat search without trying a later compatible index. Group physical index segments by logical index name, visit logical indexes in lexicographic name order, filter each complete group by the requested metric, and select the first group that can be planned safely. If no compatible usable group exists, preserve the existing flat-search fallback. This keeps every physical segment of one logical index together and never mixes segment UUIDs from different logical indexes. ### Release note Fix Lance vector index selection when multiple indexes with different metrics exist on the same column. ### Check List (For Author) - Test: Unit Test / FE validation - `mvn validate -pl fe-core -am -DskipTests` passed, including Checkstyle. - Added `LanceScanNodeTest` cases for metric matching, metadata-order independence, lexicographic logical-index selection, DEFAULT-to-L2 behavior, unsafe-group skipping, and multi-segment UUID grouping. - `./run-fe-ut.sh --run org.apache.doris.datasource.lance.source.LanceScanNodeTest` was attempted but blocked before `fe-core`: the macOS prebuilt thirdparty Thrift compiler generates Java incompatible with this historical branch, causing `fe-common` compilation errors. - Behavior changed: Yes. Doris now tries metric-compatible logical Lance indexes in lexicographic index-name order before falling back to flat search. - Does this need documentation: No
4 participants
What problem does this PR solve?
Issue Number: close #67117
Related PR: None
Problem Summary: When multiple Lance vector indexes exist on the same column, Doris previously selected the first physical index segment before checking its metric. If that logical index used a different metric, Doris fell back to flat search without trying a later compatible index.
Group physical index segments by logical index name, visit logical indexes in lexicographic name order, filter each complete group by the requested metric, and select the first group that can be planned safely. If no compatible usable group exists, preserve the existing flat-search fallback. This keeps every physical segment of one logical index together and never mixes segment UUIDs from different logical indexes.
Release note
Fix Lance vector index selection when multiple indexes with different metrics exist on the same column.
Check List (For Author)
mvn validate -pl fe-core -am -DskipTestspassed, including Checkstyle.LanceScanNodeTestcases for metric matching, metadata-order independence, lexicographic logical-index selection, DEFAULT-to-L2 behavior, unsafe-group skipping, and multi-segment UUID grouping../run-fe-ut.sh --run org.apache.doris.datasource.lance.source.LanceScanNodeTestwas attempted but blocked beforefe-core: the macOS prebuilt thirdparty Thrift compiler generates Java incompatible with this historical branch, causingfe-commoncompilation errors.