Clarify docs in session accessors

Signed-off-by: Nicholas Gates <[email protected]>

Author: gatesn

vortex-array/src/arrays/filter/vtable.rs

@@ -7,24 +7,15 @@ use std::ops::Range;
 use vortex_buffer::BufferHandle;
 use vortex_compute::filter::Filter;
 use vortex_dtype::DType;
+use vortex_error::vortex_bail;
 use vortex_error::VortexExpect;
 use vortex_error::VortexResult;
-use vortex_error::vortex_bail;
 use vortex_mask::Mask;
 use vortex_scalar::Scalar;
 
-use crate::Array;
-use crate::ArrayBufferVisitor;
-use crate::ArrayChildVisitor;
-use crate::ArrayEq;
-use crate::ArrayHash;
-use crate::ArrayRef;
-use crate::Canonical;
-use crate::IntoArray;
-use crate::Precision;
-use crate::arrays::LEGACY_SESSION;
 use crate::arrays::filter::array::FilterArray;
 use crate::arrays::filter::kernel::FilterKernel;
+use crate::arrays::LEGACY_SESSION;
 use crate::kernel::BindCtx;
 use crate::kernel::KernelRef;
 use crate::kernel::PushDownResult;
@@ -42,6 +33,15 @@ use crate::vtable::OperationsVTable;
 use crate::vtable::VTable;
 use crate::vtable::ValidityVTable;
 use crate::vtable::VisitorVTable;
+use crate::Array;
+use crate::ArrayBufferVisitor;
+use crate::ArrayChildVisitor;
+use crate::ArrayEq;
+use crate::ArrayHash;
+use crate::ArrayRef;
+use crate::Canonical;
+use crate::IntoArray;
+use crate::Precision;
 
 vtable!(Filter);
 
@@ -108,15 +108,19 @@ impl VTable for FilterVTable {
             pushdown_cost
         );
 
