mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 10:15:43 +00:00
fix: build text index for newly added enable_match field on growing segment reopen (#50484) (#51201)
## Summary After schema evolution adds an `enable_match` field (e.g. `drop_collection_field` + `add_collection_field` of a same-name text field), `text_match` could fail with `text index not found for field <id>` on a growing segment even though `describe_index` reported `Finished`. ## Root cause A growing segment builds its per-segment text indexes exactly once, in the constructor (`CreateTextIndexes`), against the construction-time schema — `text_indexes_` has a single writer, reachable only from that path. When a query carrying the new schema drives `LazyCheckSchema -> Reopen(SchemaPtr)`, Reopen only backfilled empty column data (`fill_empty_field`) and never built the text index for the newly added field — unlike the sealed path, where both load and schema-only reopen go through `ComputeDiff -> text_indexes_to_create -> CreateTextIndex`. So an old growing segment upgraded by Reopen had no `text_indexes_[new_field]`, and `text_match` hit `GetTextIndex()` and threw `TextIndexNotFound`. The window closes once the segment is flushed/sealed, loaded (sealed load rebuilds the index) and released from the delegator, which is why the failure is intermittent and self-healing but hard, non-retryable while it lasts. ## Fix In `SegmentGrowingImpl::Reopen(SchemaPtr)`, for newly added `IsStringDataType && enable_match` fields, **before publishing `schema_`** (LazyCheckSchema readers don't take `sch_mutex_`, so a concurrent query observing the new schema skips Reopen and queries immediately — the index must already exist by then): 1. Build the text index via the new `BuildTextIndexForMeta(const FieldMeta&)` (the field is not yet in the published `schema_`, so the meta is passed in; mirrors sealed's `CreateTextIndexWithSchema`). The index is **staged as a local object** — it is not visible in `text_indexes_` yet. 2. **Index the pre-existing rows with the same default-value-or-nulls fill the column received** (constructed directly — the content is knowable without reading the column back, since the exclusive `sch_mutex_` blocks concurrent inserts). This keeps the index consistent with the column, matching sealed's create-from-raw behavior (`BulkRawStringAt -> AddTextSealed`): when the added field carries a `default_value`, old rows must match text queries against the default text and report not-null. Backfilling every row (nulls included) also keeps text-index doc offsets dense and aligned with `insert_record_` rows, which query-side bitmap sizing relies on. 3. Force `Commit()+Reload()` on each staged index; after **every** new field succeeds, publish all staged indexes into `text_indexes_` together (an O(1) map insert per field under one `mutex_` acquisition). Publish-after-success makes the failure path sound across fields as well as within one: a present `text_indexes_` entry is always a fully backfilled, committed index; a throw anywhere during any field's build/backfill/commit publishes nothing and leaves `schema_` un-advanced, so the next query rebuilds everything from scratch (no per-field partial publication that a retry's presence guard would skip over). It also shrinks the `mutex_` hold during Reopen from the O(N) backfill to the final map insert, so concurrent `GetTextIndex` readers (text_match on existing fields) are not stalled by the backfill. Function-output fields (e.g. BM25 sparse output) remain skipped: they are non-string and never carry a text index. The related-but-distinct `field index meta not found` symptom for BM25 function-output fields (follow-up of #50783: `index_meta_` is not updated on growing reopen) is a separate fix and not covered here. ## Test `test_schema_reopen.cpp`: - `ReopenBuildsTextIndexForNewEnableMatchField`: growing segment on schema v1, old rows inserted, Reopen to v2 adding a nullable `enable_match` VARCHAR (no default) — `GetTextIndex` throws before Reopen; after Reopen it returns the index, `MatchQuery` finds nothing and `IsNotNull` reports all rows null, consistent with the column. - `ReopenTextIndexIndexesDefaultValueForOldRows`: same flow with a `default_value` on the added field — after Reopen `MatchQuery("default")` matches all N pre-existing rows and `IsNotNull` reports them non-null, matching sealed semantics. - `ReopenBuildsTextIndexesForMultipleNewFields`: one Reopen adds two enable_match fields (one nullable-null, one with default) — both indexes come out complete through the batched staged publish. All tests query immediately after Reopen with no explicit `Commit()/Reload()`, asserting the forced-visibility behavior. ## Known limitation (pre-existing, follow-up) `EnsureArrayOffsetsForStructField` still runs after `schema_` is published, so a throw there can leave a schema-advanced segment without array offsets — pre-existing behavior, unchanged by this PR; reworking Reopen's publication order for non-text state is left as a follow-up. (The text-index half of this concern is resolved: the index is now built before publication and a build failure leaves the reopen retryable.) issue: #50484 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: MrPresent-Han <chun.han@gmail.com> Co-authored-by: MrPresent-Han <chun.han@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
MrPresent-Han
Claude Opus 4.8
parent
88f357a345
commit
1993feaeea
@@ -2381,8 +2381,15 @@ SegmentGrowingImpl::CreateTextIndex(FieldId field_id,
|
||||
CheckCancellation(
|
||||
op_ctx, id_, field_id.get(), "SegmentGrowingImpl::CreateTextIndex()");
|
||||
|
||||
auto index = BuildTextIndexForMeta(schema_->operator[](field_id));
|
||||
std::unique_lock lock(mutex_);
|
||||
const auto& field_meta = schema_->operator[](field_id);
|
||||
text_indexes_[field_id] = std::move(index);
|
||||
}
|
||||
|
||||
// Builds a standalone index without publishing it into text_indexes_; the
|
||||
// meta is passed in because Reopen runs before the field is in schema_.
|
||||
std::unique_ptr<index::TextMatchIndex>
|
||||
SegmentGrowingImpl::BuildTextIndexForMeta(const FieldMeta& field_meta) {
|
||||
AssertInfo(IsStringDataType(field_meta.get_data_type()),
|
||||
"cannot create text index on non-string type");
|
||||
std::string unique_id = GetUniqueFieldId(field_meta.get_id().get());
|
||||
@@ -2397,7 +2404,7 @@ SegmentGrowingImpl::CreateTextIndex(FieldId field_id,
|
||||
index->CreateReader(milvus::index::SetBitsetGrowing);
|
||||
index->RegisterAnalyzer("milvus_tokenizer",
|
||||
field_meta.get_analyzer_params().c_str());
|
||||
text_indexes_[field_id] = std::move(index);
|
||||
return index;
|
||||
}
|
||||
|
||||
void
|
||||
@@ -2552,9 +2559,52 @@ SegmentGrowingImpl::Reopen(SchemaPtr sch) {
|
||||
fill_empty_field(field_meta);
|
||||
}
|
||||
|
||||
auto row_count = insert_record_.row_count();
|
||||
// #50484: build text indexes for new enable_match fields BEFORE
|
||||
// publishing schema_ -- readers skip Reopen once the new schema_ is
|
||||
// visible. Backfill every pre-existing row (default value or nulls,
|
||||
// same as fill_empty_field): doc offsets must stay dense and aligned
|
||||
// with insert_record_ rows. Commit+Reload so the triggering query
|
||||
// sees the backfill. Stage all fields locally and publish together
|
||||
// only after every one succeeds: a present text_indexes_ entry is
|
||||
// always complete, and a mid-build throw publishes nothing (schema_
|
||||
// un-advanced, so the next query rebuilds from scratch).
|
||||
std::vector<std::pair<FieldId, std::unique_ptr<index::TextMatchIndex>>>
|
||||
staged_text_indexes;
|
||||
for (const auto& field_meta : *absent_fields) {
|
||||
if (sch->is_function_output(field_meta.get_id())) {
|
||||
continue;
|
||||
}
|
||||
auto field_id = field_meta.get_id();
|
||||
if (IsStringDataType(field_meta.get_data_type()) &&
|
||||
field_meta.enable_match() &&
|
||||
text_indexes_.find(field_id) == text_indexes_.end()) {
|
||||
auto index = BuildTextIndexForMeta(field_meta);
|
||||
if (row_count > 0) {
|
||||
const bool has_default =
|
||||
field_meta.default_value().has_value();
|
||||
std::vector<std::string> texts(
|
||||
row_count,
|
||||
has_default ? field_meta.default_value()->string_data()
|
||||
: std::string());
|
||||
FixedVector<bool> texts_valid_data(row_count, has_default);
|
||||
index->AddTextsGrowing(
|
||||
row_count, texts.data(), texts_valid_data.data(), 0);
|
||||
}
|
||||
index->Commit();
|
||||
index->Reload();
|
||||
staged_text_indexes.emplace_back(field_id, std::move(index));
|
||||
}
|
||||
}
|
||||
if (!staged_text_indexes.empty()) {
|
||||
std::unique_lock lock(mutex_);
|
||||
for (auto& [field_id, index] : staged_text_indexes) {
|
||||
text_indexes_[field_id] = std::move(index);
|
||||
}
|
||||
}
|
||||
|
||||
schema_ = sch;
|
||||
|
||||
auto row_count = insert_record_.row_count();
|
||||
for (const auto& field_meta : *absent_fields) {
|
||||
if (sch->is_function_output(field_meta.get_id())) {
|
||||
continue;
|
||||
|
||||
@@ -776,6 +776,9 @@ class SegmentGrowingImpl : public SegmentGrowing {
|
||||
void
|
||||
CreateTextIndexes();
|
||||
|
||||
std::unique_ptr<index::TextMatchIndex>
|
||||
BuildTextIndexForMeta(const FieldMeta& field_meta);
|
||||
|
||||
/**
|
||||
* @brief Initialize TEXT LOB spillover files for each TEXT field
|
||||
*
|
||||
|
||||
@@ -119,3 +119,168 @@ TEST_F(SchemaReopenTest, LoadWithAbsentNullableVectorFieldShouldReadAllNull) {
|
||||
ASSERT_FALSE(col->valid_data(i)) << "row " << i << " should be null";
|
||||
}
|
||||
}
|
||||
|
||||
// #50484: Reopen must build the text index for an enable_match field added
|
||||
// by schema evolution and index the pre-existing rows (nulls here);
|
||||
// otherwise text_match throws TextIndexNotFound.
|
||||
TEST_F(SchemaReopenTest, ReopenBuildsTextIndexForNewEnableMatchField) {
|
||||
// V1 has no text field, so the constructor builds no text index.
|
||||
auto segment = CreateGrowingSegment(schema_v1_, milvus::empty_index_meta);
|
||||
auto* seg_impl = dynamic_cast<SegmentGrowingImpl*>(segment.get());
|
||||
ASSERT_NE(seg_impl, nullptr);
|
||||
|
||||
int N = 20;
|
||||
auto dataset = DataGen(schema_v1_, N, /*seed=*/7);
|
||||
auto reserved = segment->PreInsert(N);
|
||||
segment->Insert(reserved,
|
||||
N,
|
||||
dataset.row_ids_.data(),
|
||||
dataset.timestamps_.data(),
|
||||
dataset.raw_);
|
||||
ASSERT_EQ(segment->get_row_count(), N);
|
||||
|
||||
// V2 shares V1's field ids and adds a nullable enable_match VARCHAR.
|
||||
auto schema_v2 = std::make_shared<Schema>();
|
||||
schema_v2->AddDebugField(
|
||||
"vec", DataType::VECTOR_FLOAT, 128, knowhere::metric::L2);
|
||||
auto pk_fid = schema_v2->AddDebugField("pk", DataType::INT64);
|
||||
std::map<std::string, std::string> analyzer_params;
|
||||
auto text_fid = schema_v2->AddDebugVarcharField(FieldName("text_content"),
|
||||
DataType::VARCHAR,
|
||||
/*max_length=*/65535,
|
||||
/*nullable=*/true,
|
||||
/*enable_match=*/true,
|
||||
/*enable_analyzer=*/true,
|
||||
analyzer_params,
|
||||
std::nullopt);
|
||||
schema_v2->set_primary_field_id(pk_fid);
|
||||
schema_v2->set_schema_version(2);
|
||||
|
||||
milvus::OpContext op_ctx;
|
||||
EXPECT_ANY_THROW(seg_impl->GetTextIndex(&op_ctx, text_fid));
|
||||
|
||||
seg_impl->Reopen(schema_v2);
|
||||
|
||||
ASSERT_NO_THROW(seg_impl->GetTextIndex(&op_ctx, text_fid));
|
||||
auto pw = seg_impl->GetTextIndex(&op_ctx, text_fid);
|
||||
auto* index = pw.get();
|
||||
ASSERT_NE(index, nullptr);
|
||||
|
||||
// No explicit Commit/Reload: Reopen already made the backfill visible.
|
||||
// No default value -> all rows null, nothing matches.
|
||||
EXPECT_EQ(index->MatchQuery("anything", 1).count(), 0);
|
||||
auto not_null = index->IsNotNull();
|
||||
ASSERT_EQ(not_null.size(), static_cast<size_t>(N));
|
||||
EXPECT_EQ(not_null.count(), 0);
|
||||
}
|
||||
|
||||
// #50484: when the added enable_match field has a default value, Reopen's
|
||||
// backfill must index the default text for pre-existing rows, matching
|
||||
// sealed's create-from-raw results.
|
||||
TEST_F(SchemaReopenTest, ReopenTextIndexIndexesDefaultValueForOldRows) {
|
||||
auto segment = CreateGrowingSegment(schema_v1_, milvus::empty_index_meta);
|
||||
auto* seg_impl = dynamic_cast<SegmentGrowingImpl*>(segment.get());
|
||||
ASSERT_NE(seg_impl, nullptr);
|
||||
|
||||
int N = 20;
|
||||
auto dataset = DataGen(schema_v1_, N, /*seed=*/11);
|
||||
auto reserved = segment->PreInsert(N);
|
||||
segment->Insert(reserved,
|
||||
N,
|
||||
dataset.row_ids_.data(),
|
||||
dataset.timestamps_.data(),
|
||||
dataset.raw_);
|
||||
ASSERT_EQ(segment->get_row_count(), N);
|
||||
|
||||
auto schema_v2 = std::make_shared<Schema>();
|
||||
schema_v2->AddDebugField(
|
||||
"vec", DataType::VECTOR_FLOAT, 128, knowhere::metric::L2);
|
||||
auto pk_fid = schema_v2->AddDebugField("pk", DataType::INT64);
|
||||
std::map<std::string, std::string> analyzer_params;
|
||||
DefaultValueType default_value;
|
||||
default_value.set_string_data("sample default text");
|
||||
auto text_fid =
|
||||
schema_v2->AddDebugVarcharField(FieldName("text_content"),
|
||||
DataType::VARCHAR,
|
||||
/*max_length=*/65535,
|
||||
/*nullable=*/true,
|
||||
/*enable_match=*/true,
|
||||
/*enable_analyzer=*/true,
|
||||
analyzer_params,
|
||||
std::make_optional(default_value));
|
||||
schema_v2->set_primary_field_id(pk_fid);
|
||||
schema_v2->set_schema_version(2);
|
||||
|
||||
seg_impl->Reopen(schema_v2);
|
||||
|
||||
milvus::OpContext op_ctx;
|
||||
auto pw = seg_impl->GetTextIndex(&op_ctx, text_fid);
|
||||
auto* index = pw.get();
|
||||
ASSERT_NE(index, nullptr);
|
||||
|
||||
// No explicit Commit/Reload: every old row carries the default text.
|
||||
EXPECT_EQ(index->MatchQuery("default", 1).count(), static_cast<size_t>(N));
|
||||
EXPECT_EQ(index->MatchQuery("absent-token", 1).count(), 0);
|
||||
auto not_null = index->IsNotNull();
|
||||
ASSERT_EQ(not_null.size(), static_cast<size_t>(N));
|
||||
EXPECT_EQ(not_null.count(), static_cast<size_t>(N));
|
||||
}
|
||||
|
||||
// #50484: one Reopen may add several enable_match fields; the staged indexes
|
||||
// are published together, so every field must come out complete.
|
||||
TEST_F(SchemaReopenTest, ReopenBuildsTextIndexesForMultipleNewFields) {
|
||||
auto segment = CreateGrowingSegment(schema_v1_, milvus::empty_index_meta);
|
||||
auto* seg_impl = dynamic_cast<SegmentGrowingImpl*>(segment.get());
|
||||
ASSERT_NE(seg_impl, nullptr);
|
||||
|
||||
int N = 20;
|
||||
auto dataset = DataGen(schema_v1_, N, /*seed=*/13);
|
||||
auto reserved = segment->PreInsert(N);
|
||||
segment->Insert(reserved,
|
||||
N,
|
||||
dataset.row_ids_.data(),
|
||||
dataset.timestamps_.data(),
|
||||
dataset.raw_);
|
||||
ASSERT_EQ(segment->get_row_count(), N);
|
||||
|
||||
auto schema_v2 = std::make_shared<Schema>();
|
||||
schema_v2->AddDebugField(
|
||||
"vec", DataType::VECTOR_FLOAT, 128, knowhere::metric::L2);
|
||||
auto pk_fid = schema_v2->AddDebugField("pk", DataType::INT64);
|
||||
std::map<std::string, std::string> analyzer_params;
|
||||
auto null_fid = schema_v2->AddDebugVarcharField(FieldName("text_null"),
|
||||
DataType::VARCHAR,
|
||||
/*max_length=*/65535,
|
||||
/*nullable=*/true,
|
||||
/*enable_match=*/true,
|
||||
/*enable_analyzer=*/true,
|
||||
analyzer_params,
|
||||
std::nullopt);
|
||||
DefaultValueType default_value;
|
||||
default_value.set_string_data("sample default text");
|
||||
auto default_fid =
|
||||
schema_v2->AddDebugVarcharField(FieldName("text_default"),
|
||||
DataType::VARCHAR,
|
||||
/*max_length=*/65535,
|
||||
/*nullable=*/true,
|
||||
/*enable_match=*/true,
|
||||
/*enable_analyzer=*/true,
|
||||
analyzer_params,
|
||||
std::make_optional(default_value));
|
||||
schema_v2->set_primary_field_id(pk_fid);
|
||||
schema_v2->set_schema_version(2);
|
||||
|
||||
seg_impl->Reopen(schema_v2);
|
||||
|
||||
milvus::OpContext op_ctx;
|
||||
auto null_pw = seg_impl->GetTextIndex(&op_ctx, null_fid);
|
||||
ASSERT_NE(null_pw.get(), nullptr);
|
||||
EXPECT_EQ(null_pw.get()->MatchQuery("anything", 1).count(), 0);
|
||||
EXPECT_EQ(null_pw.get()->IsNotNull().count(), 0);
|
||||
|
||||
auto default_pw = seg_impl->GetTextIndex(&op_ctx, default_fid);
|
||||
ASSERT_NE(default_pw.get(), nullptr);
|
||||
EXPECT_EQ(default_pw.get()->MatchQuery("default", 1).count(),
|
||||
static_cast<size_t>(N));
|
||||
EXPECT_EQ(default_pw.get()->IsNotNull().count(), static_cast<size_t>(N));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user