(improvement) docs, tests and double check removal

Skip-index duplicate detection was removed.
I could have retained the code that checks if the same index is added twice,
but it would have made the implementation more complex.
At the end, configuring duplicate skip indexes is undesired,
but it affects performance rather than correctness.

Signed-off-by: Joaquin Colacci <joaquincolacci@gmail.com>

Author: joacoc

vortex-file/tests/bloom_skip_index.rs

@@ -140,7 +140,7 @@ fn strategy(
 
     // Adding the aggregate to these field-specific zoned options is the explicit write-side
     // opt-in. Registering the index in the session alone does not change the file layout.
-    options = options.with_skip_index(index.clone())?;
+    options = options.with_skip_index(index.clone());
 
     Ok(WriteStrategyBuilder::default()
         .with_field_zoned_options(field_path!(id), options)

vortex-layout/src/layouts/zoned/skip_index/mod.rs

@@ -3,32 +3,26 @@
 
 //! Skipping-index interface and implementations.
 //!
-//! The skip index provides a way to prune zones
-//! by using the results of an aggregation function.
-//! During writes, an aggregation function calculates stats for each zone.
+//! The skip index provides a way to prune zones by using the results
+//! of an aggregation function.
+//! During writes, the aggregation function calculates stats for each zone.
 //! During reads, these stats are interpreted to determine whether a zone
 //! can be pruned.
 //!
-//! Unlike a locating index, a skipping index summarizes a contiguous
-//! range of rows in a zone. It does not locate matching rows. It only
-//! proves that a zone cannot match a predicate.
-//!
-//! For correct reading and writing, a session must have registered the skip index.
-//! If a skip index is not loaded and the session allows unknown plugins,
-//! Vortex disables zoned pruning and scans the data normally. Otherwise,
-//! opening the file fails.
-//!
 //! The skip index is coupled to the zoned layout through [`ZonedLayoutOptions`].
 //! To set one up, a user must declare a skip index for a field
 //! through the writer configuration.
+//!
+//! ### Difference from a locating index
+//!
+//! Unlike a locating index, a skipping index summarizes a zone.
+//! It does not locate matching rows. It can only prove that a zone
+//! cannot match a predicate.
 
 use std::sync::Arc;
 
-use vortex_array::aggregate_fn::AggregateFnId;
 use vortex_array::aggregate_fn::AggregateFnRef;
 use vortex_array::dtype::DType;
-use vortex_error::VortexResult;
-use vortex_error::vortex_bail;
 use vortex_session::SessionExt;
 use vortex_session::VortexSession;
 
@@ -40,10 +34,13 @@ use super::writer::ZonedLayoutOptions;
 /// The writer helper [`ZonedLayoutOptions::with_skip_index`] is the explicit per-column declaration
 /// seam. Readers call [`SkipIndexSessionExt::register_skip_index`] on their
 /// [`VortexSession`] before opening the file.
+///
+/// ## Logical and physical representation
+///
+/// [`SkipIndex`] defines the logical skip-index abstraction. A skip index has no identifier
+/// or serialized representation of its own. Instead, it is represented in a Vortex file by
+/// the serialized [`AggregateFnRef`] it provides.
 pub trait SkipIndex: Send + Sync + 'static {
-    /// Returns the inner aggregate identifier.
-    fn aggregate_id(&self) -> AggregateFnId;
-
     /// The aggregate state to persist for `input_dtype`, or `None` when unsupported.
     fn aggregate_fn(&self, input_dtype: &DType) -> Option<AggregateFnRef>;
 
@@ -87,10 +84,6 @@ impl SkipIndexRef {
         SkipIndexRef(index_ref)
     }
 
-    pub fn aggregate_id(&self) -> AggregateFnId {
-        self.0.aggregate_id()
-    }
-
     pub fn aggregate_fn(&self, input_dtype: &DType) -> Option<AggregateFnRef> {
         self.0.aggregate_fn(input_dtype)
     }
@@ -105,25 +98,16 @@ impl ZonedLayoutOptions {
     ///
     /// `WriteStrategyBuilder::with_field_zoned_options` can install the configured options for one
     /// field while retaining the default data layout pipeline.
-    pub fn with_skip_index(mut self, skip_index: SkipIndexRef) -> VortexResult<Self> {
+    pub fn with_skip_index(mut self, skip_index: SkipIndexRef) -> Self {
         let mut skip_indexes = self
             .skip_indexes
             .take()
             .map(|indexes| indexes.to_vec())
             .unwrap_or_default();
 
-        if skip_indexes
-            .iter()
-            .any(|index| index.aggregate_id() == skip_index.aggregate_id())
-        {
-            vortex_bail!(
-                "skip index aggregate {} is already configured",
-                skip_index.aggregate_id()
-            );
-        }
-
         skip_indexes.push(skip_index);
         self.skip_indexes = Some(skip_indexes.into());
-        Ok(self)
+
+        self
     }
 }

vortex-layout/src/layouts/zoned/writer.rs

@@ -1,8 +1,8 @@
-//! Write-time assembly for zoned layouts.
-
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
+//! Write-time assembly for zoned layouts.
+
 use std::num::NonZeroUsize;
 use std::sync::Arc;
 
@@ -112,24 +112,8 @@ impl LayoutStrategy for ZonedStrategy {
         if let Some(skip_indexes) = &self.options.skip_indexes {
             for skip_index in skip_indexes.iter() {
                 match skip_index.aggregate_fn(stream.dtype()) {
-                    Some(skip_aggregate_fn) => {
-                        if aggregate_fns
-                            .iter()
-                            .any(|aggregate_fn| aggregate_fn.id() == skip_aggregate_fn.id())
-                        {
-                            vortex_bail!(
-                                "skip index aggregate {} is configured more than once",
-                                skip_aggregate_fn.id()
-                            );
-                        }
-
-                        aggregate_fns.push(skip_aggregate_fn);
-                    }
-                    None => vortex_bail!(
-                        "skip index aggregate {} is unsupported for type {}",
-                        skip_index.aggregate_id(),
-                        stream.dtype()
-                    ),
+                    Some(skip_aggregate_fn) => aggregate_fns.push(skip_aggregate_fn),
+                    None => vortex_bail!("skip index is unsupported for type {}", stream.dtype()),
                 }
             }
         }
@@ -230,6 +214,8 @@ mod tests {
     use vortex_array::ArrayContext;
     use vortex_array::IntoArray;
     use vortex_array::aggregate_fn::AggregateFnVTable;
+    use vortex_array::aggregate_fn::AggregateFnVTableExt;
+    use vortex_array::aggregate_fn::NumericalAggregateOpts;
     use vortex_array::aggregate_fn::fns::bounded_max::BoundedMax;
     use vortex_array::aggregate_fn::fns::bounded_min::BoundedMin;
     use vortex_array::aggregate_fn::fns::max::Max;
@@ -256,21 +242,22 @@ mod tests {
     use crate::layouts::flat::writer::FlatLayoutStrategy;
     use crate::layouts::zoned::Zoned;
     use crate::layouts::zoned::schema::default_bounded_stat_max_bytes;
+    use crate::layouts::zoned::skip_index::SkipIndex;
     use crate::segments::TestSegments;
     use crate::sequence::SequenceId;
     use crate::sequence::SequentialArrayStreamExt;
     use crate::session::LayoutSession;
 
     /// Write three zones of primitives through `ctx`, returning the aggregates the zoned
     /// layout recorded.
-    fn write_zones(ctx: LayoutWriterContext) -> VortexResult<Vec<String>> {
+    fn write_zones_with_options(
+        ctx: LayoutWriterContext,
+        options: ZonedLayoutOptions,
+    ) -> VortexResult<Vec<String>> {
         let strategy = ZonedStrategy::new(
             ChunkedLayoutStrategy::new(FlatLayoutStrategy::default()),
             FlatLayoutStrategy::default(),
-            ZonedLayoutOptions {
-                block_size: NonZeroUsize::new(3).vortex_expect("non zero"),
-                ..Default::default()
-            },
+            options,
         );
         let (ptr, eof) = SequenceId::root().split();
         let stream = ChunkedArray::from_iter([
@@ -306,6 +293,16 @@ mod tests {
             .collect())
     }
 
+    fn write_zones(ctx: LayoutWriterContext) -> VortexResult<Vec<String>> {
+        write_zones_with_options(
+            ctx,
+            ZonedLayoutOptions {
+                block_size: NonZeroUsize::new(3).vortex_expect("non zero"),
+                ..Default::default()
+            },
+        )
+    }
+
     #[test]
     fn unrestricted_context_writes_the_default_aggregates() -> VortexResult<()> {
         let written = write_zones(LayoutWriterContext::new(ArrayContext::empty()))?;
@@ -380,4 +377,64 @@ mod tests {
                 .all(|aggregate_fn| !aggregate_fn.is::<Sum>())
         );
     }
+
+    // TODO (joacoc): replace with bloom skip index after merge <https://github.com/vortex-data/vortex/pull/9398>
+    // Just a small skip-index implementation used to test the zoned writer until the bloom filter lands.
+    struct TestSkipIndex {
+        aggregate_fn: Option<AggregateFnRef>,
+    }
+
+    impl TestSkipIndex {
+        fn new(aggregate_fn: AggregateFnRef) -> Self {
+            Self {
+                aggregate_fn: Some(aggregate_fn),
+            }
+        }
+
+        fn unsupported() -> Self {
+            Self { aggregate_fn: None }
+        }
+
+        fn into_ref(self) -> SkipIndexRef {
+            SkipIndexRef::new(Arc::new(self))
+        }
+    }
+
+    impl SkipIndex for TestSkipIndex {
+        fn aggregate_fn(&self, _input_dtype: &DType) -> Option<AggregateFnRef> {
+            self.aggregate_fn.clone()
+        }
+
+        fn register(&self, _session: &VortexSession) {}
+    }
+
+    #[test]
+    fn writer_appends_skip_index_aggregate() -> VortexResult<()> {
+        let options = ZonedLayoutOptions::default().with_skip_index(
+            TestSkipIndex::new(Sum.bind(NumericalAggregateOpts::default())).into_ref(),
+        );
+        let written =
+            write_zones_with_options(LayoutWriterContext::new(ArrayContext::empty()), options)?;
+
+        assert!(
+            written.contains(&Sum.id().to_string()),
+            "expected aggregation id present, wrote {written:?}"
+        );
+        Ok(())
+    }
+
+    #[test]
+    fn writer_rejects_skip_index_unsupported_for_dtype() -> VortexResult<()> {
+        let options =
+            ZonedLayoutOptions::default().with_skip_index(TestSkipIndex::unsupported().into_ref());
+
+        let error =
+            write_zones_with_options(LayoutWriterContext::new(ArrayContext::empty()), options)
+                .expect_err("unsupported skip index should fail the write");
+        assert!(
+            error.to_string().contains("unsupported for type"),
+            "unexpected error: {error}"
+        );
+        Ok(())
+    }
 }