address comments

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

Author: Matthew Katz

vortex-array/public-api.lock

@@ -4444,15 +4444,17 @@ pub vortex_array::arrays::listview::ListViewDataParts::sizes: vortex_array::Arra
 
 pub vortex_array::arrays::listview::ListViewDataParts::validity: vortex_array::validity::Validity
 
+pub const vortex_array::arrays::listview::DEFAULT_REBUILD_DENSITY_THRESHOLD: f32
+
 pub trait vortex_array::arrays::listview::ListViewArrayExt: vortex_array::TypedArrayRef<vortex_array::arrays::ListView>
 
-pub fn vortex_array::arrays::listview::ListViewArrayExt::compute_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<core::option::Option<f32>>
+pub fn vortex_array::arrays::listview::ListViewArrayExt::compute_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<f32>
 
-pub fn vortex_array::arrays::listview::ListViewArrayExt::compute_referenced_elements_mask(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<core::option::Option<vortex_mask::Mask>>
+pub fn vortex_array::arrays::listview::ListViewArrayExt::compute_referenced_elements_mask(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<vortex_mask::Mask>
 
 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<core::option::Option<f32>>
+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>
 
@@ -4472,13 +4474,13 @@ pub fn vortex_array::arrays::listview::ListViewArrayExt::verify_is_zero_copy_to_
 
 impl<T: vortex_array::TypedArrayRef<vortex_array::arrays::ListView>> vortex_array::arrays::listview::ListViewArrayExt for T
 
-pub fn T::compute_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<core::option::Option<f32>>
+pub fn T::compute_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<f32>
 
-pub fn T::compute_referenced_elements_mask(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<core::option::Option<vortex_mask::Mask>>
+pub fn T::compute_referenced_elements_mask(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<vortex_mask::Mask>
 
 pub fn T::elements(&self) -> &vortex_array::ArrayRef
 
-pub fn T::estimate_density(&self, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<core::option::Option<f32>>
+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>
 
@@ -23966,8 +23968,6 @@ impl vortex_array::Array<vortex_array::arrays::ListView>
 
 pub fn vortex_array::Array<vortex_array::arrays::ListView>::rebuild(&self, vortex_array::arrays::listview::ListViewRebuildMode) -> vortex_error::VortexResult<vortex_array::arrays::ListViewArray>
 
-pub fn vortex_array::Array<vortex_array::arrays::ListView>::should_rebuild(&self, bool, &mut vortex_array::ExecutionCtx) -> vortex_error::VortexResult<bool>
-
 impl vortex_array::Array<vortex_array::arrays::Masked>
 
 pub fn vortex_array::Array<vortex_array::arrays::Masked>::try_new(vortex_array::ArrayRef, vortex_array::validity::Validity) -> vortex_error::VortexResult<Self>

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

@@ -407,16 +407,13 @@ pub trait ListViewArrayExt: TypedArrayRef<ListView> {
     /// Walks every `(offset, size)` pair, canonicalizes both `offsets` and `sizes`,
     /// and allocates a `BitBuffer` of length `elements.len()`, so it is extremely costly.
     ///
-    /// Returns `Ok(None)` when `elements` is empty.
-    #[allow(clippy::cognitive_complexity, clippy::unnecessary_fallible_conversions)]
-    fn compute_referenced_elements_mask(
-        &self,
-        ctx: &mut ExecutionCtx,
-    ) -> VortexResult<Option<Mask>> {
+    /// **Preconditions**
+    ///
+    /// Assumes that `self.elements()` is non-empty.
+    #[allow(clippy::cognitive_complexity)]
+    fn compute_referenced_elements_mask(&self, ctx: &mut ExecutionCtx) -> VortexResult<Mask> {
+        debug_assert!(!self.elements().is_empty());
         let len = self.elements().len();
-        if len == 0 {
-            return Ok(None);
-        }
 
         let offsets_primitive = self.offsets().clone().execute::<PrimitiveArray>(ctx)?;
         let sizes_primitive = self.sizes().clone().execute::<PrimitiveArray>(ctx)?;
@@ -430,49 +427,55 @@ pub trait ListViewArrayExt: TypedArrayRef<ListView> {
                 let sizes_slice = sizes_primitive.as_slice::<S>();
 
                 for i in 0..offset_len {
-                    let start =
-                        usize::try_from(offsets_slice[i]).vortex_expect("offset must fit in usize");
-                    let size =
-                        usize::try_from(sizes_slice[i]).vortex_expect("size must fit in usize");
+                    let start: usize = offsets_slice[i].as_();
+                    let size: usize = sizes_slice[i].as_();
                     buf.fill_range(start, start + size, true);
                 }
             })
         });
 
-        Ok(Some(Mask::from_buffer(buf.freeze())))
+        Ok(Mask::from_buffer(buf.freeze()))
     }
 
     /// Exact fraction of `elements` referenced by some view, in `[0.0, 1.0]`. Extremely costly.
     ///
-    /// Returns `Ok(None)` when `elements` is empty.
-    fn compute_density(&self, ctx: &mut ExecutionCtx) -> VortexResult<Option<f32>> {
-        Ok(self
-            .compute_referenced_elements_mask(ctx)?
-            .map(|mask| match mask {
-                Mask::AllTrue(_) => 1.0,
-                Mask::AllFalse(_) => 0.0,
-                Mask::Values(values) => values.true_count() as f32 / self.elements().len() as f32,
-            }))
+    /// Returns `Ok(1.0)` when `elements` is empty.
+    fn compute_density(&self, ctx: &mut ExecutionCtx) -> VortexResult<f32> {
+        if self.elements().is_empty() {
+            return Ok(1.0);
+        }
+
+        if self.sizes().is_empty() {
+            return Ok(0.0);
+        }
+
+        let density = match self.compute_referenced_elements_mask(ctx)? {
+            Mask::AllTrue(_) => 1.0,
+            Mask::AllFalse(_) => 0.0,
+            Mask::Values(values) => values.true_count() as f32 / self.elements().len() as f32,
+        };
+
+        Ok(density)
     }
 
     /// Upper-bound estimate of [`compute_density`](Self::compute_density) via
     /// `sum(sizes) / elements.len()`, clamped to `[0.0, 1.0]`.
     ///
     /// Exact for non-overlapping views, but overcounts when multiple views share the same elements.
     ///
-    /// Returns `Ok(None)` when `elements` is empty.
-    fn estimate_density(&self, ctx: &mut ExecutionCtx) -> VortexResult<Option<f32>> {
+    /// Returns `Ok(1.0)` when `elements` is empty.
+    fn estimate_density(&self, ctx: &mut ExecutionCtx) -> VortexResult<f32> {
         let n_elts = self.elements().len();
-        if n_elts == 0 {
-            return Ok(None);
+        if self.elements().is_empty() {
+            return Ok(1.0);
         }
 
         let sizes = self.sizes();
         if sizes.is_empty() {
-            return Ok(Some(0.0));
+            return Ok(0.0);
         }
 
-        // compute_stat short-circuits on a cached exact Sum and otherwise computes-and-caches.
+        // compute_stat short-circuits on a cached exact Sum and otherwise computes
         let sizes_sum = sizes
             .statistics()
             .compute_stat(Stat::Sum, ctx)?
@@ -481,9 +484,11 @@ pub trait ListViewArrayExt: TypedArrayRef<ListView> {
             .as_::<u64>()
             .ok_or_else(|| vortex_err!("could not cast sum of sizes to u64"))?;
 
-        let estimate = (sizes_sum as f32 / n_elts as f32).clamp(0.0, 1.0);
+        let estimate = (sizes_sum as f32 / n_elts as f32).min(1.0);
+
+        debug_assert!(estimate >= 0.0);
 
-        Ok(Some(estimate))
+        Ok(estimate)
     }
 }
 impl<T: TypedArrayRef<ListView>> ListViewArrayExt for T {}

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

@@ -18,6 +18,7 @@ pub use conversion::list_view_from_list;
 pub use conversion::recursive_list_from_list_view;
 
 mod rebuild;
+pub use rebuild::DEFAULT_REBUILD_DENSITY_THRESHOLD;
 pub use rebuild::ListViewRebuildMode;
 
 #[cfg(test)]

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

@@ -6,7 +6,6 @@ use vortex_buffer::BufferMut;
 use vortex_error::VortexExpect;
 use vortex_error::VortexResult;
 
-use crate::ExecutionCtx;
 use crate::IntoArray;
 use crate::LEGACY_SESSION;
 #[expect(deprecated)]
@@ -31,7 +30,10 @@ use crate::scalar_fn::fns::operators::Operator;
 /// A `ListViewArray` can accumulate unreferenced bytes in its `elements` buffer after
 /// metadata-only operations like `take` and `filter`. When density (referenced fraction of `elements`)
 /// falls below this threshold, the benefits of a rebuild may outweigh its cost.
-const REBUILD_DENSITY_THRESHOLD: f32 = 0.1;
+///
+/// This is a somewhat arbitrary rule-of-thumb and may be suboptimal depending on different use cases and
+/// list element dtypes.
+pub const DEFAULT_REBUILD_DENSITY_THRESHOLD: f32 = 0.1;
 
 /// Modes for rebuilding a [`ListViewArray`].
 pub enum ListViewRebuildMode {
@@ -384,16 +386,6 @@ impl ListViewArray {
             self.rebuild_zero_copy_to_list()
         }
     }
-
-    pub fn should_rebuild(&self, exact: bool, ctx: &mut ExecutionCtx) -> VortexResult<bool> {
-        let density = if exact {
-            self.compute_density(ctx)?
-        } else {
-            self.estimate_density(ctx)?
-        };
-
-        Ok(density.unwrap_or(1.0) < REBUILD_DENSITY_THRESHOLD)
-    }
 }
 
 #[cfg(test)]

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

@@ -33,8 +33,8 @@ fn test_execution_ctx() -> ExecutionCtx {
 fn full_density_no_overlap() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_basic_listview();
-    let exact = lv.compute_density(&mut ctx)?.expect("non-empty elements");
-    let est = lv.estimate_density(&mut ctx)?.expect("non-empty elements");
+    let exact = lv.compute_density(&mut ctx)?;
+    let est = lv.estimate_density(&mut ctx)?;
 
     assert!((exact - 1.0).abs() < EPS);
     assert!((est - 1.0).abs() < EPS);
@@ -45,8 +45,8 @@ fn full_density_no_overlap() -> VortexResult<()> {
 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)?.expect("non-empty");
-    let est = lv.estimate_density(&mut ctx)?.expect("non-empty");
+    let exact = lv.compute_density(&mut ctx)?;
+    let est = lv.estimate_density(&mut ctx)?;
 
     assert!((exact - 0.5).abs() < EPS);
     assert!((est - 0.5).abs() < EPS);
@@ -57,12 +57,8 @@ fn sparse_no_overlap_matches_exact() -> VortexResult<()> {
 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)?
-        .expect("elements has length 1");
-    let est = lv
-        .estimate_density(&mut ctx)?
-        .expect("elements has length 1");
+    let exact = lv.compute_density(&mut ctx)?;
+    let est = lv.estimate_density(&mut ctx)?;
 
     assert_eq!(exact, 0.0);
     assert_eq!(est, 0.0);
@@ -73,8 +69,8 @@ fn all_empty_lists_is_zero_density() -> VortexResult<()> {
 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)?.expect("non-empty");
-    let est = lv.estimate_density(&mut ctx)?.expect("non-empty");
+    let exact = lv.compute_density(&mut ctx)?;
+    let est = lv.estimate_density(&mut ctx)?;
 
     assert!((exact - 1.0).abs() < EPS);
     assert!((est - 1.0).abs() < EPS);
@@ -86,21 +82,24 @@ fn overlap_differential_exact_lower_than_estimate() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_sparse_overlapping_listview();
 
-    let exact = lv.compute_density(&mut ctx)?.expect("non-empty");
-    let est = lv.estimate_density(&mut ctx)?.expect("non-empty");
+    let exact = lv.compute_density(&mut ctx)?;
+    let est = lv.estimate_density(&mut ctx)?;
 
     assert!((exact - 0.25).abs() < EPS);
     assert!((est - 0.40).abs() < EPS);
     Ok(())
 }
 
 #[test]
-fn empty_elements_returns_none() -> VortexResult<()> {
+fn empty_elements_returns_zero() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_empty_elements_listview();
 
-    assert!(lv.compute_density(&mut ctx)?.is_none());
-    assert!(lv.estimate_density(&mut ctx)?.is_none());
+    let exact = lv.compute_density(&mut ctx)?;
+    let est = lv.estimate_density(&mut ctx)?;
+
+    assert!(exact.abs() < EPS);
+    assert!(est.abs() < EPS);
     Ok(())
 }
 
@@ -114,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)?.expect("non-empty");
+    let est = lv.estimate_density(&mut ctx)?;
     assert!((est - 0.5).abs() < EPS);
     Ok(())
 }
@@ -123,9 +122,7 @@ fn estimate_uses_cached_sum_stat() -> VortexResult<()> {
 fn referenced_mask_set_bits_match_views() -> VortexResult<()> {
     let mut ctx = test_execution_ctx();
     let lv = create_sparse_overlapping_listview();
-    let mask = lv
-        .compute_referenced_elements_mask(&mut ctx)?
-        .expect("non-empty elements");
+    let mask = lv.compute_referenced_elements_mask(&mut ctx)?;
     let bits = match mask {
         Mask::Values(v) => v,
         _ => panic!("expected Values mask"),

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

@@ -13,6 +13,8 @@ use crate::ArrayRef;
 use crate::ExecutionCtx;
 use crate::arrays::ListViewArray;
 use crate::arrays::PrimitiveArray;
+use crate::arrays::listview::DEFAULT_REBUILD_DENSITY_THRESHOLD;
+use crate::arrays::listview::ListViewArrayExt;
 use crate::arrays::listview::ListViewDataParts;
 use crate::arrays::listview::ListViewRebuildMode;
 use crate::arrow::executor::validity::to_arrow_null_buffer;
@@ -29,8 +31,11 @@ pub(super) fn to_arrow_list_view<O: OffsetSizeTrait + IntegerPType>(
 ) -> VortexResult<arrow_array::ArrayRef> {
     let array = array.execute::<ListViewArray>(ctx)?;
 
-    // If array is sparse, rebuild before handing it to Arrow
-    let array = if array.should_rebuild(false, ctx)? {
+    // 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 array = if density < DEFAULT_REBUILD_DENSITY_THRESHOLD {
         array.rebuild(ListViewRebuildMode::MakeZeroCopyToList)?
     } else {
         array

vortex-duckdb/src/exporter/list_view.rs

@@ -10,6 +10,8 @@ use parking_lot::Mutex;
 use vortex::array::ExecutionCtx;
 use vortex::array::arrays::ListViewArray;
 use vortex::array::arrays::PrimitiveArray;
+use vortex::array::arrays::listview::DEFAULT_REBUILD_DENSITY_THRESHOLD;
+use vortex::array::arrays::listview::ListViewArrayExt;
 use vortex::array::arrays::listview::ListViewDataParts;
 use vortex::array::arrays::listview::ListViewRebuildMode;
 use vortex::array::match_each_integer_ptype;
@@ -50,21 +52,25 @@ pub(crate) fn new_exporter(
     cache: &ConversionCache,
     ctx: &mut ExecutionCtx,
 ) -> VortexResult<Box<dyn ColumnExporter>> {
-    let len = array.len();
-
-    let compact_array = if array.should_rebuild(false, ctx)? {
+    // 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 array = if density < DEFAULT_REBUILD_DENSITY_THRESHOLD {
         array.rebuild(ListViewRebuildMode::MakeZeroCopyToList)?
     } else {
         array
     };
 
+    let len = array.len();
+
     let ListViewDataParts {
         elements_dtype,
         elements,
         offsets,
         sizes,
         validity,
-    } = compact_array.into_data_parts();
+    } = array.into_data_parts();
     // Cache an `elements` vector up front so that future exports can reference it.
     let num_elements = elements.len();