some comments

Signed-off-by: Adam Gutglick <adam@spiraldb.com>

Author: AdamGS

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

@@ -41,6 +41,8 @@ pub trait VariantArrayExt: TypedArrayRef<Variant> {
     }
 
     /// Returns the optional row-aligned typed shredded tree for selected variant paths.
+    /// This functions returns `Some` only if the array was canonicalized and the shredded data
+    /// was pulled out of the underlying variant storage.
     fn shredded(&self) -> Option<&ArrayRef> {
         self.as_ref().slots()[SHREDDED_SLOT].as_ref()
     }

vortex-array/src/arrays/variant/vtable/kernel.rs

@@ -134,6 +134,9 @@ impl ExecuteParentKernel<Variant> for VariantGetKernel {
 
         // Null typed rows are not necessarily missing from the logical variant value;
         // fill those rows from core storage and keep valid typed rows unchanged.
+        // TODO: we are computing the entire fallback array here but only take indices where
+        // typed_mask is false, so we can narrow the core_storage to only the false
+        // indices and compute from there then zip
         let fallback = make_fallback(ctx)?;
         typed_mask
             .into_array()

vortex-array/src/arrays/variant/vtable/operations.rs

@@ -18,7 +18,7 @@ impl OperationsVTable<Variant> for Variant {
         ctx: &mut ExecutionCtx,
     ) -> VortexResult<Scalar> {
         let core_storage = array.core_storage();
-        if !core_storage.is_valid(index, ctx)? {
+        if core_storage.is_invalid(index, ctx)? {
             return Ok(Scalar::null(array.dtype().clone()));
         }
 
@@ -27,6 +27,7 @@ impl OperationsVTable<Variant> for Variant {
         };
 
         let typed = shredded.execute_scalar(index, ctx)?;
+        // If the shredded value is null OR we shredded an object we want to merge back together.
         let fallback = (typed.is_null() || typed.dtype().is_struct())
             .then(|| core_storage.execute_scalar(index, ctx))
             .transpose()?;

vortex-array/src/canonical.rs

@@ -685,6 +685,8 @@ impl Executable for CanonicalValidity {
 /// callers should prefer an execution target that's suitable for their use case instead of this one.
 pub struct RecursiveCanonical(pub Canonical);
 
+// TODO: Currently only used for Variant, in the future
+// can probably be used for more canonical types like Struct.
 fn recursively_canonicalize_slots(
     array: &ArrayRef,
     ctx: &mut ExecutionCtx,

vortex-array/src/scalar_fn/fns/variant_get/mod.rs

@@ -119,6 +119,7 @@ impl ScalarFnVTable for VariantGet {
             "VariantGet input must be Variant, found {input_dtype}"
         );
 
+        // Missing paths, traversal mismatches, and cast failures all produce nulls.
         Ok(options
             .dtype()
             .map_or(DType::Variant(Nullability::Nullable), DType::as_nullable))
@@ -131,6 +132,7 @@ impl ScalarFnVTable for VariantGet {
         ctx: &mut ExecutionCtx,
     ) -> VortexResult<ArrayRef> {
         let input = args.get(0)?;
+        // Missing paths, traversal mismatches, and cast failures all produce nulls.
         let dtype = options
             .dtype()
             .map_or(DType::Variant(Nullability::Nullable), DType::as_nullable);