Struct reader partitioning respects struct validity (#10449)

Current implementation returns lit(true) for pack()'s validity. This in
turn makes many things incorrect. For example, "a.x > 5" where a 
is nullable may read x's value under NULL rows.

Fix this by adding a discriminant virtual field which is struct's
validity.
This is internal and not addressable by user. This is a temporary
solution, since you need to construct FieldName (~ a string) from a
validity discriminant option as well.

This validity field is replaced to is_not_null($) while evaluating the
expression, and struct's bound expression is now mask(pack(...),
is_not_null($)) in case of a nullable struct.

Author: myrrc

vortex-array/src/dtype/arbitrary/mod.rs

@@ -16,6 +16,7 @@ use crate::dtype::FieldNames;
 use crate::dtype::NativeDecimalType;
 use crate::dtype::Nullability;
 use crate::dtype::PType;
+use crate::dtype::RESERVED_VALIDITY_FIELD_NAME;
 use crate::dtype::StructFields;
 use crate::dtype::i256;
 
@@ -127,8 +128,13 @@ fn random_struct_dtype(u: &mut Unstructured<'_>, depth: u8) -> Result<StructFiel
     let names: Vec<FieldName> = (0..field_count)
         .map(|_| FieldName::arbitrary(u))
         .collect::<Result<Vec<_>>>()?;
-    // Deduplicate field names, keeping only the first occurrence of each.
-    let names: FieldNames = names.into_iter().unique().collect();
+    // Deduplicate field names, keeping only the first occurrence of each, and drop the name
+    // reserved for struct validity.
+    let names: FieldNames = names
+        .into_iter()
+        .filter(|name| name.as_ref() != RESERVED_VALIDITY_FIELD_NAME)
+        .unique()
+        .collect();
     let dtypes = (0..names.len())
         .map(|_| random_dtype(u, depth))
         .collect::<Result<Vec<_>>>()?;

vortex-array/src/dtype/field_names.rs

@@ -9,6 +9,10 @@ use std::sync::Arc;
 use itertools::Itertools;
 use vortex_utils::aliases::StringEscape;
 
+/// Field name reserved for a nullable struct's own validity when expressions over the struct
+/// are partitioned by field. Structs must not contain a field with this name.
+pub const RESERVED_VALIDITY_FIELD_NAME: &str = "$__vortex_validity";
+
 /// A name for a field in a struct.
 #[derive(Clone, Debug, Eq, PartialOrd, Ord, Hash)]
 #[allow(clippy::derived_hash_with_manual_eq)] // manual PartialEq adds Arc::ptr_eq fast path only

vortex-array/src/expr/transform/bound_partition.rs

@@ -32,15 +32,6 @@ use crate::expr::traversal::TraversalOrder;
 /// Partition an expression into sub-expressions that are uniquely associated with an annotation.
 /// A root expression is also returned that can be used to recombine the results of the partitions
 /// into the result of the original expression.
-///
-/// ## Note
-///
-/// This function currently respects the validity of each field in the scope, but the not validity
-/// of the scope itself. The fix would be for the returned `BoundPartitionedExpr` to include a
-/// partition expression for computing the validity, or to include that expression as part of the
-/// root.
-///
-/// See <https://github.com/vortex-data/vortex/issues/1907>.
 pub fn partition_bound<A: AnnotationFn<BoundExpression>>(
     expr: BoundExpression,
     annotate_fn: A,