lib/segment/src/types.rs (1)

1858-1883: ⚠️ Potential issue

Remove auto-inference of field index type.

This code segment should be removed as it's no longer required with the new explicit schema compatibility approach. The automatic type inference mechanism has been replaced by explicit schema compatibility checks throughout the system.

When inferring types from JSON values, inconsistencies could arise if the index was interrupted during creation, leading to the startup panics described in the PR objectives.

The function value_type appears to still be in the codebase but should be removed as part of this PR. Removing this auto-inference mechanism is critical to resolving the startup panic issue described in the PR objectives.

-pub fn value_type(value: &Value) -> Option<PayloadSchemaType> {
-    match value {
-        Value::Null => None,
-        Value::Bool(_) => None,
-        Value::Number(num) => {
-            if num.is_i64() {
-                Some(PayloadSchemaType::Integer)
-            } else if num.is_f64() {
-                Some(PayloadSchemaType::Float)
-            } else {
-                None
-            }
-        }
-        Value::String(_) => Some(PayloadSchemaType::Keyword),
-        Value::Array(_) => None,
-        Value::Object(obj) => {
-            let lon_op = obj.get("lon").and_then(|x| x.as_f64());
-            let lat_op = obj.get("lat").and_then(|x| x.as_f64());
-
-            if let (Some(_), Some(_)) = (lon_op, lat_op) {
-                return Some(PayloadSchemaType::Geo);
-            }
-            None
-        }
-    }
-}

lib/segment/src/index/plain_payload_index.rs (1)

76-83: ⚠️ Potential issue

build_index never marks the new schema as applied

build_index always returns AlreadyBuilt, which short‑cuts the apply_field_index call in the upper layers.
For PlainPayloadIndex this means the segment’s indexed_fields map is never updated after
drop_index_if_incompatible removed an old (incompatible) schema, leaving the segment
without any record of the new (compatible) schema.

This breaks consistency guarantees – other segments will report the field as indexed while
plain segments silently ignore it, and SegmentEntry::get_indexed_fields() will give
conflicting answers.

A minimal fix is to return Built with an empty vector so the normal apply_field_index
path persists the schema:

-    ) -> OperationResult<BuildIndexResult> {
-        Ok(BuildIndexResult::AlreadyBuilt) // No index to build
+    ) -> OperationResult<BuildIndexResult> {
+        // no on‑disk index, but we must notify the caller that the schema needs
+        // to be persisted in `indexed_fields`
+        Ok(BuildIndexResult::Built(Vec::new()))
     }

lib/collection/src/collection_manager/holders/proxy_segment.rs (1)

1199-1214: ⚠️ Potential issue

Index creation is not recorded → wrapped segment will never learn about the new index

build_field_index successfully builds the index inside write_segment, but does not add a ProxyIndexChange::Create entry to changed_indexes.
Consequently propagate_to_wrapped() (lines 270‑301) will skip the CREATE step and the wrapped segment will continue to run without the index, defeating the whole purpose of building it in the proxy.

