address comments

Signed-off-by: Matthew Katz <katz@spiraldb.com>

Author: Matthew Katz

vortex-array/public-api.lock

@@ -4454,8 +4454,6 @@ pub fn vortex_array::arrays::listview::ListViewArrayExt::compute_referenced_elem
 
 pub fn vortex_array::arrays::listview::ListViewArrayExt::elements(&self) -> &vortex_array::ArrayRef
 
-pub fn vortex_array::arrays::listview::ListViewArrayExt::estimate_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<f32>
-
 pub fn vortex_array::arrays::listview::ListViewArrayExt::list_elements_at(&self, usize) -> vortex_error::VortexResult<vortex_array::ArrayRef>
 
 pub fn vortex_array::arrays::listview::ListViewArrayExt::listview_validity(&self) -> vortex_array::validity::Validity
@@ -4470,6 +4468,8 @@ pub fn vortex_array::arrays::listview::ListViewArrayExt::size_at(&self, usize) -
 
 pub fn vortex_array::arrays::listview::ListViewArrayExt::sizes(&self) -> &vortex_array::ArrayRef
 
+pub fn vortex_array::arrays::listview::ListViewArrayExt::upper_bound_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<f32>
+
 pub fn vortex_array::arrays::listview::ListViewArrayExt::verify_is_zero_copy_to_list(&self) -> bool
 
 impl<T: vortex_array::TypedArrayRef<vortex_array::arrays::ListView>> vortex_array::arrays::listview::ListViewArrayExt for T
@@ -4480,8 +4480,6 @@ pub fn T::compute_referenced_elements_mask(&self, &mut vortex_array::ExecutionCt
 
 pub fn T::elements(&self) -> &vortex_array::ArrayRef
 
-pub fn T::estimate_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<f32>
-
 pub fn T::list_elements_at(&self, usize) -> vortex_error::VortexResult<vortex_array::ArrayRef>
 
 pub fn T::listview_validity(&self) -> vortex_array::validity::Validity
@@ -4496,6 +4494,8 @@ pub fn T::size_at(&self, usize) -> usize
 
 pub fn T::sizes(&self) -> &vortex_array::ArrayRef
 
+pub fn T::upper_bound_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<f32>
+
 pub fn T::verify_is_zero_copy_to_list(&self) -> bool
 
 pub fn vortex_array::arrays::listview::list_from_list_view(vortex_array::arrays::ListViewArray) -> vortex_error::VortexResult<vortex_array::arrays::ListArray>

vortex-array/src/arrays/listview/array.rs

@@ -315,6 +315,31 @@ impl Default for ListViewData {
     }
 }
 
