unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
milvus-io/milvus
mid7/10
data-structureserror-handlingtesting-strategy

fix: invalidate stale offsets for re-added struct array fields [3.0]

Project: milvus-io/milvus | Language: C++ | Concepts: data-structures, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/milvus-io/milvus/pull/52921

The problem

When a struct array field is dropped and then re-added (with the same name but different field IDs) during schema evolution, the system could incorrectly reuse old array offset data. This stale data could cause incorrect query results or crashes, because the offsets no longer match the new schema's structure.

Why it's non-trivial

A naive approach might only look at field names or types, not at the continuity of field IDs or the actual structure of the schema. Since the schema flattening process loses parent-child relationships, it's not obvious which offsets are still valid. Simply reusing or dropping offsets based on field names could lead to subtle bugs, especially in cases where a struct is dropped and then re-added with new children.

The approach

1. The core logic: invalidating stale offsets

A new static method is introduced to handle the invalidation of stale struct array offsets. It checks for continuity of child field IDs under the same struct name, and only preserves offsets for structs that have at least one surviving child field.

+void
+ChunkedSegmentSealedImpl::InvalidateStaleStructArrayOffsets(
+    const SchemaPtr& current_schema,
+    const SchemaPtr& target_schema,
+    RuntimeResourceState& runtime) {
+    if (runtime.struct_to_array_offsets.empty()) {
+        return;
+    }
+
+    // A StructArray generation survives schema evolution when at least one of
+    // its child field IDs survives under the same struct name.  This preserves
+    // offsets for ordinary schema changes while distinguishing a dropped and
+    // re-added StructArray whose name is reused with entirely new field IDs.
+    // The flattened C++ Schema does not retain the StructArray parent field ID,
+    // so child-ID continuity is the available generation marker here.
+    std::unordered_set<std::string> surviving_structs;
+    for (const auto& [field_id, target_field_meta] :
+         target_schema->get_fields()) {
+        if (!current_schema->has_field(field_id)) {
+            continue;
+        }
+
+        auto current_struct_name =
+            GetStructNameForArrayField(current_schema->operator[](field_id));
+        auto target_struct_name = GetStructNameForArrayField(target_field_meta);
+        if (current_struct_name.has_value() &&
+            current_struct_name == target_struct_name) {
+            surviving_structs.emplace(*target_struct_name);
+        }
+    }
+
+    for (auto it = runtime.struct_to_array_offsets.begin();
+         it != runtime.struct_to_array_offsets.end();) {
+        if (surviving_structs.find(it->first) == surviving_structs.end()) {
+            it = runtime.struct_to_array_offsets.erase(it);
+        } else {
+            ++it;
+        }
+    }
+}

What this does:

  • Checks if there are any struct array offsets to process.
  • Finds struct names that have at least one child field ID surviving between the current and target schema.
  • Removes offsets for any struct name that does not have such continuity, ensuring that offsets for dropped-and-readded structs (with new children) are not reused.

2. Integrating the logic into schema reopen and reload

The invalidation logic is called during schema reloads and reopens, ensuring that runtime state is always consistent with the latest schema.

@@ -7402,6 +7443,7 @@ ChunkedSegmentSealedImpl::ReopenSchemaLocked(milvus::OpContext* op_ctx,
         "Schema-only reopen segment {} with diff {}", id_, diff.ToString());
 
     auto next_runtime = CloneMutableRuntimeResourceState();
+    InvalidateStaleStructArrayOffsets(current_schema, sch, *next_runtime);
     auto staged = ClonePublishedState(current);
     staged->schema = sch;
     staged->load_info = std::make_shared<const SegmentLoadInfo>(new_local);

@@ -7477,6 +7521,10 @@ ChunkedSegmentSealedImpl::Reopen(
     LOG_INFO("Reopen segment {} with diff {}", id_, diff.ToString());
 
     auto next_runtime = CloneMutableRuntimeResourceState();
+    if (has_schema_update) {
+        InvalidateStaleStructArrayOffsets(
+            current_schema, target_schema, *next_runtime);
+    }
     auto staged = ClonePublishedState(current);
     staged->schema = target_schema;
     staged->load_info = std::make_shared<const SegmentLoadInfo>(new_local);

What this does:

  • Ensures that stale offsets are purged whenever the schema is updated, preventing accidental reuse of offsets for structurally different fields.

3. Testing the fix

A targeted test is added to simulate the bug scenario: dropping and re-adding a struct array field with the same name but different field IDs.

+// #52877: lazy reopen may skip the drop-only schema and see the old and new
+// StructArrays as one same-name transition with disjoint child field IDs.
+TEST(test_chunk_segment,
+     ReopenInvalidatesOffsetsForReaddedStructArrayWithSameName) {
+    ...
+    // Model lazy reopen skipping the intermediate drop-only schema: the struct
+    // name is unchanged, while every child field ID belongs to a new generation.
+    segment->Reopen(new_schema);
+
+    auto new_offsets = segment->GetArrayOffsets(new_label);
+    ASSERT_NE(new_offsets, nullptr);
+    EXPECT_NE(new_offsets.get(), old_offsets.get());
+    EXPECT_EQ(new_offsets.get(), segment->GetArrayOffsets(new_score).get());
+    EXPECT_EQ(new_offsets->GetRowCount(), row_count);
+    EXPECT_EQ(new_offsets->GetTotalElementCount(), 0);
+    EXPECT_EQ(segment->GetArrayOffsets(old_label), nullptr);
+    EXPECT_EQ(segment->GetArrayOffsets(old_score), nullptr);
+}

What this does:

  • Loads data with an original schema.
  • Drops and re-adds struct array fields with the same names but new field IDs.
  • Reopens the segment with the new schema.
  • Asserts that the old offsets are not reused, and the new offsets are fresh and empty.

From the review thread

No notable review discussion on this PR.

Takeaways

  • When managing runtime state tied to schema structure, always ensure data is invalidated or migrated when schema changes break the continuity of field IDs or structure, not just names or types.
  • Relying on field names alone is insufficient for schema evolution; use unique identifiers (like field IDs) to track true continuity.
  • Automated, scenario-driven tests are essential for catching subtle bugs that only occur during complex schema transitions.