Remove unused take_into function (#3236)

gatesn · web-flow · commit 9b9f0459ae9b · 2025-05-07T11:42:57.000+01:00
Believe it or not, this is never actually called anywhere by Vortex
scans.

Happy to look into adding `invoke_into` to all compute functions in the
future if it's warranted, but for now it breaks the compute function API
for something that isn't used anywhere.

The benchmarks degrade, but they exercise a code path that isn't used in
practice. So not too bothered about them at the moment.
diff --git a/encodings/dict/src/array.rs b/encodings/dict/src/array.rs
@@ -1,8 +1,7 @@
 use std::fmt::Debug;
 
 use arrow_buffer::BooleanBuffer;
-use vortex_array::builders::ArrayBuilder;
-use vortex_array::compute::{cast, take, take_into};
+use vortex_array::compute::{cast, take};
 use vortex_array::stats::{ArrayStats, StatsSetRef};
 use vortex_array::variants::PrimitiveArrayTrait;
 use vortex_array::vtable::VTableRef;
@@ -107,22 +106,6 @@ impl ArrayCanonicalImpl for DictArray {
             _ => take(self.values(), self.codes())?.to_canonical(),
         }
     }
-
-    fn _append_to_builder(&self, builder: &mut dyn ArrayBuilder) -> VortexResult<()> {
-        match self.dtype() {
-            // NOTE: Utf8 and Binary will decompress into VarBinViewArray, which requires a full
-            // decompression to construct the views child array.
-            // For this case, it is *always* faster to decompress the values first and then create
-            // copies of the view pointers.
-            // TODO(joe): is the above still true?, investigate this.
-            DType::Utf8(_) | DType::Binary(_) => {
-                let canonical_values: ArrayRef = self.values().to_canonical()?.into_array();
-                take_into(&canonical_values, self.codes(), builder)
-            }
-            // Non-string case: take and then canonicalize
-            _ => take_into(self.values(), self.codes(), builder),
-        }
-    }
 }
 
 impl ArrayValidityImpl for DictArray {
diff --git a/encodings/fsst/src/compute/mod.rs b/encodings/fsst/src/compute/mod.rs
@@ -2,7 +2,6 @@ mod compare;
 mod filter;
 
 use vortex_array::arrays::VarBinArray;
-use vortex_array::builders::ArrayBuilder;
 use vortex_array::compute::{TakeFn, fill_null, take};
 use vortex_array::vtable::ComputeVTable;
 use vortex_array::{Array, ArrayExt, ArrayRef};
@@ -35,13 +34,4 @@ impl TakeFn<&FSSTArray> for FSSTEncoding {
         )?
         .into_array())
     }
-
-    fn take_into(
-        &self,
-        array: &FSSTArray,
-        indices: &dyn Array,
-        builder: &mut dyn ArrayBuilder,
-    ) -> VortexResult<()> {
-        builder.extend_from_array(&take(array, indices)?)
-    }
 }
diff --git a/vortex-array/src/arrays/bool/compute/take.rs b/vortex-array/src/arrays/bool/compute/take.rs
@@ -7,7 +7,6 @@ use vortex_mask::Mask;
 use vortex_scalar::Scalar;
 
 use crate::arrays::{BoolArray, BoolEncoding, ConstantArray};
-use crate::builders::ArrayBuilder;
 use crate::compute::{TakeFn, fill_null};
 use crate::variants::PrimitiveArrayTrait;
 use crate::{Array, ArrayRef, ToCanonical};
@@ -32,15 +31,6 @@ impl TakeFn<&BoolArray> for BoolEncoding {
 
         Ok(BoolArray::new(buffer, array.validity().take(indices)?).into_array())
     }
-
-    fn take_into(
-        &self,
-        array: &BoolArray,
-        indices: &dyn Array,
-        builder: &mut dyn ArrayBuilder,
-    ) -> VortexResult<()> {
-        builder.extend_from_array(&self.take(array, indices)?)
-    }
 }
 
 fn take_valid_indices<I: AsPrimitive<usize>>(
diff --git a/vortex-array/src/arrays/primitive/compute/take.rs b/vortex-array/src/arrays/primitive/compute/take.rs
@@ -7,12 +7,10 @@ use vortex_dtype::{
     NativePType, Nullability, PType, match_each_integer_ptype, match_each_native_ptype,
     match_each_native_simd_ptype, match_each_unsigned_integer_ptype,
 };
-use vortex_error::{VortexResult, vortex_err};
-use vortex_mask::Mask;
+use vortex_error::VortexResult;
 
 use crate::arrays::PrimitiveEncoding;
 use crate::arrays::primitive::PrimitiveArray;
-use crate::builders::{ArrayBuilder, PrimitiveBuilder};
 use crate::compute::TakeFn;
 use crate::variants::PrimitiveArrayTrait;
 use crate::{Array, ArrayRef, ToCanonical};
@@ -51,43 +49,6 @@ impl TakeFn<&PrimitiveArray> for PrimitiveEncoding {
             })
         })
     }
