[fix](inverted index) Make select_best_reader deterministic for multi-index columns#61596
[fix](inverted index) Make select_best_reader deterministic for multi-index columns#61596airborne12 wants to merge 3 commits intoapache:masterfrom
Conversation
…sults with multiple analyzers
### What problem does this PR solve?
Issue Number: close #DORIS-24685
Problem Summary:
When a table has multiple inverted indexes with different analyzers on the same
column, and these indexes are built via sequential BUILD INDEX operations with
data inserts interleaved between builds, the query results become inconsistent
with a baseline table that has the same indexes defined at creation time.
Root cause: BUILD INDEX appends new indexes to the output rowset schema via
`append_index()`, so successive builds produce index ordering that depends on
execution order (e.g., [idx_ch3, idx_ch2]) rather than matching the tablet
schema order (e.g., [idx_ch2, idx_ch3]). The search function's
`select_best_reader()` returns the first matching candidate for REGEXP and
other query types, so different index ordering across segments causes different
indexes to be selected for the same query, producing inconsistent results.
Fix: After appending new indexes in `IndexBuilder::update_inverted_index_info()`,
reorder the output rowset schema's indexes to match the tablet schema's order
via a new `TabletSchema::reorder_indexes_by()` method.
### Release note
Fixed a bug where BUILD INDEX with multiple analyzers on the same column could
produce query results inconsistent with tables where indexes were defined at
creation time. The issue was caused by index ordering in rowset schemas diverging
from the tablet schema after sequential build index operations.
### Check List (For Author)
- Test: Regression test / Unit Test
- Added regression test: test_build_index_multi_analyzer_order
- Added unit tests: TestReorderIndexesBy, TestReorderIndexesByWithExtraIndexes
- Behavior changed: No
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
PR Goal: Fix BUILD INDEX producing inconsistent query results when multiple inverted indexes with different analyzers exist on the same column. The root cause is that select_best_reader() returns the first matching candidate, and successive BUILD INDEX operations append indexes in execution order rather than tablet schema order, causing different segments to select different indexes for the same query.
Fix approach: Add TabletSchema::reorder_indexes_by() to sort indexes in the output rowset schema to match the tablet schema's ordering after appending new indexes.
The fix is well-targeted and the approach is correct. The implementation correctly uses stable_sort to preserve relative order of indexes not in the reference schema, and properly rebuilds lookup caches after reordering. Both unit tests and regression tests are included.
Critical Checkpoint Conclusions
-
Goal achievement: Yes, the fix correctly addresses the root cause. The regression test proves correctness by comparing BUILD INDEX results against a baseline table.
-
Modification scope: Small, focused, and clear. Only adds one new method and one call site.
-
Concurrency: Not applicable -
reorder_indexes_by()operates on a localoutput_rs_tablet_schemathat is not shared across threads at the point of the call. -
Lifecycle management: No special lifecycle concerns.
-
Configuration items: None added.
-
Incompatible changes: None - existing rowset schemas with inconsistent ordering will continue to work; only newly built rowsets will have corrected ordering.
-
Parallel code paths:
CloudIndexChangeCompaction::rebuild_tablet_schema()incloud_index_change_compaction.cpphas a similar pattern of clearing and appending indexes from a list. It may benefit from the same reordering fix, but is lower risk since the ordering comes from FE's index list rather than multiple sequential BUILD INDEX operations. Worth noting but not blocking. -
Special conditional checks: The
std::numeric_limits<size_t>::max()sentinel for unknown indexes is appropriate - ensures unknown indexes sort to the end. -
Test coverage: Good - two unit tests (basic reorder + stale index) and a comprehensive regression test. See inline suggestion for additional mixed-type test coverage.
-
Observability: Not needed for this fix.
-
Transaction/persistence: Not applicable.
-
Data writes: Not directly applicable - this only reorders in-memory schema metadata before rowset creation.
-
FE-BE variable passing: Not applicable.
-
Performance:
reorder_indexes_by()usesstable_sort(O(n log n)) and a full cache rebuild (O(n * m) where m is col_unique_ids per index). For typical index counts (<100), this is negligible and occurs once per rowset during BUILD INDEX.
Issues Found
See inline comments for two suggestions (non-blocking).
| // their relative order. | ||
| std::unordered_map<int64_t, size_t> ref_order; | ||
| const auto& ref_indexes = reference_schema->inverted_indexes(); | ||
| for (size_t i = 0; i < ref_indexes.size(); ++i) { |
There was a problem hiding this comment.
Suggestion (non-blocking): inverted_indexes() only returns indexes with IndexType::INVERTED, but _indexes stores ALL index types (BLOOMFILTER, NGRAM_BF, ANN, BITMAP, INVERTED). The stable_sort on line 16 sorts the entire _indexes vector, so any non-INVERTED indexes (e.g., NGRAM_BF, BLOOMFILTER) will receive std::numeric_limits<size_t>::max() and be pushed to the end of _indexes.
In the current call site (index_builder.cpp), output_rs_tablet_schema is copy_from() of the input rowset schema, which may contain NGRAM_BF or BLOOMFILTER indexes. After reorder_indexes_by(), these would move from their original positions (potentially interleaved with INVERTED indexes) to the end of the vector.
Since the lookup caches are correctly rebuilt, this doesn't cause functional incorrectness. However, it's worth considering using reference_schema->indexes() (all indexes) instead of inverted_indexes() for the reference ordering, to maintain the relative positions of ALL index types, not just inverted ones. Alternatively, the sort could be scoped to only inverted indexes in the vector.
| } | ||
|
|
||
| TEST_F(TabletSchemaIndexTest, TestReorderIndexesBy) { | ||
| // Simulate the build index scenario: indexes appended in different order |
There was a problem hiding this comment.
Suggestion (non-blocking): Consider adding a test case with mixed index types (e.g., INVERTED + NGRAM_BF or BLOOMFILTER) to verify that reorder_indexes_by() correctly handles schemas containing non-INVERTED indexes. Currently both test cases use only IndexType::INVERTED, but in production, output_rs_tablet_schema from copy_from() can contain BLOOMFILTER and NGRAM_BF indexes as well.
…-index columns
### What problem does this PR solve?
Issue Number: close #DORIS-24685
Problem Summary:
When multiple inverted indexes with different analyzers exist on the same
column, select_best_reader() returns the first matching candidate based on
iteration order of _reader_entries. Since _reader_entries ordering depends
on the rowset schema's index ordering, and different segments can have
different orderings (e.g. after sequential BUILD INDEX operations), the
same query selects different indexes for different segments, producing
inconsistent results.
Fix: Replace all order-dependent candidate selection in select_for_text(),
select_for_numeric(), and select_best_reader() with deterministic selection
by smallest index_id via pick_preferred() and pick_smallest_index_id()
helpers. This ensures consistent index selection regardless of schema
ordering across segments.
### Release note
Fixed a bug where queries on columns with multiple inverted indexes could
return inconsistent results after BUILD INDEX operations, due to
non-deterministic index selection across segments with different schema
orderings.
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason
- Behavior changed:
- [x] No.
- [ ] Yes.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
airborne12
changed the title
|
run buildall |
TPC-H: Total hot run time: 26980 ms |
TPC-DS: Total hot run time: 169227 ms |
…patibility Master merged apache#61142 which removed the vectorized namespace. Update references in inverted_index_iterator to use DataTypePtr/DataTypeString directly to fix BE UT compilation failure on CI.
|
run buildall |
TPC-H: Total hot run time: 26707 ms |
TPC-DS: Total hot run time: 169627 ms |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
4 participants
What problem does this PR solve?
Issue Number: close #DORIS-24685
Related PR: N/A
Problem Summary:
When multiple inverted indexes with different analyzers exist on the same column,
select_best_reader()returns the first matching candidate based on iteration order of_reader_entries. Since_reader_entriesordering depends on the rowset schema's index ordering, and different segments can have different orderings (e.g. after sequentialBUILD INDEXoperations), the same query selects different indexes for different segments, producing inconsistent results.Fix: Replace all order-dependent candidate selection in
select_for_text(),select_for_numeric(), andselect_best_reader()with deterministic selection by smallestindex_idviapick_preferred()andpick_smallest_index_id()helpers. This ensures consistent index selection regardless of schema ordering across segments.Release note
Fixed a bug where queries on columns with multiple inverted indexes could return inconsistent results after BUILD INDEX operations, due to non-deterministic index selection across segments with different schema orderings.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)