-        if pushdown_cost < full_cost {
+        // NOTE(ngates): for now we keep the same behavior as develop where we push-down any
+        //  query with <20% true values.
+        let pushdown = (pushdown_cost < full_cost) || (array.mask.density() < 0.2);
+
+        if pushdown {
             // Try to push down the filter to the child if it's cheaper.
             child = match child.push_down_filter(&mask)? {
                 PushDownResult::Pushed(new_k) => {
                     log::debug!("Filter push down kernel:\n{:?}", new_k);
                     return Ok(new_k);
                 }
                 PushDownResult::NotPushed(child) => {
-                    log::debug!(
+                    log::warn!(
                         "Filter pushdown was cheaper but not supported by child array {}",
                         array.child.display_tree()
                     );

vortex-array/src/arrays/scalar_fn/kernel.rs

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
-use itertools::Itertools;
 use vortex_dtype::DType;
 use vortex_error::VortexResult;
 use vortex_mask::Mask;
@@ -40,14 +39,17 @@ pub(super) enum KernelInput {
 
 impl Kernel for ScalarFnKernel {
     fn execute(self: Box<Self>) -> VortexResult<Vector> {
-        let datums: Vec<_> = self
-            .inputs
-            .into_iter()
-            .map(|input| match input {
-                KernelInput::Scalar(s) => Ok(Datum::Scalar(s)),
-                KernelInput::Vector(k) => k.execute().map(Datum::Vector),
-            })
-            .try_collect()?;
+        let mut datums: Vec<Datum> = Vec::with_capacity(self.inputs.len());
+        for input in self.inputs {
+            match input {
+                KernelInput::Scalar(s) => {
+                    datums.push(Datum::Scalar(s));
+                }
+                KernelInput::Vector(kernel) => {
+                    datums.push(Datum::Vector(kernel.execute()?));
+                }
+            }
+        }
 
         let args = ExecutionArgs {
             datums,
@@ -96,7 +98,7 @@ impl Kernel for ScalarFnKernel {
             scalar_fn: self.scalar_fn,
             inputs: new_inputs,
             input_dtypes: self.input_dtypes,
-            row_count: self.row_count,
+            row_count: selection.true_count(),
             return_dtype: self.return_dtype,
         })))
     }

vortex-array/src/arrays/scalar_fn/vtable/canonical.rs

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
-use itertools::Itertools;
 use vortex_error::VortexExpect;
 
 use crate::arrays::scalar_fn::array::ScalarFnArray;
@@ -17,15 +16,16 @@ use crate::Canonical;
 impl CanonicalVTable<ScalarFnVTable> for ScalarFnVTable {
     fn canonicalize(array: &ScalarFnArray) -> Canonical {
         let child_dtypes: Vec<_> = array.children.iter().map(|c| c.dtype().clone()).collect();
-        let child_datums: Vec<_> = array
-            .children()
-            .iter()
-            .map(|child| child.execute_datum_optimized(&LEGACY_SESSION))
-            .try_collect()
-            // FIXME(ngates): canonicalizing really ought to be fallible
-            .vortex_expect(
-                "Failed to execute child array during canonicalization of ScalarFnArray",
-            );
+
+        let mut child_datums = Vec::with_capacity(array.children.len());
+        for child in array.children.iter() {
+            let datum = child
+                .execute_datum_optimized(&LEGACY_SESSION)
+                .vortex_expect(
+                    "Failed to execute child array during canonicalization of ScalarFnArray",
+                );
+            child_datums.push(datum);
+        }
 
         let ctx = ExecutionArgs {
             datums: child_datums,

vortex-array/src/expr/exprs/get_item.rs

@@ -10,7 +10,6 @@ use vortex_dtype::FieldName;
 use vortex_dtype::FieldPath;
 use vortex_dtype::Nullability;
 use vortex_error::vortex_err;
-use vortex_error::vortex_panic;
 use vortex_error::VortexExpect;
 use vortex_error::VortexResult;
 use vortex_mask::Mask;

vortex-array/src/expr/exprs/pack.rs

@@ -17,14 +17,12 @@ use vortex_error::VortexExpect;
 use vortex_error::VortexResult;
 use vortex_mask::Mask;
 use vortex_proto::expr as pb;
+use vortex_vector::struct_::StructVector;
 use vortex_vector::Datum;
 use vortex_vector::ScalarOps;
 use vortex_vector::VectorMutOps;
 use vortex_vector::VectorOps;
-use vortex_vector::struct_::StructVector;
 
-use crate::ArrayRef;
-use crate::IntoArray;
 use crate::arrays::StructArray;
 use crate::expr::Arity;
 use crate::expr::ChildName;
@@ -34,6 +32,8 @@ use crate::expr::Expression;
 use crate::expr::VTable;
 use crate::expr::VTableExt;
 use crate::validity::Validity;
+use crate::ArrayRef;
+use crate::IntoArray;
 
 /// Pack zero or more expressions into a structure with named fields.
 pub struct Pack;
@@ -157,7 +157,7 @@ impl VTable for Pack {
                 .map(|v| v.ensure_vector(args.row_count))
                 .collect();
             return Ok(Datum::Vector(
-                StructVector::new(Arc::new(fields), Mask::new_true(args.row_count)).into(),
+                StructVector::try_new(Arc::new(fields), Mask::new_true(args.row_count))?.into(),
             ));
         }
 
@@ -219,22 +219,22 @@ pub fn pack(
 mod tests {
     use vortex_buffer::buffer;
     use vortex_dtype::Nullability;
-    use vortex_error::VortexResult;
     use vortex_error::vortex_bail;
+    use vortex_error::VortexResult;
 
+    use super::pack;
     use super::Pack;
     use super::PackOptions;
-    use super::pack;
-    use crate::Array;
-    use crate::ArrayRef;
-    use crate::IntoArray;
-    use crate::ToCanonical;
     use crate::arrays::PrimitiveArray;
     use crate::arrays::StructArray;
-    use crate::expr::VTableExt;
     use crate::expr::exprs::get_item::col;
+    use crate::expr::VTableExt;
     use crate::validity::Validity;
     use crate::vtable::ValidityHelper;
+    use crate::Array;
+    use crate::ArrayRef;
+    use crate::IntoArray;
+    use crate::ToCanonical;
 
     fn test_array() -> ArrayRef {
         StructArray::from_fields(&[

vortex-compute/src/arrow/primitive.rs

@@ -19,7 +19,6 @@ use arrow_array::ArrayRef;
 use arrow_array::PrimitiveArray;
 use vortex_buffer::Buffer;
 use vortex_dtype::half::f16;
-use vortex_error::vortex_panic;
 use vortex_error::VortexResult;
 use vortex_vector::match_each_pvector;
 use vortex_vector::primitive::PVector;

vortex-vector/src/datum.rs

@@ -1,14 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 
-use vortex_dtype::NativeDecimalType;
-use vortex_dtype::NativePType;
-use vortex_dtype::PType;
-
-use crate::Scalar;
-use crate::ScalarOps;
-use crate::Vector;
-use crate::VectorMutOps;
 use crate::binaryview::BinaryType;
 use crate::binaryview::BinaryViewScalar;
 use crate::binaryview::BinaryViewType;
@@ -32,6 +24,15 @@ use crate::primitive::PrimitiveScalar;
 use crate::primitive::PrimitiveVector;
 use crate::struct_::StructScalar;
 use crate::struct_::StructVector;
+use crate::Scalar;
+use crate::ScalarOps;
+use crate::Vector;
+use crate::VectorMutOps;
+use crate::VectorOps;
+use vortex_dtype::NativeDecimalType;
+use vortex_dtype::NativePType;
+use vortex_dtype::PType;
+use vortex_error::vortex_panic;
 
 /// Represents either a scalar or vector value.
 #[derive(Clone, Debug)]
@@ -59,7 +60,16 @@ impl Datum {
     pub fn ensure_vector(self, len: usize) -> Vector {
         match self {
             Datum::Scalar(scalar) => scalar.repeat(len).freeze(),
-            Datum::Vector(vector) => vector,
+            Datum::Vector(vector) => {
+                if vector.len() != len {
+                    vortex_panic!(
+                        "Vector length mismatch: expected length {}, got {}",
+                        len,
+                        vector.len()
+                    );
+                }
+                vector
+            }
         }
     }