-
-    fn take_into(
-        &self,
-        array: &PrimitiveArray,
-        indices: &dyn Array,
-        builder: &mut dyn ArrayBuilder,
-    ) -> VortexResult<()> {
-        let indices = indices.to_primitive()?;
-        let mask = array.validity().take(&indices)?.to_mask(indices.len())?;
-
-        match_each_native_ptype!(array.ptype(), |$T| {
-            match_each_integer_ptype!(indices.ptype(), |$I| {
-                take_into_impl(array.as_slice::<$T>(), indices.as_slice::<$I>(), mask, builder)
-            })
-        })
-    }
-}
-
-fn take_into_impl<T: NativePType, I: NativePType + AsPrimitive<usize>>(
-    array: &[T],
-    indices: &[I],
-    mask: Mask,
-    builder: &mut dyn ArrayBuilder,
-) -> VortexResult<()> {
-    assert_eq!(indices.len(), mask.len());
-
-    let builder = builder
-        .as_any_mut()
-        .downcast_mut::<PrimitiveBuilder<T>>()
-        .ok_or_else(|| {
-            vortex_err!(
-                "Failed to downcast builder to PrimitiveBuilder<{}>",
-                T::PTYPE
-            )
-        })?;
-    builder.extend_with_iterator(indices.iter().map(|idx| array[idx.as_()]), mask);
-    Ok(())
 }
 
 fn take_primitive<T: NativePType, I: NativePType + AsPrimitive<usize>>(
@@ -167,14 +128,12 @@ where
 #[cfg(test)]
 mod test {
     use vortex_buffer::buffer;
-    use vortex_dtype::Nullability;
     use vortex_scalar::Scalar;
 
     use crate::array::Array;
     use crate::arrays::primitive::compute::take::take_primitive;
     use crate::arrays::{BoolArray, PrimitiveArray};
-    use crate::builders::{ArrayBuilder as _, PrimitiveBuilder};
-    use crate::compute::{take, take_into};
+    use crate::compute::take;
     use crate::validity::Validity;
 
     #[test]
@@ -201,50 +160,4 @@ mod test {
         // the third index is null
         assert_eq!(actual.scalar_at(2).unwrap(), Scalar::null_typed::<i32>());
     }
-
-    #[test]
-    fn test_take_into() {
-        let values = PrimitiveArray::new(buffer![1i32, 2, 3, 4, 5], Validity::NonNullable);
-        let all_valid_indices = PrimitiveArray::new(
-            buffer![0, 3, 4],
-            Validity::Array(BoolArray::from_iter([true, true, true]).into_array()),
-        );
-        let mut builder = PrimitiveBuilder::<i32>::new(Nullability::Nullable);
-        take_into(&values, &all_valid_indices, &mut builder).unwrap();
-        let actual = builder.finish();
-        assert_eq!(actual.scalar_at(0).unwrap(), Scalar::from(Some(1)));
-        assert_eq!(actual.scalar_at(1).unwrap(), Scalar::from(Some(4)));
-        assert_eq!(actual.scalar_at(2).unwrap(), Scalar::from(Some(5)));
-
-        let mixed_valid_indices = PrimitiveArray::new(
-            buffer![0, 3, 4],
-            Validity::Array(BoolArray::from_iter([true, true, false]).into_array()),
-        );
-        let mut builder = PrimitiveBuilder::<i32>::new(Nullability::Nullable);
-        take_into(&values, &mixed_valid_indices, &mut builder).unwrap();
-        let actual = builder.finish();
-        assert_eq!(actual.scalar_at(0).unwrap(), Scalar::from(Some(1)));
-        assert_eq!(actual.scalar_at(1).unwrap(), Scalar::from(Some(4)));
-        // the third index is null
-        assert_eq!(actual.scalar_at(2).unwrap(), Scalar::null_typed::<i32>());
-
-        let all_invalid_indices = PrimitiveArray::new(
-            buffer![0, 3, 4],
-            Validity::Array(BoolArray::from_iter([false, false, false]).into_array()),
-        );
-        let mut builder = PrimitiveBuilder::<i32>::new(Nullability::Nullable);
-        take_into(&values, &all_invalid_indices, &mut builder).unwrap();
-        let actual = builder.finish();
-        assert_eq!(actual.scalar_at(0).unwrap(), Scalar::null_typed::<i32>());
-        assert_eq!(actual.scalar_at(1).unwrap(), Scalar::null_typed::<i32>());
-        assert_eq!(actual.scalar_at(2).unwrap(), Scalar::null_typed::<i32>());
-
-        let non_null_indices = PrimitiveArray::new(buffer![0, 3, 4], Validity::NonNullable);
-        let mut builder = PrimitiveBuilder::<i32>::new(Nullability::NonNullable);
-        take_into(&values, &non_null_indices, &mut builder).unwrap();
-        let actual = builder.finish();
-        assert_eq!(actual.scalar_at(0).unwrap(), Scalar::from(1));
-        assert_eq!(actual.scalar_at(1).unwrap(), Scalar::from(4));
-        assert_eq!(actual.scalar_at(2).unwrap(), Scalar::from(5));
-    }
 }
diff --git a/vortex-array/src/arrays/varbinview/compute/mod.rs b/vortex-array/src/arrays/varbinview/compute/mod.rs
@@ -24,10 +24,9 @@ mod tests {
     use crate::accessor::ArrayAccessor;
     use crate::array::Array;
     use crate::arrays::VarBinViewArray;
-    use crate::builders::{ArrayBuilder, VarBinViewBuilder};
     use crate::canonical::ToCanonical;
     use crate::compute::conformance::mask::test_mask;
-    use crate::compute::{take, take_into};
+    use crate::compute::take;
 
     #[test]
     fn take_nullable() {
@@ -69,33 +68,4 @@ mod tests {
             Some("five"),
         ]));
     }
