feat: add readonly null index open (#9197)

* feat: add readonly null index open

* feat: add get_mutability_type to ReadOnlyNullIndex

* fix: pr reviews

* simpler phantomdata

---------

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>

lib/common/common/Cargo.toml

@@ -38,6 +38,7 @@ quick_cache = "0.6.22"
 rand = { workspace = true }
 roaring = { workspace = true }
 schemars = { workspace = true }
+self_cell = { workspace = true }
 semver = { workspace = true }
 serde = { workspace = true }
 serde_json = { workspace = true }
@@ -58,7 +59,6 @@ criterion = { workspace = true }
 duplicate = "2.0.1"
 fs-err = { workspace = true, features = ["debug"] }
 rstest = { workspace = true }
-self_cell = { workspace = true }
 tango-bench = "0.7.2"
 
 [target.'cfg(target_os = "linux")'.dependencies]

lib/common/common/src/stored_bitslice.rs

@@ -10,7 +10,7 @@ use std::path::Path;
 
 use bitvec::mem::BitRegister;
 use bitvec::order::Lsb0;
-use itertools::Itertools;
+use itertools::{Either, Itertools};
 
 use crate::bitvec::BitVec;
 use crate::generic_consts::Random;
@@ -19,6 +19,28 @@ use crate::universal_io::{
     UniversalReadFs, UniversalWrite,
 };
 
+/// `IterOnes` view over a `BitSlice<u64, Lsb0>` — type alias so `self_cell`
+/// can refer to it as a single-lifetime type constructor.
+type IterOnesView<'a> = bitvec::slice::IterOnes<'a, BitStore, Lsb0>;
+
+self_cell::self_cell!(
+    /// Owns a `BitVec` and the `IterOnes` cursor over it, so an owned-path
+    /// `iter_ones()` can return without collecting set positions into a `Vec`.
+    struct OwnedOnes {
+        owner: BitVec,
+        #[covariant]
+        dependent: IterOnesView,
+    }
+);
+
+impl Iterator for OwnedOnes {
+    type Item = u64;
+
+    fn next(&mut self) -> Option<u64> {
+        self.with_dependent_mut(|_, it| it.next()).map(|i| i as u64)
+    }
+}
+
 /// Number of bits per `BitStore` element.
 const BITS_PER_ELEMENT: u32 = BitStore::BITS;
 
@@ -146,6 +168,23 @@ impl<S: UniversalRead> StoredBitSlice<S> {
         Ok(self.read_all()?.count_ones())
     }
 
+    /// Iterate the bit indices of all set bits, in ascending order.
+    ///
+    /// Zero-copy on backends that support it (mmap): the iterator borrows the
+    /// mapped pages. On owned-read backends the iterator carries the materialized
+    /// `BitVec` alongside its `IterOnes` cursor via [`OwnedOnes`], so no
+    /// intermediate `Vec` of set positions is allocated. Reads the whole storage
+    /// including any trailing capacity, so callers that keep unused capacity
+    /// cleared get back exactly their set positions.
+    pub fn iter_ones(&self) -> Result<impl Iterator<Item = u64> + '_> {
+        let cow_bitslice = self.read_all()?;
+        let iter = match cow_bitslice {
+            Cow::Borrowed(bitslice) => Either::Left(bitslice.iter_ones().map(|i| i as u64)),
+            Cow::Owned(bitvec) => Either::Right(OwnedOnes::new(bitvec, |bv| bv.iter_ones())),
+        };
+        Ok(iter)
+    }
+
     /// Get a single bit at the given bit index.
     ///
     /// Fetches the containing `u64` element from the backend and extracts the
@@ -596,4 +635,19 @@ mod tests {
         // With mmap backend, read_all returns Cow::Borrowed (zero-copy)
         assert!(matches!(bs, Cow::Borrowed(_)));
     }
