raghuvanshraj commented on code in PR #11537:
URL: https://github.com/apache/arrow-rs/pull/11537#discussion_r4466484960
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -529,6 +529,87 @@ impl VariantArray {
}
}
+ /// Returns a row as a [`Variant`], reconstructing shredded objects and
lists
+ /// into a caller-owned buffer when necessary.
+ ///
+ /// Unlike [`Self::try_value`], this can read composite `typed_value`
columns.
+ /// Only the requested row is unshredded. The returned value borrows from
this
+ /// array and, for reconstructed values, from `buffer`. The buffer is
replaced
+ /// for composite values and is unchanged for values that can be borrowed
+ /// directly. It can be reused after the returned value is dropped.
+ ///
+ /// For reading many rows, [`crate::unshred_variant()`] avoids repeatedly
+ /// initializing the recursive row builders.
+ ///
+ /// # Example
+ /// ```
+ /// use std::sync::Arc;
+ /// use arrow::array::{ArrayRef, StringArray};
+ /// use arrow_schema::{DataType, Field};
+ /// use parquet_variant::Variant;
+ /// use parquet_variant_compute::{json_to_variant, shred_variant};
+ ///
+ /// let json: ArrayRef = Arc::new(StringArray::from(vec![r#"{"a": 1}"#]));
+ /// let input = json_to_variant(&json)?;
+ /// let schema = DataType::Struct(vec![Field::new("a", DataType::Int64,
true)].into());
+ /// let shredded = shred_variant(&input, &schema)?;
+ /// let mut buffer = Vec::new();
+ /// let value = shredded.try_value_with_buffer(0, &mut buffer)?;
+ /// assert_eq!(value.as_object().unwrap().get("a"),
Some(Variant::Int64(1)));
+ /// # Ok::<(), arrow_schema::ArrowError>(())
+ /// ```
+ ///
+ /// # Errors
+ /// Returns an error for an out-of-bounds index, an array-level null row,
+ /// invalid shredded data, or a conversion error from [`Self::try_value`].
+ ///
+ /// # Panics
+ /// Has the same validation requirements as [`Self::try_value`] and
+ /// [`crate::unshred_variant()`]. Malformed inputs may panic in their
readers
+ /// or builders.
+ pub fn try_value_with_buffer<'a>(
+ &'a self,
+ index: usize,
+ buffer: &'a mut Vec<u8>,
+ ) -> Result<Variant<'a, 'a>> {
+ if index >= self.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Index {index} out of bounds for VariantArray of length {}",
+ self.len()
+ )));
+ }
+ if self.is_null(index) {
+ return Err(ArrowError::InvalidArgumentError(
+ "Cannot read a null Variant row".into(),
+ ));
+ }
+ let needs_buffer = self.typed_value_column().is_some_and(|typed_value|
{
+ typed_value.is_valid(index)
+ && matches!(
+ typed_value.data_type(),
+ DataType::Struct(_)
+ | DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::ListView(_)
+ | DataType::LargeListView(_)
+ )
+ });
+ if !needs_buffer {
+ return self.try_value(index);
+ }
+
+ let row = crate::unshred_variant(&self.slice(index, 1))?;
Review Comment:
Each call runs `UnshredVariantRowBuilder::try_new_opt` over the whole
`typed_value` schema, then builds one row. Fine for a scalar accessor. For the
batch kernels (`variant_to_json`, `variant_get`), looping this would rebuild
the row builders per row; those should `unshred_variant` once per array, as the
doc comment already suggests. Could the doc state that split explicitly: scalar
path = this accessor, batch path = unshred once?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -529,6 +529,87 @@ impl VariantArray {
}
}
+ /// Returns a row as a [`Variant`], reconstructing shredded objects and
lists
+ /// into a caller-owned buffer when necessary.
+ ///
+ /// Unlike [`Self::try_value`], this can read composite `typed_value`
columns.
+ /// Only the requested row is unshredded. The returned value borrows from
this
+ /// array and, for reconstructed values, from `buffer`. The buffer is
replaced
+ /// for composite values and is unchanged for values that can be borrowed
+ /// directly. It can be reused after the returned value is dropped.
+ ///
+ /// For reading many rows, [`crate::unshred_variant()`] avoids repeatedly
+ /// initializing the recursive row builders.
+ ///
+ /// # Example
+ /// ```
+ /// use std::sync::Arc;
+ /// use arrow::array::{ArrayRef, StringArray};
+ /// use arrow_schema::{DataType, Field};
+ /// use parquet_variant::Variant;
+ /// use parquet_variant_compute::{json_to_variant, shred_variant};
+ ///
+ /// let json: ArrayRef = Arc::new(StringArray::from(vec![r#"{"a": 1}"#]));
+ /// let input = json_to_variant(&json)?;
+ /// let schema = DataType::Struct(vec![Field::new("a", DataType::Int64,
true)].into());
+ /// let shredded = shred_variant(&input, &schema)?;
+ /// let mut buffer = Vec::new();
+ /// let value = shredded.try_value_with_buffer(0, &mut buffer)?;
+ /// assert_eq!(value.as_object().unwrap().get("a"),
Some(Variant::Int64(1)));
+ /// # Ok::<(), arrow_schema::ArrowError>(())
+ /// ```
+ ///
+ /// # Errors
+ /// Returns an error for an out-of-bounds index, an array-level null row,
+ /// invalid shredded data, or a conversion error from [`Self::try_value`].
+ ///
+ /// # Panics
+ /// Has the same validation requirements as [`Self::try_value`] and
+ /// [`crate::unshred_variant()`]. Malformed inputs may panic in their
readers
+ /// or builders.
+ pub fn try_value_with_buffer<'a>(
+ &'a self,
+ index: usize,
+ buffer: &'a mut Vec<u8>,
+ ) -> Result<Variant<'a, 'a>> {
+ if index >= self.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Index {index} out of bounds for VariantArray of length {}",
+ self.len()
+ )));
+ }
+ if self.is_null(index) {
+ return Err(ArrowError::InvalidArgumentError(
+ "Cannot read a null Variant row".into(),
+ ));
+ }
+ let needs_buffer = self.typed_value_column().is_some_and(|typed_value|
{
+ typed_value.is_valid(index)
+ && matches!(
+ typed_value.data_type(),
+ DataType::Struct(_)
+ | DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::ListView(_)
+ | DataType::LargeListView(_)
+ )
+ });
+ if !needs_buffer {
+ return self.try_value(index);
+ }
+
+ let row = crate::unshred_variant(&self.slice(index, 1))?;
+ let metadata = binary_array_value(self.metadata.as_ref(),
index).ok_or_else(|| {
Review Comment:
Pairing `self.metadata[index]` with `row.value[0]` is only valid because
`unshred_variant` returns `metadata.clone()` unchanged and resolves field ids
against that same dictionary. Either a one-line comment naming that invariant,
or read the metadata from `row.metadata_column()` at index 0 so the pairing
cannot drift if `unshred_variant` ever rebuilds metadata.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -529,6 +529,87 @@ impl VariantArray {
}
}
+ /// Returns a row as a [`Variant`], reconstructing shredded objects and
lists
+ /// into a caller-owned buffer when necessary.
+ ///
+ /// Unlike [`Self::try_value`], this can read composite `typed_value`
columns.
+ /// Only the requested row is unshredded. The returned value borrows from
this
+ /// array and, for reconstructed values, from `buffer`. The buffer is
replaced
+ /// for composite values and is unchanged for values that can be borrowed
+ /// directly. It can be reused after the returned value is dropped.
+ ///
+ /// For reading many rows, [`crate::unshred_variant()`] avoids repeatedly
+ /// initializing the recursive row builders.
+ ///
+ /// # Example
+ /// ```
+ /// use std::sync::Arc;
+ /// use arrow::array::{ArrayRef, StringArray};
+ /// use arrow_schema::{DataType, Field};
+ /// use parquet_variant::Variant;
+ /// use parquet_variant_compute::{json_to_variant, shred_variant};
+ ///
+ /// let json: ArrayRef = Arc::new(StringArray::from(vec![r#"{"a": 1}"#]));
+ /// let input = json_to_variant(&json)?;
+ /// let schema = DataType::Struct(vec![Field::new("a", DataType::Int64,
true)].into());
+ /// let shredded = shred_variant(&input, &schema)?;
+ /// let mut buffer = Vec::new();
+ /// let value = shredded.try_value_with_buffer(0, &mut buffer)?;
+ /// assert_eq!(value.as_object().unwrap().get("a"),
Some(Variant::Int64(1)));
+ /// # Ok::<(), arrow_schema::ArrowError>(())
+ /// ```
+ ///
+ /// # Errors
+ /// Returns an error for an out-of-bounds index, an array-level null row,
Review Comment:
Nit: `try_value` says it "does not check for nulls"; this one errors on a
null row. Good behaviour, but one line stating the difference would save
callers porting from `try_value` a surprise.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -529,6 +529,87 @@ impl VariantArray {
}
}
+ /// Returns a row as a [`Variant`], reconstructing shredded objects and
lists
+ /// into a caller-owned buffer when necessary.
+ ///
+ /// Unlike [`Self::try_value`], this can read composite `typed_value`
columns.
+ /// Only the requested row is unshredded. The returned value borrows from
this
+ /// array and, for reconstructed values, from `buffer`. The buffer is
replaced
+ /// for composite values and is unchanged for values that can be borrowed
+ /// directly. It can be reused after the returned value is dropped.
+ ///
+ /// For reading many rows, [`crate::unshred_variant()`] avoids repeatedly
+ /// initializing the recursive row builders.
+ ///
+ /// # Example
+ /// ```
+ /// use std::sync::Arc;
+ /// use arrow::array::{ArrayRef, StringArray};
+ /// use arrow_schema::{DataType, Field};
+ /// use parquet_variant::Variant;
+ /// use parquet_variant_compute::{json_to_variant, shred_variant};
+ ///
+ /// let json: ArrayRef = Arc::new(StringArray::from(vec![r#"{"a": 1}"#]));
+ /// let input = json_to_variant(&json)?;
+ /// let schema = DataType::Struct(vec![Field::new("a", DataType::Int64,
true)].into());
+ /// let shredded = shred_variant(&input, &schema)?;
+ /// let mut buffer = Vec::new();
+ /// let value = shredded.try_value_with_buffer(0, &mut buffer)?;
+ /// assert_eq!(value.as_object().unwrap().get("a"),
Some(Variant::Int64(1)));
+ /// # Ok::<(), arrow_schema::ArrowError>(())
+ /// ```
+ ///
+ /// # Errors
+ /// Returns an error for an out-of-bounds index, an array-level null row,
+ /// invalid shredded data, or a conversion error from [`Self::try_value`].
+ ///
+ /// # Panics
+ /// Has the same validation requirements as [`Self::try_value`] and
+ /// [`crate::unshred_variant()`]. Malformed inputs may panic in their
readers
+ /// or builders.
+ pub fn try_value_with_buffer<'a>(
+ &'a self,
+ index: usize,
+ buffer: &'a mut Vec<u8>,
+ ) -> Result<Variant<'a, 'a>> {
+ if index >= self.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Index {index} out of bounds for VariantArray of length {}",
+ self.len()
+ )));
+ }
+ if self.is_null(index) {
+ return Err(ArrowError::InvalidArgumentError(
+ "Cannot read a null Variant row".into(),
+ ));
+ }
+ let needs_buffer = self.typed_value_column().is_some_and(|typed_value|
{
+ typed_value.is_valid(index)
+ && matches!(
+ typed_value.data_type(),
+ DataType::Struct(_)
+ | DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::ListView(_)
+ | DataType::LargeListView(_)
+ )
+ });
+ if !needs_buffer {
+ return self.try_value(index);
+ }
+
+ let row = crate::unshred_variant(&self.slice(index, 1))?;
+ let metadata = binary_array_value(self.metadata.as_ref(),
index).ok_or_else(|| {
+ ArrowError::InvalidArgumentError("metadata field must be a
binary-like array".into())
+ })?;
+ let value = binary_array_value(row.value_column().as_ref(),
0).ok_or_else(|| {
+ ArrowError::InvalidArgumentError("value field must be a
binary-like array".into())
+ })?;
+ buffer.clear();
+ buffer.extend_from_slice(value);
+ Variant::try_new(metadata, buffer)
Review Comment:
`try_value` deliberately skips deep validation ("Does not do deep validation
of the Variant"). The bytes here were just produced by the unshred builder.
`Variant::new` would match `try_value` and skip a validation walk per row.
Maintainer call on whether the extra check is worth keeping.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -529,6 +529,87 @@ impl VariantArray {
}
}
+ /// Returns a row as a [`Variant`], reconstructing shredded objects and
lists
+ /// into a caller-owned buffer when necessary.
+ ///
+ /// Unlike [`Self::try_value`], this can read composite `typed_value`
columns.
+ /// Only the requested row is unshredded. The returned value borrows from
this
+ /// array and, for reconstructed values, from `buffer`. The buffer is
replaced
+ /// for composite values and is unchanged for values that can be borrowed
+ /// directly. It can be reused after the returned value is dropped.
+ ///
+ /// For reading many rows, [`crate::unshred_variant()`] avoids repeatedly
+ /// initializing the recursive row builders.
+ ///
+ /// # Example
+ /// ```
+ /// use std::sync::Arc;
+ /// use arrow::array::{ArrayRef, StringArray};
+ /// use arrow_schema::{DataType, Field};
+ /// use parquet_variant::Variant;
+ /// use parquet_variant_compute::{json_to_variant, shred_variant};
+ ///
+ /// let json: ArrayRef = Arc::new(StringArray::from(vec![r#"{"a": 1}"#]));
+ /// let input = json_to_variant(&json)?;
+ /// let schema = DataType::Struct(vec![Field::new("a", DataType::Int64,
true)].into());
+ /// let shredded = shred_variant(&input, &schema)?;
+ /// let mut buffer = Vec::new();
+ /// let value = shredded.try_value_with_buffer(0, &mut buffer)?;
+ /// assert_eq!(value.as_object().unwrap().get("a"),
Some(Variant::Int64(1)));
+ /// # Ok::<(), arrow_schema::ArrowError>(())
+ /// ```
+ ///
+ /// # Errors
+ /// Returns an error for an out-of-bounds index, an array-level null row,
+ /// invalid shredded data, or a conversion error from [`Self::try_value`].
+ ///
+ /// # Panics
+ /// Has the same validation requirements as [`Self::try_value`] and
+ /// [`crate::unshred_variant()`]. Malformed inputs may panic in their
readers
+ /// or builders.
+ pub fn try_value_with_buffer<'a>(
+ &'a self,
+ index: usize,
+ buffer: &'a mut Vec<u8>,
+ ) -> Result<Variant<'a, 'a>> {
+ if index >= self.len() {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Index {index} out of bounds for VariantArray of length {}",
+ self.len()
+ )));
+ }
+ if self.is_null(index) {
+ return Err(ArrowError::InvalidArgumentError(
+ "Cannot read a null Variant row".into(),
+ ));
+ }
+ let needs_buffer = self.typed_value_column().is_some_and(|typed_value|
{
Review Comment:
This is a second copy of "which `typed_value` types are composite"; the
first is the unsupported arm in `typed_value_to_variant`
(`type_conversion.rs`). Two options: call `try_value` first and only fall to
the buffer path on its `NotYetImplemented`, or factor one
`is_composite(&DataType)` helper that both sites use. Otherwise the two lists
can drift.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
