Doris-Breakwater commented on issue #67117:
URL: https://github.com/apache/doris/issues/67117#issuecomment-5407361652

   Breakwater-GitHub-Analysis-Slot: slot_9e82f2ff694b
   
   ## Initial triage
   
   **Verdict: confirmed FE index-planning bug.** The result remains correct 
through the fragment/Flat Search fallback, but index use—and therefore latency 
and resource consumption—incorrectly depends on `describeIndices()` ordering.
   
   I verified the report against the current `upstream/branch-4.1` 
implementation at `3bfc343de0c496597a59a8377ff2043339987ba8`. The issue 
currently has no labels, assignee, milestone, or prior comments. Suggested 
existing labels are `area/catalog`, `kind/performance`, and 
`kind/need-regression-test`.
   
   ### Verified facts
   
   - `LanceMetadataLoader.loadVectorIndexSegments()` preserves every physical 
segment's logical index name, field IDs, UUID, fragment bitmap, and the 
normalized `metric_type` from its `IndexDescription`. The FE therefore already 
has enough information to choose among same-column logical indexes.
   - `LanceScanNode.selectIndexSegments()` sets `selectedIndexName` from the 
first segment covering `vectorFieldId` and then retains only segments with that 
name. `createIndexSegmentSplits()` calls `metricMatches()` only after this 
irreversible selection. Thus an earlier L2 index prevents a later cosine index 
from being considered for a cosine query.
   - When that metric check fails, `createIndexSegmentSplits()` returns empty 
and `getSplits()` creates fragment splits; the index counters remain zero. This 
matches the reported `EXPLAIN` behavior.
   - The BE path already serializes `TLanceFileDesc.index_segment_uuids` into 
`lance_scanner_set_index_segments()`. Selection by logical `index_name` is not 
required for this fix: routing only the chosen logical index's physical UUIDs 
is sufficient.
   - The requested-metric resolver already treats an omitted metric and 
`TVectorMetric.DEFAULT` as L2. Missing metric metadata and missing fragment 
bitmaps already cause conservative fallback.
   - Existing FE coverage tests one logical index with multiple physical 
segments and a single-index metric mismatch, but it does not cover multiple 
logical indexes on the same field. The open test PR #67039 also explicitly uses 
one vector column per matrix cell because of this limitation, independently 
corroborating the gap.
   
   No runtime profile or additional user logs are needed to establish this root 
cause; the control flow is deterministic. I did not run an end-to-end Lance 
reproduction, so physical execution still needs to be proven by the regression 
test requested below.
   
   ### Recommended implementation
   
   1. Resolve the requested metric once using the existing Doris semantics.
   2. Group target-field segments by logical index name before selecting a 
candidate.
   3. Evaluate each complete group for metric compatibility **and safe 
plannability**. If a group has an unknown metric, a missing fragment bitmap, or 
produces no visible segment split, continue to the next candidate instead of 
allowing that group to block a later safe match.
   4. Choose among multiple safe field+metric matches with an order-independent 
policy. A minimal policy is lexicographically smallest logical index name; if 
maintainers prefer best coverage, compare visible covered fragments/rows first 
and use the name as a stable tie-breaker. Whichever rule is chosen should be 
documented.
   5. Pass only that group's physical UUIDs to the existing split planner and 
retain the existing Flat Search splits for visible fragments not covered by the 
selected index.
   6. Preserve whole-query conservative fallback when no logical group can be 
planned safely. No BE or Thrift API change appears necessary.
   
   The important detail is to choose a **safe plan**, not merely the first 
metric-compatible group: otherwise a compatible legacy group with no fragment 
bitmap could still hide a later fully usable group.
   
   ### Validation expected in the fix
   
   - FE unit test: `[L2 index, cosine index]` with a cosine query selects only 
the cosine UUIDs.
   - Repeat with reversed metadata order and assert the same selected 
UUIDs/splits.
   - Two same-field, same-metric logical indexes: assert the documented 
deterministic winner and prove UUIDs from different names are never mixed.
   - No compatible metric: retain fragment splits and zero index-segment 
planning.
   - Test both an unset metric and explicit `DEFAULT`; each must select the L2 
group.
   - Legacy/unsafe candidate tests: an unknown metric or missing fragment 
bitmap falls back when alone, but does not block a later safe compatible 
logical index.
   - Add a dedicated regression fixture with two named indexes on one vector 
column. Assert non-zero indexed segment planning in `EXPLAIN`, then use an 
`nprobes` or `ef` discriminator that demonstrably differs from Flat Search; 
indexed-vs-flat result equality alone cannot prove physical index use.
   - Coordinate the documentation change with apache/doris-website#4082: remove 
the "one vector index per column" workaround once the fix is available, while 
retaining the metric-default and legacy-fallback documentation.
   
   ### Maintainer decisions still needed
   
   - Target branch and any backport scope are not stated in the issue.
   - Select and document the tie-breaker for multiple safe indexes with the 
same field and metric.
   - Decide whether the dedicated multi-index regression fixture belongs in 
this fix or in the currently open matrix-test work, while keeping the FE unit 
coverage in the fix itself.
   


-- 
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]

Reply via email to