@@ fn build_field_index(&self, op_num: SeqNumberType, key: PayloadKeyTypeRef, field_type: &PayloadFieldSchema, hw_counter: &HardwareCounterCell) -> OperationResult<BuildFieldIndexResult> {
-        let field_index = match self
+        let field_index = match self
             .payload_index
             .borrow()
             .build_index(key, field_type, hw_counter)?
         {
             BuildIndexResult::Built(indexes) => indexes,
@@
-        Ok(BuildFieldIndexResult::Built {
-            indexes: field_index,
-            schema: field_type.clone(),
-        })
+        // Record the change so it gets propagated later
+        self.changed_indexes
+            .write()
+            .insert(key.clone(), ProxyIndexChange::Create(field_type.clone(), op_num));
+
+        Ok(BuildFieldIndexResult::Built {
+            indexes: field_index,
+            schema: field_type.clone(),
+        })
 }

Without this, index schemas will diverge again after optimisation or restart.

lib/segment/src/index/plain_payload_index.rs (1)

121-129: Over‑aggressive drop in drop_index_if_incompatible

Always removing the entry even when it is compatible is safe but loses the optimisation of
avoiding an unnecessary save_config().
(You already do this optimisation in StructPayloadIndex.)

-        // Just always drop the index, as we don't have any indexes
-        self.config.indexed_fields.remove(field);
-        self.save_config()
+        if let Some(current) = self.config.indexed_fields.get(field) {
+            if current == _new_payload_schema {
+                return Ok(()); // schema matches – nothing to do
+            }
+        }
+        self.config.indexed_fields.remove(field);
+        self.save_config()

lib/collection/src/collection_manager/segments_updater.rs (1)

347-358: Consider propagating the “index was dropped” signal

delete_field_index_if_incompatible returns bool, but the result is ignored.
Forwarding it would let the caller know whether a rebuild was triggered by an actual
schema change, which could be useful for logging or telemetry:

-            write_segment.with_upgraded(|segment| {
-                segment.delete_field_index_if_incompatible(op_num, field_name, field_schema)
-            })?;
+            let index_dropped = write_segment.with_upgraded(|segment| {
+                segment.delete_field_index_if_incompatible(op_num, field_name, field_schema)
+            })?;

and later include index_dropped in the return value if desired.

lib/collection/src/collection_manager/holders/proxy_segment.rs (1)

1255-1261: get_indexed_fields removes entries on schema mismatch but never adds updated schema

When an existing schema differs, the field is simply removed from the map.
Down‑stream components cannot tell whether the field is un‑indexed or indexed with the new schema.

Return the new schema instead:

-    if let Some(existing_schema) = indexed_fields.get(field_name) {
-        if existing_schema != schema {
-            indexed_fields.remove(field_name);
-        }
-    }
+    match indexed_fields.get_mut(field_name) {
+        Some(existing) if existing != schema => *existing = schema.clone(),
+        None => { /* ignore – index was never there */ }
+        _ => {}
+    }

This keeps the caller’s view consistent with what delete_field_index_if_incompatible achieved.

lib/segment/src/index/payload_index_base.rs (2)

18-26: Derive common traits for BuildIndexResult

The enum is passed around (and sometimes compared) but does not implement Debug, Clone, or PartialEq.
Deriving them eases logging, testing, and pattern matching.

-pub enum BuildIndexResult {
+#[derive(Debug, Clone, PartialEq)]
+pub enum BuildIndexResult {

No run‑time impact, just better ergonomics.

---

59-65: Provide a default no‑op implementation for drop_index_if_incompatible

All index types are now forced to implement this even if they never create per‑field indexes (e.g. the simple payload index).
Adding a default implementation keeps the trait backwards‑compatible and reduces boilerplate:

fn drop_index_if_incompatible(
    &mut self,
    _field: PayloadKeyTypeRef,
    _new_payload_schema: &PayloadFieldSchema,
) -> OperationResult<()> {
    Ok(())
}

Implementers that need special logic can still override it.

lib/segment/src/segment/entry.rs (2)

21-22: Two similarly‑named result types are imported – consider an alias to avoid confusion

use crate::data_types::build_index_result::BuildFieldIndexResult;
use crate::index::{BuildIndexResult, …};

Developers reading the file must constantly remember which one is which. A simple alias clarifies intent:

use crate::index::{BuildIndexResult as PayloadBuildResult, PayloadIndex, VectorIndex};

and update the references accordingly.

---

768-787: Return path on IncompatibleSchema lacks guidance

The method bubbles up BuildFieldIndexResult::IncompatibleSchema but gives no hint on what the caller should do next.
Consider extending the doc‑comment (or the enum variant) to state that the caller is expected to invoke
delete_field_index_if_incompatible and retry the build.

Small, but improves DX and makes the control‑flow obvious.

lib/segment/src/index/struct_payload_index.rs (3)

41-41: PayloadContainer appears to be unused – consider removing

The extra import generates a compiler warning (unused import: 'PayloadContainer') in strict #![deny(warnings)] builds.

-    PayloadContainer, PayloadFieldSchema, PayloadKeyType, PayloadKeyTypeRef, VectorNameBuf,
+    PayloadFieldSchema, PayloadKeyType, PayloadKeyTypeRef, VectorNameBuf,

---

514-542: set_indexed writes configuration twice and contains an unreachable branch

1. drop_index_if_incompatible → drop_index already persists PayloadConfig.
2. The happy‑path (BuildIndexResult::Built) calls apply_index, which saves the config again.
   That is two disk writes for one logical operation.
3. After drop_index_if_incompatible, the IncompatibleSchema variant can no longer occur, so the error branch is effectively dead code.

You could streamline the method and avoid redundant I/O:

-        self.drop_index_if_incompatible(field, &payload_schema)?;
+        let schema_was_dropped =
+            self.drop_index_if_incompatible(field, &payload_schema)?;

-        let field_index = match self.build_index(field, &payload_schema, hw_counter)? {
+        let field_index = match self.build_index(field, &payload_schema, hw_counter)? {
             BuildIndexResult::Built(field_index) => field_index,
             BuildIndexResult::AlreadyBuilt => {
-                // Index already built, no need to do anything
+                if schema_was_dropped {
+                    // Should be impossible – defensive check
+                    return Err(OperationError::service_error(
+                        "index dropped but BuildIndex reported AlreadyBuilt",
+                    ));
+                }
                 return Ok(());
             }
             BuildIndexResult::IncompatibleSchema => unreachable!(),
         };

Where drop_index_if_incompatible returns a bool indicating whether it actually removed anything.
This eliminates double‑flushes and clarifies control‑flow.

---

558-571: drop_index_if_incompatible triggers a full config flush even when unnecessary

self.drop_index(field)? persists the updated config every time schemas differ.
Immediately afterwards, set_indexed will rebuild the index and again call apply_index, causing a second save_config().

Consider:

• Return a flag (bool) to indicate that an index was removed, allowing the caller to decide when to persist.
• Or move save_config() out of drop_index and call it once at a higher level.

Reducing superfluous RocksDB flushes will shorten index‑rebuild latency under heavy write load.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro