lib/segment/src/index/field_index/map_index/tests.rs (1)

477-501: ⚡ Quick win

Add behavior assertions to the round-trip swap test.

int_map_immutable_to_mmap_round_trip_preserves_files currently proves byte-for-byte file stability, but not query/value correctness after each swap. Please also assert representative get_iterator/values_count outputs before and after both transitions to catch in-memory rebuild regressions.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/field_index/map_index/tests.rs` around lines 477 - 501,
The test int_map_immutable_to_mmap_round_trip_preserves_files currently only
checks file snapshots and swap state; add assertions to validate query
correctness by calling MapIndex methods like get_iterator and values_count on
the loaded index (from data saved via save_map_index/load_map_index) before the
first swap, after swapping to Mmap, after the no-op swap, and after swapping
back to Immutable: pick a few representative keys from the test data (e.g., keys
corresponding to values 1, 4, and 6) and assert that values_count(key) matches
expected counts and that iterating via get_iterator yields the expected Value
items for those keys each time so that in-memory rebuilds preserve semantic
results across swap_on_disk(true/false) transitions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

lib/segment/src/index/field_index/field_index_base/field_index.rs (1)

249-252: ⚡ Quick win

Update stale phase comment to match current behavior.

Line 249–252 says every arm returns Ok(false), but bool/null swaps now return Ok(true). Please adjust this doc to avoid misleading future changes.

Proposed doc-only diff

-    /// Phase 1 scaffolding: every arm returns `Ok(false)` so the wiring is
-    /// in place but the fast path never fires. Each family's real
-    /// implementation will replace its arm in later phases (numeric → map →
-    /// geo → full-text).
+    /// Some families perform real in-place swaps already (for example,
+    /// schema-only families); others still return `Ok(false)` until their
+    /// swap implementation is added.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/field_index/field_index_base/field_index.rs` around
lines 249 - 252, Update the Phase 1 scaffolding doc comment in field_index.rs
(the "Phase 1 scaffolding" block near FieldIndexBase) to reflect current
behavior: instead of saying every arm returns `Ok(false)`, note that most arms
return `Ok(false)` but bool/null swap arms now return `Ok(true)` (so the fast
path can fire for those cases); rephrase to avoid misleading future maintainers
and mention that concrete family implementations will replace these arms in
later phases.

lib/segment/src/index/field_index/map_index/tests.rs (1)

477-500: ⚡ Quick win

Strengthen swap regression coverage with behavioral assertions.

Please also assert read behavior after each swap direction (and add the swap_on_disk(false) false-path check for mutable) so this test guards semantics, not only file-byte stability.

Proposed test additions

     assert!(index.swap_on_disk(true).unwrap());
     assert!(matches!(index, MapIndex::Mmap(_)));
     assert_eq!(snapshot_files(&index), files_before);
+    let hw_counter = HardwareCounterCell::new();
+    let mut hits: Vec<PointOffsetType> = index.get_iterator(&4, &hw_counter).collect();
+    hits.sort();
+    assert_eq!(hits, vec![1, 2]);

     // No-op
     assert!(index.swap_on_disk(true).unwrap());

     assert!(index.swap_on_disk(false).unwrap());
     assert!(matches!(index, MapIndex::Immutable(_)));
     assert_eq!(snapshot_files(&index), files_before);
+    let hw_counter = HardwareCounterCell::new();
+    let mut hits: Vec<PointOffsetType> = index.get_iterator(&4, &hw_counter).collect();
+    hits.sort();
+    assert_eq!(hits, vec![1, 2]);
@@
     assert!(matches!(index, MapIndex::Mutable(_)));
     assert!(!index.swap_on_disk(true).unwrap());
     assert!(matches!(index, MapIndex::Mutable(_)));
+    assert!(!index.swap_on_disk(false).unwrap());
+    assert!(matches!(index, MapIndex::Mutable(_)));

