github-actions[bot] commented on code in PR #67553:
URL: https://github.com/apache/doris/pull/67553#discussion_r3968607917
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/lance/source/LanceScanNodeTest.java:
##########
@@ -421,6 +421,134 @@ public void
testExternalSearchFallsBackToFragmentSplitsForMetricMismatch() throw
assertSplit(splits.get(1), 2, 8, 88);
}
+ @Test
+ public void testExternalSearchSelectsLaterMetricCompatibleIndex() throws
Exception {
+ UUID l2Segment =
UUID.fromString("11111111-1111-1111-1111-111111111111");
+ UUID cosineSegment =
UUID.fromString("22222222-2222-2222-2222-222222222222");
+ LanceTableMetadata metadata = LanceTableMetadata.withIndexSegments(
+ "s3://bucket/table.lance",
+ 42,
+ vectorSchema(),
+ 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),
Review 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.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/lance/source/LanceScanNodeTest.java:
##########
@@ -421,6 +421,134 @@ public void
testExternalSearchFallsBackToFragmentSplitsForMetricMismatch() throw
assertSplit(splits.get(1), 2, 8, 88);
}
+ @Test
Review 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.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/source/LanceScanNode.java:
##########
@@ -385,22 +388,17 @@ private List<Split>
createFullTextIndexSegmentSplits(LanceTableMetadata metadata
return plan.buildSplits();
}
- private static List<LanceIndexSegmentInfo> selectVectorIndexSegments(
+ private static List<List<LanceIndexSegmentInfo>>
selectVectorIndexSegmentGroups(
List<LanceIndexSegmentInfo> indexSegments, int fieldId) {
- List<LanceIndexSegmentInfo> selectedSegments = new ArrayList<>();
- String selectedIndexName = null;
+ // A stable order keeps index selection independent of Lance metadata
ordering.
Review 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.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