+/// Walks parallel `(offset, size)` slices and marks every referenced position in `buf`.
+///
+/// **Precondition**
+///
+/// `offsets` and `sizes` must be the same length (which is always the case in valid `ListViewArray`s).
+fn fill_referenced_mask<O: IntegerPType, S: IntegerPType>(
+    buf: &mut BitBufferMut,
+    offsets: &[O],
+    sizes: &[S],
+) {
+    let len = offsets.len();
+
+    assert_eq!(
+        len,
+        sizes.len(),
+        "offsets and sizes must be the same length"
+    );
+
+    for i in 0..len {
+        let start: usize = offsets[i].as_();
+        let size: usize = sizes[i].as_();
+        buf.fill_range(start, start + size, true);
+    }
+}
+
 pub trait ListViewArrayExt: TypedArrayRef<ListView> {
     fn nullability(&self) -> crate::dtype::Nullability {
         match self.as_ref().dtype() {
@@ -402,35 +427,31 @@ pub trait ListViewArrayExt: TypedArrayRef<ListView> {
     }
 
     /// Returns a [`Mask`] of length `elements.len()` where each bit is set iff that
-    /// position in `elements` is referenced by at least one view.
+    /// position in `elements` is referenced by at least one view. Caller must ensure `elements`
+    /// is non-empty.
     ///
     /// Walks every `(offset, size)` pair, canonicalizes both `offsets` and `sizes`,
     /// and allocates a `BitBuffer` of length `elements.len()`, so it is extremely costly.
     ///
     /// **Preconditions**
     ///
-    /// Assumes that `self.elements()` is non-empty.
-    #[allow(clippy::cognitive_complexity)]
+    /// `self.elements()` must be non-empty.
     fn compute_referenced_elements_mask(&self, ctx: &mut ExecutionCtx) -> VortexResult<Mask> {
-        debug_assert!(!self.elements().is_empty());
+        assert!(!self.elements().is_empty());
         let len = self.elements().len();
 
         let offsets_primitive = self.offsets().clone().execute::<PrimitiveArray>(ctx)?;
         let sizes_primitive = self.sizes().clone().execute::<PrimitiveArray>(ctx)?;
 
         let mut buf = BitBufferMut::new_unset(len);
-        let offset_len = self.as_ref().len();
 
         match_each_integer_ptype!(offsets_primitive.ptype(), |O| {
             match_each_integer_ptype!(sizes_primitive.ptype(), |S| {
-                let offsets_slice = offsets_primitive.as_slice::<O>();
-                let sizes_slice = sizes_primitive.as_slice::<S>();
-
-                for i in 0..offset_len {
-                    let start: usize = offsets_slice[i].as_();
-                    let size: usize = sizes_slice[i].as_();
-                    buf.fill_range(start, start + size, true);
-                }
+                fill_referenced_mask::<O, S>(
+                    &mut buf,
+                    offsets_primitive.as_slice::<O>(),
+                    sizes_primitive.as_slice::<S>(),
+                );
             })
         });
 
@@ -464,7 +485,7 @@ pub trait ListViewArrayExt: TypedArrayRef<ListView> {
     /// Exact for non-overlapping views, but overcounts when multiple views share the same elements.
     ///
     /// Returns `Ok(1.0)` when `elements` is empty.
-    fn estimate_density(&self, ctx: &mut ExecutionCtx) -> VortexResult<f32> {
+    fn upper_bound_density(&self, ctx: &mut ExecutionCtx) -> VortexResult<f32> {
         let n_elts = self.elements().len();
         if n_elts == 0 {
             return Ok(1.0);
@@ -479,10 +500,10 @@ pub trait ListViewArrayExt: TypedArrayRef<ListView> {
         let sizes_sum = sizes
             .statistics()
             .compute_stat(Stat::Sum, ctx)?
-            .ok_or_else(|| vortex_err!("Sum stat unavailable for sizes"))?
+            .vortex_expect("sizes array has integer ptype elements")
             .as_primitive()
             .as_::<u64>()
-            .ok_or_else(|| vortex_err!("could not cast sum of sizes to u64"))?;
+            .vortex_expect("integer ptypes can be upcast to u64");
 
         // if the same elements are referenced more than once the estimate may be
         // greater than 1.0, so clamp

vortex-array/src/arrays/listview/tests/density.rs

@@ -34,7 +34,7 @@ fn full_density_no_overlap() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_basic_listview();
     let exact = lv.compute_density(&mut ctx)?;
-    let est = lv.estimate_density(&mut ctx)?;
+    let est = lv.upper_bound_density(&mut ctx)?;
 
     assert!((exact - 1.0).abs() < EPS);
     assert!((est - 1.0).abs() < EPS);
@@ -46,7 +46,7 @@ fn sparse_no_overlap_matches_exact() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_large_listview();
     let exact = lv.compute_density(&mut ctx)?;
-    let est = lv.estimate_density(&mut ctx)?;
+    let est = lv.upper_bound_density(&mut ctx)?;
 
     assert!((exact - 0.5).abs() < EPS);
     assert!((est - 0.5).abs() < EPS);
@@ -58,7 +58,7 @@ fn all_empty_lists_is_zero_density() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_empty_lists_listview();
     let exact = lv.compute_density(&mut ctx)?;
-    let est = lv.estimate_density(&mut ctx)?;
+    let est = lv.upper_bound_density(&mut ctx)?;
 
     assert_eq!(exact, 0.0);
     assert_eq!(est, 0.0);
@@ -70,7 +70,7 @@ fn overlap_full_coverage_clamps_estimate() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_overlapping_listview();
     let exact = lv.compute_density(&mut ctx)?;
-    let est = lv.estimate_density(&mut ctx)?;
+    let est = lv.upper_bound_density(&mut ctx)?;
 
     assert!((exact - 1.0).abs() < EPS);
     assert!((est - 1.0).abs() < EPS);
@@ -83,7 +83,7 @@ fn overlap_differential_exact_lower_than_estimate() -> VortexResult<()> {
     let lv = create_sparse_overlapping_listview();
 
     let exact = lv.compute_density(&mut ctx)?;
-    let est = lv.estimate_density(&mut ctx)?;
+    let est = lv.upper_bound_density(&mut ctx)?;
 
     assert!((exact - 0.25).abs() < EPS);
     assert!((est - 0.40).abs() < EPS);
@@ -96,7 +96,7 @@ fn empty_elements_returns_one() -> VortexResult<()> {
     let lv = create_empty_elements_listview();
 
     let exact = lv.compute_density(&mut ctx)?;
-    let est = lv.estimate_density(&mut ctx)?;
+    let est = lv.upper_bound_density(&mut ctx)?;
 
     assert!((exact - 1.0).abs() < EPS);
     assert!((est - 1.0).abs() < EPS);
@@ -113,7 +113,7 @@ fn estimate_uses_cached_sum_stat() -> VortexResult<()> {
         .statistics()
         .set(Stat::Sum, Precision::Exact(ScalarValue::from(5u64)));
 
-    let est = lv.estimate_density(&mut ctx)?;
+    let est = lv.upper_bound_density(&mut ctx)?;
     assert!((est - 0.5).abs() < EPS);
     Ok(())
 }

vortex-array/src/arrow/executor/list_view.rs

@@ -34,7 +34,7 @@ pub(super) fn to_arrow_list_view<O: OffsetSizeTrait + IntegerPType>(
     // If the array is sufficiently sparse, rebuild before handing it to Arrow. Otherwise downstream
     // consumers hold an elements buffer containing unreferenced data in memory indefinitely,
     // and any compute pass over that buffer wastes work on data nothing references.
-    let density = array.estimate_density(ctx)?;
+    let density = array.upper_bound_density(ctx)?;
     let array = if density < DEFAULT_REBUILD_DENSITY_THRESHOLD {
         array.rebuild(ListViewRebuildMode::MakeZeroCopyToList)?
     } else {

vortex-duckdb/src/exporter/list_view.rs

@@ -55,7 +55,7 @@ pub(crate) fn new_exporter(
     // If the array is sufficiently sparse, rebuild. Otherwise the DuckDB vector will
     // hold an elements buffer containing unreferenced data in memory indefinitely,
     // and any compute pass over that buffer wastes work on data nothing references.
-    let density = array.estimate_density(ctx)?;
+    let density = array.upper_bound_density(ctx)?;
     let array = if density < DEFAULT_REBUILD_DENSITY_THRESHOLD {
         array.rebuild(ListViewRebuildMode::MakeZeroCopyToList)?
     } else {