Also applies to: 503-519

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/field_index/map_index/tests.rs` around lines 477 - 500,
Add behavioral assertions to
int_map_immutable_to_mmap_round_trip_preserves_files: after loading via
load_map_index and after every swap_on_disk(true/false) call, verify that
reading from the index returns the expected values for the original `data` (use
the same accessors used elsewhere in tests to read mapped values); also add the
missing false-path check for swap_on_disk(false) on the mutable-to-mmap
transition to ensure it returns Ok(false) when no-op is expected. Reference the
test helpers save_map_index, load_map_index, snapshot_files and the MapIndex
variants (MapIndex::Immutable, MapIndex::Mmap) and call swap_on_disk on the
`index` variable, asserting both the boolean return and that lookups into the
`index` match `data` after each swap.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

lib/segment/src/index/struct_payload_index/mod.rs (1)

348-363: ⚠️ Potential issue | 🟠 Major

Rollback the in-memory schema when save_config() fails.

update_indexed_schema() mutates self.config.indices before persistence. If save_config() errors, later retries in the same process can see the field as already transitioned and return AlreadyBuilt, while the on-disk config is still stale.

Minimal fix

     let index_types: Vec<_> = self
         .field_indexes
         .get(field)
         .map(|indexes| indexes.iter().map(|i| i.get_full_index_type()).collect())
         .unwrap_or_default();
-    self.config.indices.insert(
-        field.clone(),
-        PayloadFieldSchemaWithIndexType::new(new_schema, index_types),
-    );
-    self.save_config()?;
+    let next = PayloadFieldSchemaWithIndexType::new(new_schema, index_types);
+    let prev = self.config.indices.insert(field.clone(), next);
+    if let Err(err) = self.save_config() {
+        match prev {
+            Some(prev) => {
+                self.config.indices.insert(field.clone(), prev);
+            }
+            None => {
+                self.config.indices.remove(field);
+            }
+        }
+        return Err(err);
+    }
     Ok(())

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/struct_payload_index/mod.rs` around lines 348 - 363,
update_indexed_schema currently mutates self.config.indices before calling
save_config, causing a stale on-disk state if save_config fails; capture the
previous entry (via the insert return or get) for the given field, perform the
insert of PayloadFieldSchemaWithIndexType(new_schema, index_types), call
save_config(), and if save_config() returns Err restore the prior in-memory
value (re-insert the previous value or remove the key if it did not exist) and
propagate the error so callers don’t see a half-applied in-memory state
(referencing update_indexed_schema, self.config.indices,
PayloadFieldSchemaWithIndexType, save_config and the AlreadyBuilt semantics).

lib/segment/src/index/field_index/geo_index/immutable_geo_index/lifecycle.rs (1)

22-25: ⚡ Quick win

Use explicit OperationError::from(err) conversions here.

These new err.into() calls violate the repo rule for explicit conversions in Rust.

As per coding guidelines, "Prefer explicit conversions: use SomeType::from(x) instead of implicit x.into() in new code."

Also applies to: 28-30, 61-63, 131-133

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/field_index/geo_index/immutable_geo_index/lifecycle.rs`
around lines 22 - 25, Replace implicit conversions using err.into() with
explicit OperationError::from(err) in the Result error branches: where
counts_per_hash is built from index.storage.counts_per_hash.read_whole() (the
match arm returning Err(err) -> Err(err.into())), and the other similar matches
referenced (the match that constructs counts_per_hash, the ones at the other
occurrences around the code and the ones noted at 28-30, 61-63, 131-133).
Concretely, change each Err(err) => return Err(err.into()) to Err(err) => return
Err(OperationError::from(err)) so the code uses explicit OperationError::from
conversions for those read_whole() error paths and any identical patterns in the
same module.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

lib/segment/src/index/field_index/map_index/immutable_map_index/lifecycle.rs (1)

24-24: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Mmap-backed index is dropped on scan failure, preventing rollback.

The boxed index (line 24) is dropped if for_each_value_map returns an error and the ? operator on line 67 triggers an early return. This makes the original mmap-backed UniversalMapIndex<N> unavailable for rollback during swap operations, as noted in the previous review.

To preserve the index for rollback, either leak it before returning the error (Box::into_raw / mem::forget) or change the return type to include the index on failure: Result<Self, (OperationError, Box<UniversalMapIndex<N>>)>.

Also applies to: 40-67

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/field_index/map_index/immutable_map_index/lifecycle.rs`
at line 24, The boxed mmap-backed index is dropped when for_each_value_map
returns an error because the function currently returns early with `?`; change
the function to return Result<Self, (OperationError,
Box<UniversalMapIndex<N>>)>, keep the `let index = Box::new(index);` as is, and
update all error propagation around the for_each_value_map call (and any returns
in the 40-67 region) to map/map_err the OperationError into Err((error, index))
so the caller receives the boxed index for rollback; update the function
signature and any call sites accordingly to accept the new Err payload.

