github-actions[bot] commented on code in PR #61596:
URL: https://github.com/apache/doris/pull/61596#discussion_r2969824019
##########
be/src/storage/tablet/tablet_schema.cpp:
##########
@@ -1105,6 +1105,48 @@ void TabletSchema::remove_index(int64_t index_id) {
}
}
+void TabletSchema::reorder_indexes_by(const TabletSchemaSPtr&
reference_schema) {
+ // Build a position map from the reference schema's index ordering.
+ // Indexes present in the reference schema are sorted to match its order;
+ // indexes not in the reference schema are placed at the end, preserving
+ // 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) {
Review 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.
##########
be/test/storage/tablet/tablet_schema_index_test.cpp:
##########
@@ -237,4 +237,62 @@ TEST_F(TabletSchemaIndexTest,
TestIsSameExceptIdWithSameId) {
EXPECT_TRUE(index1.is_same_except_id(&index2));
}
+TEST_F(TabletSchemaIndexTest, TestReorderIndexesBy) {
+ // Simulate the build index scenario: indexes appended in different order
Review 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.
--
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]
