fix: compare for struct with constant items

subtle bug introduced in #5412, where the nested elements are constant,
would change the len of `lhs` and cause the assertion to fail. Fix is
to assert the length earlier.

Signed-off-by: Andrew Duffy <[email protected]>

Author: a10y

vortex-array/src/compute/compare.rs

@@ -294,6 +294,9 @@ fn arrow_compare(
     right: &dyn Array,
     operator: Operator,
 ) -> VortexResult<ArrayRef> {
+    assert_eq!(left.len(), right.len());
+    let len = left.len();
+
     let nullable = left.dtype().is_nullable() || right.dtype().is_nullable();
 
     let array = if left.dtype().is_nested() || right.dtype().is_nested() {
@@ -312,8 +315,6 @@ fn arrow_compare(
         );
 
         let cmp = make_comparator(lhs, rhs, SortOptions::default())?;
-        assert_eq!(lhs.len(), rhs.len());
-        let len = lhs.len();
         let values = (0..len)
             .map(|i| {
                 let cmp = cmp(i, i);
@@ -365,6 +366,7 @@ pub fn scalar_cmp(lhs: &Scalar, rhs: &Scalar, operator: Operator) -> Scalar {
 #[cfg(test)]
 mod tests {
     use rstest::rstest;
+    use vortex_dtype::{FieldName, FieldNames};
 
     use super::*;
     use crate::ToCanonical;
@@ -572,4 +574,30 @@ mod tests {
         assert!(!bool_result.bit_buffer().value(1)); // {false, 2} > {false, 2} = false
         assert!(bool_result.bit_buffer().value(2)); // {true, 3} > {false, 4} = true (bool field takes precedence)
     }
+
+    #[test]
+    fn test_empty_struct_compare() {
+        let empty1 = StructArray::try_new(
+            FieldNames::from(Vec::<FieldName>::new()),
+            Vec::new(),
+            5,
+            Validity::NonNullable,
+        )
+        .unwrap();
+
+        let empty2 = StructArray::try_new(
+            FieldNames::from(Vec::<FieldName>::new()),
+            Vec::new(),
+            5,
+            Validity::NonNullable,
+        )
+        .unwrap();
+
+        let result = compare(empty1.as_ref(), empty2.as_ref(), Operator::Eq).unwrap();
+        let result = result.to_bool();
+
+        for idx in 0..5 {
+            assert!(result.bit_buffer().value(idx));
+        }
+    }
 }

vortex-layout/src/layouts/struct_/reader.rs

@@ -261,6 +261,7 @@ impl LayoutReader for StructReader {
         }
     }
 
+    #[allow(clippy::unwrap_in_result)]
     fn projection_evaluation(
         &self,
         row_range: &Range<u64>,
@@ -331,7 +332,7 @@ mod tests {
     use itertools::Itertools;
     use rstest::{fixture, rstest};
     use vortex_array::arrays::{BoolArray, StructArray};
-    use vortex_array::expr::{col, eq, get_item, gt, lit, or, pack, root, select};
+    use vortex_array::expr::{Expression, col, eq, get_item, gt, lit, or, pack, root, select};
     use vortex_array::validity::Validity;
     use vortex_array::{Array, ArrayContext, IntoArray, MaskFuture, ToCanonical};
     use vortex_buffer::buffer;
@@ -346,6 +347,36 @@ mod tests {
     use crate::sequence::{SequenceId, SequentialArrayStreamExt};
     use crate::{LayoutRef, LayoutStrategy};
 
+    #[fixture]
+    fn empty_struct() -> (Arc<dyn SegmentSource>, LayoutRef) {
+        let ctx = ArrayContext::empty();
+        let segments = Arc::new(TestSegments::default());
+        let (ptr, eof) = SequenceId::root().split();
+        let strategy =
+            StructStrategy::new(FlatLayoutStrategy::default(), FlatLayoutStrategy::default());
+        let layout = block_on(|handle| {
+            strategy.write_stream(
+                ctx,
+                segments.clone(),
+                StructArray::try_new(
+                    Vec::<FieldName>::new().into(),
+                    vec![],
+                    5,
+                    Validity::NonNullable,
+                )
+                .unwrap()
+                .into_array()
+                .to_array_stream()
+                .sequenced(ptr),
+                eof,
+                handle,
+            )
+        })
+        .unwrap();
+
+        (segments, layout)
+    }
+
     #[fixture]
     /// Create a chunked layout with three chunks of primitive arrays.
     fn struct_layout() -> (Arc<dyn SegmentSource>, LayoutRef) {
@@ -633,4 +664,21 @@ mod tests {
             Scalar::primitive(6, Nullability::Nullable)
         );
     }
+
+    #[rstest]
+    fn test_empty_struct(
+        #[from(empty_struct)] (segments, layout): (Arc<dyn SegmentSource>, LayoutRef),
+    ) {
+        let reader = layout.new_reader("".into(), segments).unwrap();
+        let expr = pack(Vec::<(String, Expression)>::new(), Nullability::Nullable);
+
+        let project = reader
+            .projection_evaluation(&(0..5), &expr, MaskFuture::new_true(5))
+            .unwrap();
+
+        let result = block_on(move |_| project).unwrap();
+        assert!(result.dtype().is_struct());
+
+        assert_eq!(result.len(), 5);
+    }
 }

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

@@ -64,9 +64,11 @@ impl LayoutStrategy for StructStrategy {
             vortex_bail!("StructLayout must have unique field names");
         }
 
+        let is_nullable = dtype.is_nullable();
+
         // Optimization: when there are no fields, don't spawn any work and just write a trivial
         // StructLayout.
-        if struct_dtype.nfields() == 0 {
+        if struct_dtype.nfields() == 0 && !is_nullable {
             let row_count = stream
                 .try_fold(
                     0u64,
@@ -77,8 +79,6 @@ impl LayoutStrategy for StructStrategy {
         }
 
         // stream<struct_chunk> -> stream<vec<column_chunk>>
-        let is_nullable = dtype.is_nullable();
-
         let columns_vec_stream = stream.map(move |chunk| {
             let (sequence_id, chunk) = chunk?;
             let mut sequence_pointer = sequence_id.descend();