From 636ab1120256bcbf439fc127eec13f29b47cbfa8 Mon Sep 17 00:00:00 2001 From: Jack Date: Mon, 3 Aug 2026 20:24:10 +0800 Subject: [PATCH] [fix](be) Key the index lookup map by each surviving index in remove_index (#66316) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: `TabletSchema::remove_index` rebuilds the `(index_type, col_unique_id, index_suffix) -> position` lookup map after dropping an entry. The loop walks each surviving index, but builds every map key from `_indexes.back()` instead of the entry being registered: ```cpp for (size_t new_pos = 0; new_pos < _indexes.size(); ++new_pos) { const auto& index = _indexes[new_pos]; ... IndexKey key = std::make_tuple(_indexes.back()->index_type(), col_uid, _indexes.back()->get_index_suffix()); _col_id_suffix_to_index[key].push_back(new_pos); } ``` So every survivor is filed under the LAST survivor's index type and suffix. When the survivors are homogeneous -- e.g. several INVERTED indexes with no suffix, which is what the existing coverage uses -- every key is identical and the result is accidentally correct. When they differ, it is not: after dropping one of two INVERTED indexes on a table that also carries an NGRAM_BF index, the surviving INVERTED index is filed under `(NGRAM_BF, col, "")`, and `inverted_indexs()` -- which looks up `IndexType::INVERTED` -- no longer finds it. A surviving index then becomes invisible to callers that resolve indexes through this map, including the segment writer and index compaction, with no error reported. The same applies to indexes that differ only in suffix (variant sub-column indexes). The fix keys each entry by its own `index_type()` / `get_index_suffix()`. This also makes `remove_index` consistent with the other three sites that populate the same map (`append_index`, `init_from_pb`, and the column-append path): those iterate right after `_indexes.emplace_back(...)`, where `_indexes.back()` IS the entry being registered, so they are correct as written and are left unchanged. The map is a runtime cache rebuilt from scratch by `init_from_pb`, so a schema that round-trips through protobuf is unaffected; only an in-memory schema that keeps being used after `remove_index` sees the stale mapping. --- be/src/storage/tablet/tablet_schema.cpp | 4 +- be/test/storage/tablet/tablet_schema_test.cpp | 56 +++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/be/src/storage/tablet/tablet_schema.cpp b/be/src/storage/tablet/tablet_schema.cpp index eefdfb31f6a908..0dff2a8bc5a55f 100644 --- a/be/src/storage/tablet/tablet_schema.cpp +++ b/be/src/storage/tablet/tablet_schema.cpp @@ -942,8 +942,8 @@ void TabletSchema::remove_index(int64_t index_id) { auto& pattern_to_index_map = _index_by_unique_id_with_pattern[col_uid]; pattern_to_index_map[field_pattern].emplace_back(index); } else { - IndexKey key = std::make_tuple(_indexes.back()->index_type(), col_uid, - _indexes.back()->get_index_suffix()); + IndexKey key = + std::make_tuple(index->index_type(), col_uid, index->get_index_suffix()); _col_id_suffix_to_index[key].push_back(new_pos); } } diff --git a/be/test/storage/tablet/tablet_schema_test.cpp b/be/test/storage/tablet/tablet_schema_test.cpp index e2b8424e8d602d..f988c3feda184d 100644 --- a/be/test/storage/tablet/tablet_schema_test.cpp +++ b/be/test/storage/tablet/tablet_schema_test.cpp @@ -334,6 +334,62 @@ TEST_F(TabletSchemaTest, test_tablet_column_protobuf_roundtrip) { deserialized.variant_enable_typed_paths_to_sparse()); } +// remove_index() rebuilds the (index_type, col_uid, suffix) -> position lookup +// map after dropping an entry. Every surviving index must be filed under ITS OWN +// key. The existing coverage below uses three INVERTED indexes with no suffix, +// where every key is identical, so it cannot tell a correct rebuild from one +// that keys every entry off the LAST surviving index. +// +// This pins the heterogeneous case: after the drop the survivors are an +// INVERTED index followed by an NGRAM_BF index. If the rebuild takes the key +// from _indexes.back(), the INVERTED index is filed under (NGRAM_BF, ...) and +// inverted_indexs() -- which looks up IndexType::INVERTED -- stops seeing it, +// making a surviving index invisible to the segment writer and to compaction. +TEST_F(TabletSchemaTest, test_remove_index_keeps_heterogeneous_survivors_findable) { + TabletSchema schema; + + TabletColumn text_col; + text_col.set_unique_id(9001); + text_col.set_name("text_col"); + text_col.set_type(FieldType::OLAP_FIELD_TYPE_STRING); + schema.append_column(text_col); + + TabletColumn code_col; + code_col.set_unique_id(9002); + code_col.set_name("code_col"); + code_col.set_type(FieldType::OLAP_FIELD_TYPE_STRING); + schema.append_column(code_col); + + auto add_index = [&](int64_t index_id, IndexType type, int32_t col_uid) { + TabletIndex index; + TabletIndexPB index_pb; + index_pb.set_index_id(index_id); + index_pb.set_index_name("hetero_idx_" + std::to_string(index_id)); + index_pb.set_index_type(type); + index_pb.add_col_unique_id(col_uid); + index.init_from_pb(index_pb); + schema.append_index(std::move(index)); + }; + + add_index(500, IndexType::INVERTED, 9001); // survives + add_index(501, IndexType::INVERTED, 9001); // dropped below + add_index(502, IndexType::NGRAM_BF, 9002); // survives, and becomes back() + + ASSERT_EQ(2, schema.inverted_indexs(9001, "").size()); + + schema.remove_index(501); + + // The surviving INVERTED index must still be reachable by its own key. + auto survivors = schema.inverted_indexs(9001, ""); + ASSERT_EQ(1, survivors.size()) + << "surviving INVERTED index became invisible after remove_index; the lookup map was " + "rebuilt using the last survivor's (index_type, suffix) instead of each entry's own"; + EXPECT_EQ(500, survivors[0]->index_id()); + + // The NGRAM_BF survivor must not be reported as an inverted index on its column. + EXPECT_TRUE(schema.inverted_indexs(9002, "").empty()); +} + TEST_F(TabletSchemaTest, test_tablet_schema_remove_and_clear_index) { TabletSchema schema;