simplify comments

Signed-off-by: Baris Palaska <barispalaska@gmail.com>

Author: palaska

vortex-array/src/arrow/convert.rs

@@ -743,8 +743,8 @@ impl FromArrowArray<&RecordBatch> for ArrayRef {
     }
 }
 
-/// Rewrap `storage` as `ExtensionArray` if `field` carries `ARROW:extension:name`
-/// for a registered extension. Inverse of `field_from_dtype` in `dtype/arrow.rs`.
+/// Rewrap `storage` as an `ExtensionArray` when `field` carries extension metadata
+/// for an extension registered on the session.
 fn wrap_extension_if_field_has_metadata(
     storage: ArrayRef,
     field: &Field,
@@ -1968,8 +1968,8 @@ mod tests {
         ArrayRef::from_arrow(null_struct_array_with_non_nullable_field.as_ref(), true).unwrap();
     }
 
-    /// Dictionary value with a struct field carrying registered extension metadata recovers
-    /// the extension dtype when converted via the session-aware path.
+    /// Extension metadata on a struct field nested inside a Dictionary's values is
+    /// recovered when the session has the extension registered.
     #[test]
     fn dictionary_struct_value_recovers_extension_through_session() {
         let session = VortexSession::empty().with::<DTypeSession>();

vortex-array/src/arrow/executor/mod.rs

@@ -99,9 +99,8 @@ impl ArrowArrayExecutor for ArrayRef {
             None => preferred_arrow_type(&self)?,
         };
 
-        // Temporal extensions keep their wrapper so `to_arrow_temporal` can read the
-        // metadata. Other extensions unwrap to storage; their identity rides on Field
-        // metadata.
+        // Temporal extensions stay wrapped — `to_arrow_temporal` reads their metadata.
+        // Other extensions unwrap to storage; their identity lives on the Field.
         if let DType::Extension(ext) = self.dtype()
             && ext.metadata_opt::<AnyTemporal>().is_none()
         {
@@ -279,8 +278,8 @@ mod tests {
         assert_eq!(primitives.values(), &[0, 1, 2, 3, 4, 5]);
     }
 
-    /// Temporal extensions have native Arrow mappings. The executor must keep the extension
-    /// array intact so `to_arrow_temporal` can read `TemporalMetadata` from its dtype.
+    /// Temporal extensions map to native Arrow types — the executor must not unwrap them,
+    /// otherwise `to_arrow_temporal` can't read the time unit / timezone.
     #[test]
     fn execute_arrow_keeps_temporal_extension_for_native_arrow_type() {
         use crate::dtype::DType;

vortex-array/src/arrow/executor/struct_.rs

@@ -86,9 +86,8 @@ pub(super) fn to_arrow_struct(
 
     // Otherwise, we fall back to executing to a StructArray.
     let array = if let Some(fields) = target_fields {
-        // Thread the ctx session so any extension Field metadata in `fields` resolves to
-        // `DType::Extension` in the cast target — keeps the cast path consistent with the
-        // short-circuit Struct/`Pack` paths above.
+        // Use the ctx session so extension fields resolve to `DType::Extension` in the
+        // cast target, matching the short-circuit paths above.
         let vx_fields = StructFields::from_arrow_with_session(fields, ctx.session());
         // We apply a cast to ensure we push down casting where possible into the struct fields.
         array.cast(DType::Struct(

vortex-array/src/arrow/mod.rs

@@ -28,9 +28,8 @@ use crate::VortexSessionExecute;
 pub trait FromArrowArray<A>: Sized {
     fn from_arrow(array: A, nullable: bool) -> VortexResult<Self>;
 
-    /// Same conversion, with session for resolving `ARROW:extension:name` field metadata to
-    /// registered extension dtypes. The default ignores the session — override on impls that
-    /// see Arrow `Field`s (RecordBatch, Struct, List, FSL).
+    /// Like [`Self::from_arrow`], but resolves `ARROW:extension:name` field metadata
+    /// against extensions registered on `session`. The default ignores the session.
     fn from_arrow_with_session(
         array: A,
         nullable: bool,

vortex-array/src/arrow/record_batch.rs

@@ -22,9 +22,7 @@ impl StructArray {
         self.into_record_batch_with_schema_with_session(schema, &LEGACY_SESSION)
     }
 
-    /// Same as [`Self::into_record_batch_with_schema`], but routes execution through `session`
-    /// so canonical Arrow extension aliases declared on registered vtables apply uniformly to
-    /// both schema construction and array conversion.
+    /// Like [`Self::into_record_batch_with_schema`], but runs the conversion under `session`.
     pub fn into_record_batch_with_schema_with_session(
         self,
         schema: impl AsRef<Schema>,

vortex-array/src/dtype/arrow.rs

@@ -53,18 +53,17 @@ use crate::extension::datetime::Time;
 use crate::extension::datetime::TimeUnit;
 use crate::extension::datetime::Timestamp;
 
-/// Canonical Arrow extension name for Parquet Variant — handled as `DType::Variant` rather
-/// than going through the extension registry.
+/// Arrow's Parquet Variant extension name. We map it to `DType::Variant` directly,
+/// not through the extension registry.
 pub const ARROW_EXT_NAME_VARIANT: &str = "arrow.parquet.variant";
 
 /// Trait for converting Arrow types to Vortex types.
 pub trait FromArrowType<T>: Sized {
     /// Convert the Arrow type to a Vortex type.
     fn from_arrow(value: T) -> Self;
 
-    /// Convert the Arrow type to a Vortex type, consulting `session` for extension lookup.
-    ///
-    /// Unregistered or malformed extension metadata falls back to the storage dtype.
+    /// Convert an Arrow type to a Vortex type, looking up extensions in `session`.
+    /// Unregistered or malformed extensions fall back to the storage dtype.
     fn from_arrow_with_session(value: T, session: &VortexSession) -> Self {
         let _ = session;
         Self::from_arrow(value)
@@ -269,8 +268,9 @@ fn dtype_from_field(field: &Field, dtypes: &DTypeSession) -> DType {
     }
 }
 
-/// Resolve the [`ExtDTypeRef`] for a Field whose `ARROW:extension:name` names a registered
-/// Vortex extension. Returns `None` for missing/unregistered/malformed metadata.
+/// Look up the extension dtype for a Field's `ARROW:extension:name` against the session.
+/// Returns `None` (after logging a warning) if the extension is missing, unregistered,
+/// or has malformed metadata.
 pub(crate) fn resolve_extension_dtype(
     field: &Field,
     dtypes: &DTypeSession,
@@ -315,8 +315,7 @@ pub(crate) fn resolve_extension_dtype(
     }
 }
 
-/// Extensions base64-encode arbitrary binary metadata to survive Arrow's String-typed
-/// metadata channel.
+/// Arrow's metadata channel is a `String`, so we base64-encode the raw extension bytes.
 fn decode_extension_metadata(field: &Field) -> VortexResult<Vec<u8>> {
     match field.extension_type_metadata() {
         None | Some("") => Ok(Vec::new()),
@@ -326,8 +325,7 @@ fn decode_extension_metadata(field: &Field) -> VortexResult<Vec<u8>> {
     }
 }
 
-/// Build the storage [`DType`] for `field`, recursing through nested children so each level
-/// runs the extension lookup against `dtypes`.
+/// Build the storage [`DType`] for `field`, running the extension lookup at every nested level.
 fn storage_dtype_from_field(field: &Field, dtypes: &DTypeSession) -> DType {
     let nullability: Nullability = field.is_nullable().into();
     match field.data_type() {
@@ -374,10 +372,10 @@ impl DType {
         Ok(builder.finish())
     }
 
-    /// Returns the Arrow [`DataType`] that best corresponds to this Vortex [`DType`].
+    /// Returns the Arrow [`DataType`] for this Vortex [`DType`].
     ///
-    /// Extensions without a native Arrow mapping degrade to their storage `DataType`;
-    /// identity only survives via `Field` metadata (see [`Self::to_arrow_schema`]).
+    /// Extensions without a native Arrow mapping return only their storage `DataType`.
+    /// Use [`Self::to_arrow_schema`] to keep the extension identity on the Field.
     pub fn to_arrow_dtype(&self) -> VortexResult<DataType> {
         arrow_dtype_from_dtype(self)
     }
@@ -450,8 +448,7 @@ fn arrow_dtype_from_dtype(dtype: &DType) -> VortexResult<DataType> {
             if let Some(native) = native_arrow_dtype_for_extension(ext_dtype) {
                 return Ok(native);
             }
-            // Extension identity lives on the Field (see `field_from_dtype`), not on
-            // DataType, so here we only encode the storage type.
+            // Extension identity lives on the Field, not the DataType — emit only storage here.
             arrow_dtype_from_dtype(ext_dtype.storage_dtype())?
         }
     })
@@ -473,8 +470,8 @@ fn field_from_dtype(name: &str, dtype: &DType) -> VortexResult<Field> {
     }
 
     if let DType::Extension(ext) = dtype {
-        // Native Arrow mapping carries the semantics in DataType; emitting extension metadata
-        // on top would break consumers that only understand native Arrow types.
+        // Temporal extensions map to native Arrow types — don't add extension metadata,
+        // it would confuse Arrow-only consumers.
         if let Some(native) = native_arrow_dtype_for_extension(ext) {
             return Ok(Field::new(name, native, dtype.is_nullable()));
         }
@@ -501,8 +498,8 @@ fn field_from_dtype(name: &str, dtype: &DType) -> VortexResult<Field> {
     ))
 }
 
-/// Returns the native Arrow [`DataType`] for extensions Arrow models directly (e.g. temporal).
-/// `None` means the extension should round-trip via storage + Field metadata.
+/// The native Arrow [`DataType`] for extensions Arrow models directly (currently temporal),
+/// or `None` if the extension should round-trip via storage + Field metadata.
 fn native_arrow_dtype_for_extension(ext_dtype: &ExtDTypeRef) -> Option<DataType> {
     let temporal = ext_dtype.metadata_opt::<AnyTemporal>()?;
     Some(match temporal {

vortex-python/src/arrays/from_arrow.rs

@@ -37,8 +37,8 @@ use crate::error::PyVortexResult;
 
 /// Convert a Python `pyarrow` array (including `pa.ExtensionArray`) into a Vortex array.
 ///
-/// The Arrow C ABI strips extension identity from leaf arrays; we recover it from the
-/// Python object via `extension_name` and `__arrow_ext_serialize__`.
+/// The Arrow C ABI doesn't carry extension identity on leaf arrays, so we read it from
+/// the Python object via `extension_name` and `__arrow_ext_serialize__`.
 pub trait FromPyArrowArray: Sized {
     /// Convert a Python `pyarrow` array to a Vortex array.
     fn from_pyarrow(py_array: &Bound<'_, PyAny>, nullable: bool) -> PyResult<Self>;
@@ -77,9 +77,9 @@ impl FromPyArrowArray for ArrayRef {
     }
 }
 
-/// Raw bytes from `__arrow_ext_serialize__` — no base64 (that's only for the
-/// Arrow Field-metadata string channel). Variant short-circuits to `None` so it surfaces
-/// as `DType::Variant` via the storage path, mirroring `dtype/arrow.rs::dtype_from_field`.
+/// `__arrow_ext_serialize__` returns raw bytes — no base64 (that's only for the Field
+/// metadata channel). Variant returns `None` so it surfaces as `DType::Variant` instead
+/// of going through the extension registry.
 fn extract_extension_info(py_array: &Bound<'_, PyAny>) -> PyResult<Option<(String, Vec<u8>)>> {
     let py = py_array.py();
     let py_type = py_array.getattr(intern!(py, "type"))?;
@@ -96,8 +96,8 @@ fn extract_extension_info(py_array: &Bound<'_, PyAny>) -> PyResult<Option<(Strin
     Ok(Some((ext_name, ext_meta_bytes)))
 }
 
-/// Soft fallback to storage on registry miss or malformed metadata, mirroring
-/// `dtype/arrow.rs::resolve_extension_dtype`.
+/// Wrap `storage` as an extension array if the extension is registered. On a registry
+/// miss or malformed metadata, log a warning and return `storage` as-is.
 fn wrap_with_extension(
     storage: ArrayRef,
     ext_name: &str,
@@ -148,7 +148,7 @@ pub(super) fn from_arrow(obj: &Borrowed<'_, '_, PyAny>) -> PyVortexResult<PyArra
         Ok(PyArrayRef::from(enc_array))
     } else if obj.is_instance(chunked_array)? {
         let chunks: Vec<Bound<PyAny>> = obj.getattr(intern!(py, "chunks"))?.extract()?;
-        // ChunkedArray has a uniform type — peek extension identity once and reuse.
+        // All chunks share the same type, so read the extension info once.
         let bound = obj.to_owned();
         let ext_info = extract_extension_info(&bound)?;
         let encoded_chunks = chunks
@@ -172,8 +172,8 @@ pub(super) fn from_arrow(obj: &Borrowed<'_, '_, PyAny>) -> PyVortexResult<PyArra
         let dtype: DType = if let Some(first) = encoded_chunks.first() {
             first.dtype().clone()
         } else {
-            // Empty array: `obj.type` over the C ABI loses extension metadata, so we
-            // recover only the storage dtype.
+            // Empty: there's no chunk to read the dtype from, and `obj.type` over the C ABI
+            // doesn't carry extension metadata. Fall back to the storage dtype.
             obj.getattr(intern!(py, "type"))
                 .and_then(|v| DataType::from_pyarrow(&v.as_borrowed()))
                 .map(|dt| DType::from_arrow_with_session(&Field::new("_", dt, false), &SESSION))?
@@ -182,8 +182,7 @@ pub(super) fn from_arrow(obj: &Borrowed<'_, '_, PyAny>) -> PyVortexResult<PyArra
             ChunkedArray::try_new(encoded_chunks, dtype)?.into_array(),
         ))
     } else if obj.is_instance(table)? {
-        // The C ABI Stream carries Field metadata on the schema — session-aware
-        // conversion recovers extensions directly, no Python peek needed.
+        // The C ABI Stream carries Field metadata, so we don't need a Python peek here.
         let array_stream = ArrowArrayStreamReader::from_pyarrow(&obj.as_borrowed())?;
         let dtype = DType::from_arrow_with_session(array_stream.schema(), &SESSION);
         let chunks = array_stream

vortex-python/test/test_arrow_extension.py

@@ -1,8 +1,7 @@
 # SPDX-License-Identifier: Apache-2.0
 # SPDX-FileCopyrightText: Copyright the Vortex contributors
 
-"""Round-trip tests for `vx.array` against pyarrow inputs carrying Vortex
-extension identity over the Arrow C ABI."""
+"""Tests that `vx.array` recovers Vortex extension identity from pyarrow inputs."""
 
 from __future__ import annotations
 
@@ -12,16 +11,16 @@
 
 import vortex as vx
 
-# vortex.timestamp wire format: u8 unit_tag + u16 LE tz_len (us=1, no tz).
-# See vortex-array/src/extension/datetime/timestamp.rs::serialize_metadata.
+# Wire format: u8 unit_tag + u16 LE tz_len. Microseconds = 1, no timezone.
+# See vortex-array/src/extension/datetime/timestamp.rs.
 _TIMESTAMP_US_METADATA = bytes([1, 0, 0])
 
 
 class VortexTimestampType(pa.ExtensionType):
-    """A pyarrow `ExtensionType` matching Vortex's `vortex.timestamp` extension."""
+    """pyarrow `ExtensionType` matching Vortex's `vortex.timestamp`."""
 
     def __init__(self, unit: str = "us"):
-        # pyarrow calls `__arrow_ext_serialize__` in __init__, so set `_unit` first.
+        # pyarrow calls `__arrow_ext_serialize__` from __init__, so `_unit` must be set first.
         self._unit = unit
         pa.ExtensionType.__init__(self, pa.int64(), "vortex.timestamp")
 

vortex-tensor/src/tests/arrow_roundtrip.rs

@@ -54,7 +54,7 @@ fn vector_forward_carries_extension_name() {
             .map(String::as_str),
         Some(Vector.id().as_str()),
     );
-    // EmptyMetadata → no metadata key.
+    // Vector uses EmptyMetadata, so no metadata key is written.
     assert!(field.metadata().get(EXTENSION_TYPE_METADATA_KEY).is_none());
 
     let DataType::FixedSizeList(element, size) = field.data_type() else {