+
+    #[test]
+    fn test_owned_ones_walks_all_set_bits() {
+        // The owned path of `iter_ones` carries the BitVec alongside its
+        // IterOnes cursor via OwnedOnes. Verify the cursor actually advances
+        // and yields every set position — coszio's retracted from_fn shape
+        // failed this exact test (it re-yielded the first set bit forever).
+        let mut bv: BitVec = BitVec::repeat(false, 200);
+        for i in [1u64, 3, 4, 64, 65, 199] {
+            bv.set(i as usize, true);
+        }
+        let iter = OwnedOnes::new(bv, |bv| bv.iter_ones());
+        let collected: Vec<u64> = iter.collect();
+        assert_eq!(collected, vec![1, 3, 4, 64, 65, 199]);
+    }
 }

lib/segment/src/common/flags/dynamic_stored_flags.rs

@@ -11,7 +11,6 @@ use common::universal_io::{
     OpenOptions, Populate, StoredStruct, UniversalReadFileOps, UniversalWrite,
 };
 use fs_err as fs;
-use itertools::Either;
 
 use crate::common::Flusher;
 use crate::common::operation_error::{OperationError, OperationResult};
@@ -283,18 +282,9 @@ where
 
     /// Iterate over all "true" flags
     pub fn iter_trues(&self) -> OperationResult<impl Iterator<Item = PointOffsetType> + '_> {
-        Ok(match self.flags.read_all()? {
-            Cow::Borrowed(bitslice) => {
-                Either::Left(bitslice.iter_ones().map(|x| x as PointOffsetType))
-            }
-            Cow::Owned(bitvec) => {
-                // Owned path: backend doesn't support zero-copy; materialize into Vec
-                // so we don't return a reference to a local
-                let indices: Vec<PointOffsetType> =
-                    bitvec.iter_ones().map(|x| x as PointOffsetType).collect();
-                Either::Right(indices.into_iter())
-            }
-        })
+        // Unused capacity past `len` is always cleared, so iterating set bits
+        // over the whole storage yields exactly the "true" positions.
+        Ok(self.flags.iter_ones()?.map(|i| i as PointOffsetType))
     }
 
     /// Populate all pages in the mmap.

lib/segment/src/common/flags/read_only_roaring_flags.rs

@@ -1,27 +1,27 @@
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 
+use common::mmap::AdviceSetting;
 use common::stored_bitslice::StoredBitSlice;
-use common::universal_io::UniversalRead;
+use common::types::PointOffsetType;
+use common::universal_io::{OpenOptions, Populate, TypedStorage, UniversalRead};
 use roaring::RoaringBitmap;
 
-use super::dynamic_stored_flags::{FLAGS_FILE, status_file};
+use super::dynamic_stored_flags::{DynamicFlagsStatus, FLAGS_FILE, status_file};
 use super::roaring_flags::RoaringFlagsRead;
 use crate::common::operation_error::OperationResult;
 
 /// Read-only counterpart of [`RoaringFlags`][1].
 ///
-/// Loads the persisted flags into an in-memory roaring bitmap on open and keeps
-/// the underlying flags file around for [`populate`] / [`clear_cache`] /
-/// [`files`]. No write path: there is no buffer, no [`BufferedDynamicFlags`][2],
-/// no [`DynamicStoredFlags`][3] — the storage backend is bound to
-/// [`UniversalRead`] only.
+/// Loads the persisted flags into an in-memory roaring bitmap on open. No
+/// write path: there is no buffer, no [`BufferedDynamicFlags`][2], no
+/// [`DynamicStoredFlags`][3] — the storage backend is bound to
+/// [`UniversalRead`] only. Everything is in RAM after construction, so
+/// `populate` / `clear_cache` from [`RoaringFlagsRead`] use their default
+/// no-op behavior.
 ///
 /// [1]: super::roaring_flags::RoaringFlags
 /// [2]: super::buffered_dynamic_flags::BufferedDynamicFlags
 /// [3]: super::dynamic_stored_flags::DynamicStoredFlags