-
-    #[test]
-    fn take_into_nullable() {
-        let arr = VarBinViewArray::from_iter_nullable_str([
-            Some("one"),
-            None,
-            Some("three"),
-            Some("four"),
-            None,
-            Some("six"),
-        ]);
-
-        let mut builder = VarBinViewBuilder::with_capacity(arr.dtype().clone(), arr.len());
-
-        take_into(&arr, &buffer![0, 3].into_array(), &mut builder).unwrap();
-
-        let taken = builder.finish();
-        assert!(taken.dtype().is_nullable());
-        assert_eq!(
-            taken
-                .to_varbinview()
-                .unwrap()
-                .with_iterator(|it| it
-                    .map(|v| v.map(|b| unsafe { String::from_utf8_unchecked(b.to_vec()) }))
-                    .collect::<Vec<_>>())
-                .unwrap(),
-            [Some("one".to_string()), Some("four".to_string())]
-        );
-    }
 }
diff --git a/vortex-array/src/arrays/varbinview/compute/take.rs b/vortex-array/src/arrays/varbinview/compute/take.rs
@@ -1,13 +1,11 @@
 use std::ops::Deref;
 
 use num_traits::AsPrimitive;
-use vortex_buffer::{Buffer, ByteBuffer};
+use vortex_buffer::Buffer;
 use vortex_dtype::match_each_integer_ptype;
-use vortex_error::{VortexResult, vortex_bail};
-use vortex_mask::Mask;
+use vortex_error::VortexResult;
 
 use crate::arrays::{BinaryView, VarBinViewArray, VarBinViewEncoding};
-use crate::builders::{ArrayBuilder, VarBinViewBuilder};
 use crate::compute::TakeFn;
 use crate::variants::PrimitiveArrayTrait;
 use crate::{Array, ArrayRef, ToCanonical};
@@ -37,58 +35,6 @@ impl TakeFn<&VarBinViewArray> for VarBinViewEncoding {
         )?
         .into_array())
     }
-
-    fn take_into(
-        &self,
-        array: &VarBinViewArray,
-        indices: &dyn Array,
-        builder: &mut dyn ArrayBuilder,
-    ) -> VortexResult<()> {
-        if array.len() == 0 {
-            vortex_bail!("Cannot take_into from an empty array");
-        }
-
-        let Some(builder) = builder.as_any_mut().downcast_mut::<VarBinViewBuilder>() else {
-            vortex_bail!(
-                "Cannot take_into a non-varbinview builder {:?}",
-                builder.as_any().type_id()
-            );
-        };
-        // Compute the new validity
-
-        // This is valid since all elements (of all arrays) even null values are inside must be the
-        // min-max valid range.
-        let validity = array.validity().take(indices)?;
-        let mask = validity.to_mask(indices.len())?;
-        let indices = indices.to_primitive()?;
-
-        match_each_integer_ptype!(indices.ptype(), |$I| {
-            // This is valid since all elements even null values are inside the min-max valid range.
-            take_views_into(array.views(), array.buffers(), indices.as_slice::<$I>(), mask, builder)?;
-        });
-
-        Ok(())
-    }
-}
-
-fn take_views_into<I: AsPrimitive<usize>>(
-    views: &Buffer<BinaryView>,
-    buffers: &[ByteBuffer],
-    indices: &[I],
-    mask: Mask,
-    builder: &mut VarBinViewBuilder,
-) -> VortexResult<()> {
-    let buffers_offset = u32::try_from(builder.completed_block_count())?;
-    // NOTE(ngates): this deref is not actually trivial, so we run it once.
-    let views_ref = views.deref();
-    builder.push_buffer_and_adjusted_views(
-        buffers.iter().cloned(),
-        indices
-            .iter()
-            .map(|i| views_ref[i.as_()].offset_view(buffers_offset)),
-        mask,
-    );
-    Ok(())
 }
 
 fn take_views<I: AsPrimitive<usize>>(
diff --git a/vortex-array/src/compute/mod.rs b/vortex-array/src/compute/mod.rs
@@ -28,7 +28,7 @@ pub use nan_count::*;
 pub use numeric::*;
 pub use search_sorted::*;
 pub use sum::*;
-pub use take::{TakeFn, take, take_into};
+pub use take::{TakeFn, take};
 pub use take_from::TakeFromFn;
 use vortex_dtype::DType;
 use vortex_error::{VortexError, VortexExpect, VortexResult, vortex_bail, vortex_err};
diff --git a/vortex-array/src/compute/take.rs b/vortex-array/src/compute/take.rs