lib/segment/src/index/field_index/geo_index/immutable_geo_index/lifecycle.rs (1)

21-29: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Mmap-backed index is still dropped on all error paths, preventing rollback.

The boxed index is dropped when any error path returns early (? on lines 25, 32, 99, 129, or explicit Err on line 63). This prevents the caller from recovering the original mmap-backed index during swap rollback, as flagged in the previous review.

The current code boxes the index on line 21 but does not prevent it from being dropped on error. To support rollback, the index must survive failures—either by using Box::into_raw / mem::forget before returning errors, or by changing the signature to return Result<Self, (OperationError, Box<StoredGeoMapIndex<MmapFile>>)>.

Also applies to: 32-32, 62-64, 98-99, 129-129

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/field_index/geo_index/immutable_geo_index/lifecycle.rs`
around lines 21 - 29, The boxed mmap-backed index (created as Box::new(index))
is being dropped on early error returns which prevents rollback; update the code
so the boxed index is preserved on all error paths: either convert the Box to a
raw pointer (Box::into_raw or std::mem::forget) before any `?`/Err returns so it
is not dropped, or change the function's error type to return Result<Self,
(OperationError, Box<StoredGeoMapIndex<MmapFile>>)>, and adjust every early
return (the places that currently `?` on storage.counts_per_hash.read_whole()
and other fallible calls and explicit Err returns) to return the error paired
with the boxed index instead of letting the Box be dropped. Ensure every path
that currently returns an error returns the boxed index unchanged so the caller
can perform rollback.

lib/segment/src/index/field_index/geo_index/immutable_geo_index/lifecycle.rs (1)

20-32: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Error branches still drop the boxed mmap index, removing the swap-rollback recovery path.

After boxing index at Line 21, every fallible step (read_whole()? at Lines 25 and 32, read_batch(...)? at Line 61, collected? at Line 96, and encode_max_precision(...)? at Line 126) propagates only the OperationError and drops the owned StoredGeoMapIndex<MmapFile>. During an in-place Mmap → Immutable swap, a load failure here therefore destroys the original mmap-backed index and prevents restoring it on rollback.

Also applies to: 61-61, 96-96, 126-126

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/segment/src/index/field_index/geo_index/immutable_geo_index/lifecycle.rs`
around lines 20 - 32, In open_mmap, do not box/move the
StoredGeoMapIndex<MmapFile> into heap (the local variable index) before
performing fallible reads; currently boxing at the start (variable index) causes
the mmap-backed index to be dropped on any early error (e.g.,
counts_per_hash.read_whole(), storage.points_map.read_whole(), read_batch(...),
iterator.collect(), encode_max_precision(...)), preventing swap-rollback
recovery. Fix by deferring the Box::new(...) (or move into the final
ImmutableGeoIndex) until after all
read_whole/read_batch/collect/encode_max_precision calls succeed — use
references/borrows to access index.storage for these operations or perform the
reads into temporaries first, then box the StoredGeoMapIndex only once all
fallible operations have completed successfully.