-/// [`populate`]: ReadOnlyRoaringFlags::populate
-/// [`clear_cache`]: ReadOnlyRoaringFlags::clear_cache
-/// [`files`]: ReadOnlyRoaringFlags::files
 pub struct ReadOnlyRoaringFlags<S: UniversalRead> {
     /// In-memory bitmap of true flags, materialized from the backing file on open.
     bitmap: RoaringBitmap,
@@ -29,12 +29,67 @@ pub struct ReadOnlyRoaringFlags<S: UniversalRead> {
     /// Total length of the flags, including trailing falses. Read from the status file.
     len: usize,
 
-    /// Backing flags file. Kept open so `populate` / `clear_cache` / `files`
-    /// can drive the underlying storage; never read from again after the
-    /// bitmap is built.
-    flags_storage: StoredBitSlice<S>,
-
     directory: PathBuf,
+
+    _marker: std::marker::PhantomData<S>,
+}
+
+impl<S: UniversalRead> ReadOnlyRoaringFlags<S> {
+    /// Open persisted flags read-only and materialize them into an in-memory
+    /// roaring bitmap.
+    ///
+    /// Read-only counterpart of [`RoaringFlags::new`][1]: every file is opened
+    /// through `fs` non-writable, nothing is created and nothing is written.
+    /// The logical length comes from the status file (the flags file is padded
+    /// past it), and the set positions from the flags file — shared with the
+    /// writable path via [`StoredBitSlice::iter_ones`].
+    ///
+    /// [1]: super::roaring_flags::RoaringFlags::new
+    pub fn open(fs: &S::Fs, directory: &Path) -> OperationResult<Self> {
+        // Logical length: read the status struct directly. `StoredStruct` is
+        // write-bound, so go through the read-only `TypedStorage`.
+        let status_path = status_file(directory);
+        let status = TypedStorage::<S, DynamicFlagsStatus>::open(
+            fs,
+            &status_path,
+            OpenOptions {
+                writeable: false,
+                need_sequential: false,
+                populate: Populate::No,
+                advice: AdviceSetting::Global,
+            },
+            Default::default(),
+        )?;
+        let len = status.read_whole()?[0].len();
+
+        // Set positions: open the flags file and build the bitmap. The
+        // `StoredBitSlice` is dropped at the end of this function — the
+        // bitmap is the only state we keep.
+        let flags_path = directory.join(FLAGS_FILE);
+        let flags_storage = StoredBitSlice::<S>::open(
+            fs,
+            &flags_path,
+            OpenOptions {
+                writeable: false,
+                need_sequential: false,
+                populate: Populate::No,
+                advice: AdviceSetting::Global,
+            },
+            Default::default(),
+        )?;
+
+        let bitmap = RoaringBitmap::from_sorted_iter(
+            flags_storage.iter_ones()?.map(|i| i as PointOffsetType),
+        )
+        .expect("iter_ones iterates in sorted order");
+
+        Ok(Self {
+            bitmap,
+            len,
+            directory: directory.to_path_buf(),
+            _marker: std::marker::PhantomData,
+        })
+    }
 }
 
 impl<S: UniversalRead> RoaringFlagsRead for ReadOnlyRoaringFlags<S> {
@@ -46,16 +101,6 @@ impl<S: UniversalRead> RoaringFlagsRead for ReadOnlyRoaringFlags<S> {
         &self.bitmap
     }
 
-    fn populate(&self) -> OperationResult<()> {
-        self.flags_storage.populate()?;
-        Ok(())
-    }
-
-    fn clear_cache(&self) -> OperationResult<()> {
-        self.flags_storage.clear_ram_cache()?;
-        Ok(())
-    }
-
     fn files(&self) -> Vec<PathBuf> {
         vec![
             status_file(&self.directory),

lib/segment/src/common/flags/roaring_flags.rs

@@ -60,7 +60,12 @@ pub trait RoaringFlagsRead {
     }
 
     /// Drop disk cache for the backing file.
-    fn clear_cache(&self) -> OperationResult<()>;
+    ///
+    /// Default: no-op (mirrors [`populate`][Self::populate] — variants that
+    /// hold everything in memory after open have no on-disk cache to drop).
+    fn clear_cache(&self) -> OperationResult<()> {
+        Ok(())
+    }
 
     /// Paths of the on-disk files backing this storage.
     fn files(&self) -> Vec<PathBuf>;

lib/segment/src/index/field_index/null_index/read_only_null_index/lifecycle.rs

@@ -0,0 +1,35 @@
+use std::path::Path;
+
+use common::universal_io::UniversalRead;
+
+use super::super::mutable_null_index::{HAS_VALUES_DIRNAME, IS_NULL_DIRNAME};
+use super::{ReadOnlyNullIndex, ReadOnlyStorage};
+use crate::common::flags::read_only_roaring_flags::ReadOnlyRoaringFlags;
+use crate::common::operation_error::OperationResult;
+
+impl<S: UniversalRead> ReadOnlyNullIndex<S> {
+    /// Open a read-only null index at `path`, threading every file open through
+    /// the filesystem handle `fs`.
+    ///
+    /// `fs` is the generic filesystem object (e.g. `ReadOnlyFs<MmapFs>`): the
+    /// index never touches the local filesystem directly, it opens all of its
+    /// files — the `has_values` and `is_null` flag directories — through `fs`.
+    /// `total_point_count` is the segment-wide point count, the same value the
+    /// writable [`MutableNullIndex::open`][1] receives; it is not derivable from
+    /// the index files alone.
+    ///
+    /// [1]: super::super::mutable_null_index::MutableNullIndex::open
+    pub fn open(fs: &S::Fs, path: &Path, total_point_count: usize) -> OperationResult<Self> {
+        let has_values_flags = ReadOnlyRoaringFlags::open(fs, &path.join(HAS_VALUES_DIRNAME))?;
+        let is_null_flags = ReadOnlyRoaringFlags::open(fs, &path.join(IS_NULL_DIRNAME))?;
+
+        Ok(Self {
+            _base_dir: path.to_path_buf(),
+            storage: ReadOnlyStorage {
+                has_values_flags,
+                is_null_flags,
+            },
+            total_point_count,
+        })
+    }
+}

lib/segment/src/index/field_index/null_index/read_only_null_index/mod.rs

@@ -3,7 +3,9 @@ use std::path::PathBuf;
 use common::universal_io::UniversalRead;
 
 use crate::common::flags::read_only_roaring_flags::ReadOnlyRoaringFlags;
+use crate::index::payload_config::IndexMutability;
 
+mod lifecycle;
 mod read_ops;
 
 /// Read-only counterpart of [`MutableNullIndex`][1] / [`ImmutableNullIndex`][2].
@@ -29,3 +31,134 @@ pub(super) struct ReadOnlyStorage<S: UniversalRead> {
     /// Points which have null values
     pub(super) is_null_flags: ReadOnlyRoaringFlags<S>,
 }
+
+impl<S: UniversalRead> ReadOnlyNullIndex<S> {
+    /// Reports the on-disk format's mutability, mirroring
+    /// [`NullIndex::get_mutability_type`][1].
+    ///
+    /// `MutableNullIndex` and `ImmutableNullIndex` share the same on-disk
+    /// layout (the latter is a newtype wrapper around the former — see
+    /// `ImmutableNullIndex(MutableNullIndex)`), so the read path cannot
+    /// distinguish them from the files alone. The read-only wrapper denies
+    /// mutation either way, so it conservatively reports
+    /// [`IndexMutability::Immutable`]. If a future caller needs to preserve
+    /// the writable-side label exactly (e.g. for round-tripping
+    /// [`FullPayloadIndexType`][2] against the persisted schema), the
+    /// mutability should be threaded through [`Self::open`] and stored on the
+    /// struct.
+    ///
+    /// [1]: super::super::NullIndex::get_mutability_type
+    /// [2]: crate::index::payload_config::FullPayloadIndexType
+    pub fn get_mutability_type(&self) -> IndexMutability {
+        IndexMutability::Immutable
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use common::counter::hardware_counter::HardwareCounterCell;
+    use common::universal_io::{MmapFile, ReadOnly, UniversalRead, UniversalReadFileOps};
+    use itertools::Itertools as _;
+    use serde_json::{Value, json};
+    use tempfile::TempDir;
+
+    use super::super::mutable_null_index::MutableNullIndex;
+    use super::super::read_ops::NullIndexRead;
+    use super::ReadOnlyNullIndex;
+    use crate::index::field_index::{FieldIndexBuilderTrait, PayloadFieldIndexRead};
+    use crate::json_path::JsonPath;
+    use crate::types::FieldCondition;
+
+    /// Build a writable null index on disk, then open it read-only through a
+    /// write-prevented `ReadOnlyFs<MmapFs>` backend and assert the read surface
+    /// (both bitmaps and the persisted length) matches what was written.
+    #[test]
+    fn read_only_null_index_round_trip() {
+        let dir = TempDir::with_prefix("read_only_null_index").unwrap();
+        let hw_counter = HardwareCounterCell::new();
+
+        let null_in_array = Value::Array(vec![Value::String("x".to_string()), Value::Null]);
+        let mut builder = MutableNullIndex::builder(dir.path(), 0).unwrap();
+        builder.add_point(0, &[&Value::Null], &hw_counter).unwrap(); // null, no values
+        builder
+            .add_point(1, &[&null_in_array], &hw_counter)
+            .unwrap(); // null + values
+        builder.add_point(2, &[], &hw_counter).unwrap(); // empty
+        builder.add_point(3, &[&json!(true)], &hw_counter).unwrap(); // values, not null
+        let total = 4;
+        builder.finalize().unwrap();
+
+        // `S = ReadOnly<MmapFile>` → `S::Fs = ReadOnlyFs<MmapFs>`, named via the
+        // associated-type projection since the wrapper type isn't exported.
+        // The read-only filesystem context is `Default`.
+        type RoFs = <ReadOnly<MmapFile> as UniversalRead>::Fs;
+        let fs = RoFs::from_context(Default::default()).unwrap();
+
+        let index: ReadOnlyNullIndex<ReadOnly<MmapFile>> =
+            ReadOnlyNullIndex::open(&fs, dir.path(), total).unwrap();
+
+        let key = JsonPath::new("test");
+        let is_null = FieldCondition::new_is_null(key.clone(), true);
+        let is_not_empty = FieldCondition {
+            key: key.clone(),
+            r#match: None,
+            range: None,
+            geo_bounding_box: None,
+            geo_radius: None,
+            geo_polygon: None,
+            values_count: None,
+            is_empty: Some(false),
+            is_null: None,
+        };
+        let is_empty = FieldCondition {
+            key: key.clone(),
+            r#match: None,
+            range: None,
+            geo_bounding_box: None,
+            geo_radius: None,
+            geo_polygon: None,
+            values_count: None,
+            is_empty: Some(true),
+            is_null: None,
+        };
+
+        // Bitmap-driven branches: the set positions came back intact.
+        assert_eq!(
+            index
+                .filter(&is_null, &hw_counter)
+                .unwrap()
+                .unwrap()
+                .collect_vec(),
+            vec![0, 1],
+        );
+        assert_eq!(
+            index
+                .filter(&is_not_empty, &hw_counter)
+                .unwrap()
+                .unwrap()
+                .collect_vec(),
+            vec![1, 3],
+        );
+        // `len`-driven branch: iter_falses over [0, len) chained with [len, total).
+        // Wrong `len` (e.g. 0) would wrongly include points 1 and 3 here.
+        assert_eq!(
+            index
+                .filter(&is_empty, &hw_counter)
+                .unwrap()
+                .unwrap()
+                .collect_vec(),
+            vec![0, 2],
+        );
+        // Length read straight from the status file.
+        assert_eq!(index.count_indexed_points(), 4);
+
+        assert!(index.values_is_empty(0));
+        assert!(!index.values_is_empty(1));
+        assert!(index.values_is_empty(2));
+        assert!(!index.values_is_empty(3));
+
+        assert!(index.values_is_null(0));
+        assert!(index.values_is_null(1));
+        assert!(!index.values_is_null(3));
+    }
+}