You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "codephage2020 (via GitHub)" <gi...@apache.org> on 2026/03/21 01:06:27 UTC
[PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
codephage2020 opened a new pull request, #9598:
URL: https://github.com/apache/arrow-rs/pull/9598
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
-->
- Closes #9519 .
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
variant_get cannot extract a struct where some fields remain as VariantArrays. Callers must call variant_get per leaf and assemble results manually.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Support extracting struct fields as VariantArray via VariantType extension metadata in variant_get, and fix panics on absent fields and silent data loss on non-struct typed values.
# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes — test_struct_extraction_with_variant_fields and test_struct_extraction_missing_variant_field_no_panic.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
No breaking changes. Fields marked with VariantType extension metadata are extracted as VariantArrays in struct extraction.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3063162132
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
+ // so we must update the field's data type and re-attach VariantType.
+ //
+ // When the field is entirely absent, shredded_get_path returns a
+ // NullArray. Construct an all-null VariantArray so the extension
+ // metadata is preserved.
+ let child = if child.data_type() == &DataType::Null {
Review Comment:
Maybe we can add a test which covers this code block?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
+ // so we must update the field's data type and re-attach VariantType.
+ //
+ // When the field is entirely absent, shredded_get_path returns a
+ // NullArray. Construct an all-null VariantArray so the extension
+ // metadata is preserved.
+ let child = if child.data_type() == &DataType::Null {
Review Comment:
`cargo llvm-cov --html test -p parquet-variant-compute` shows that this is not covered
<img width="843" height="160" alt="Image" src="https://github.com/user-attachments/assets/e5e5bf41-175a-4851-ab1f-50a15c740b96" />
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#issuecomment-4725088457
> Hi @codephage2020, are you looking to come back to this?
Thank you for the reminder. I've been very busy recently. I'll pick up this pr this week.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#issuecomment-4724690335
Hi @codephage2020, are you looking to come back to this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3025630995
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +224,61 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray. For variant fields, construct an all-null
+ // VariantArray so the extension metadata is preserved.
+ let child = if is_variant_field && child.data_type() == &DataType::Null {
+ let mut builder = VariantArrayBuilder::new(child.len());
+ for _ in 0..child.len() {
+ builder.append_null();
+ }
+ let null_variant = builder.build();
+ Arc::new(null_variant.into_inner()) as ArrayRef
+ } else {
+ child
+ };
+
+ // Update field data type to match the actual child array.
+ // Preserve VariantType extension metadata for variant fields so
+ // downstream consumers can recognize them as Variant columns.
+ let mut new_field = field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone());
Review Comment:
Good question. VariantType does NOT leak to intermediate fields. Here's why:
The `is_variant_field` check is driven by the **user's requested schema**, not by the
data's shredding structure. When a user marks field "metadata" with VariantType, we pass
`as_type = None` to `shredded_get_path`, which returns a VariantArray directly without
recursing into the struct branch. So the recursion stops at that level — inner fields
of the VariantArray's internal structure never go through the VariantType annotation path.
For case 1 (strongly typed struct), I've also addressed your observation that `with_data_type`
is redundant — strongly typed fields now use the original field directly, since the child
data type already matches the requested type.
To make this clearer, I split the handling into two explicit branches:
if is_variant_field {
// Update data type + attach VariantType extension
// (or build all-null VariantArray when field is absent)
} else {
// Strongly typed: use original field as-is
}
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3066264176
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
+ // so we must update the field's data type and re-attach VariantType.
+ //
+ // When the field is entirely absent, shredded_get_path returns a
+ // NullArray. Construct an all-null VariantArray so the extension
+ // metadata is preserved.
+ let child = if child.data_type() == &DataType::Null {
+ let mut builder = VariantArrayBuilder::new(child.len());
+ for _ in 0..child.len() {
+ builder.append_null();
+ }
Review Comment:
Pushed here #9685
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3048591246
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +88,14 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray, so it cannot be
+ // followed further. Fall back to the value column if present,
+ // otherwise the path is missing.
Review Comment:
I agree with @codephage2020. This is malformed data rather than a JSONPath-handling case.
<details>
<summary>Spark appears to treat this as <code>malformedVariant</code> as well</summary>
What Codex spewed out for me:
**Path-extraction route (closest to `variant_get` path-step behavior):**
1. `ParquetRowConverter.end` calls `assembleVariantStruct(...)`
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetRowConverter.scala#L1036-L1043
2. `assembleVariantStruct` calls `extractField(...)`
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/SparkShreddingUtils.scala#L827-L844
3. In `extractField`, a null object-field slot triggers malformed variant
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/SparkShreddingUtils.scala#L795-L799
4. That throws `QueryExecutionErrors.malformedVariant()`
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala#L2995-L2998
**Rebuild route (directly via `common/variant/.../ShreddingUtils.java`):**
1. `ParquetRowConverter.end` calls `assembleVariant(...)`
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetRowConverter.scala#L1039-L1040
2. `assembleVariant` calls `ShreddingUtils.rebuild(...)`
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/SparkShreddingUtils.scala#L821-L823
3. `ShreddingUtils.rebuild` throws `malformedVariant()` on invalid shredded layout
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/common/variant/src/main/java/org/apache/spark/types/variant/ShreddingUtils.java#L49-L63
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/common/variant/src/main/java/org/apache/spark/types/variant/ShreddingUtils.java#L131-L135
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/common/variant/src/main/java/org/apache/spark/types/variant/ShreddingUtils.java#L167-L170
4. `malformedVariant()` definition
https://github.com/apache/spark/blob/c8695495569e4058a758b111ebc942fc4906494c/common/variant/src/main/java/org/apache/spark/types/variant/VariantUtil.java#L177-L180
</details>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r2985563286
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
Review Comment:
good catch
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#issuecomment-5305260748
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3048457085
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -4376,4 +4434,167 @@ mod test {
);
}
}
+
+ /// Test extracting a struct with mixed typed and variant fields.
+ /// Fields with VariantType extension metadata should be extracted as VariantArrays.
+ #[test]
+ fn test_struct_extraction_with_variant_fields() {
+ // Create test data: [{"id": 1, "name": "Alice", "data": {"score": 95}},
+ // {"id": 2, "name": "Bob", "data": null}]
+ let json_strings = vec![
+ r#"{"id": 1, "name": "Alice", "data": {"score": 95}}"#,
+ r#"{"id": 2, "name": "Bob", "data": null}"#,
+ r#"{"id": 3, "name": null, "data": {"level": 5}}"#,
+ ];
+ let string_array: Arc<dyn Array> = Arc::new(StringArray::from(json_strings));
+ let variant_array = json_to_variant(&string_array).unwrap();
+
+ // Request struct where:
+ // - "id" is extracted as Int32
+ // - "name" is extracted as String (Utf8)
+ // - "data" is extracted as Variant (using VariantType extension metadata)
+ let struct_fields = Fields::from(vec![
+ Field::new("id", DataType::Int32, true),
+ Field::new("name", DataType::Utf8, true),
+ // Use VariantType extension metadata to request extraction as VariantArray.
+ // The data type must be Struct to satisfy VariantType::supports_data_type.
+ Field::new("data", DataType::Struct(Fields::empty()), true)
+ .with_extension_type(VariantType),
+ ]);
+ let struct_type = DataType::Struct(struct_fields);
+
+ let options = GetOptions {
+ path: VariantPath::default(),
+ as_type: Some(Arc::new(Field::new("result", struct_type, true))),
+ cast_options: CastOptions::default(),
+ };
+
+ let variant_array_ref = ArrayRef::from(variant_array);
+ let result = variant_get(&variant_array_ref, options).unwrap();
+
+ // Verify the result is a StructArray with 3 fields
+ let struct_result = result.as_any().downcast_ref::<StructArray>().unwrap();
+ assert_eq!(struct_result.len(), 3);
+ assert_eq!(struct_result.num_columns(), 3);
+
+ // Verify "id" field (Int32)
+ let id_field = struct_result
+ .column(0)
+ .as_any()
+ .downcast_ref::<Int32Array>()
+ .unwrap();
+ assert_eq!(id_field.value(0), 1);
+ assert_eq!(id_field.value(1), 2);
+ assert_eq!(id_field.value(2), 3);
+
+ // Verify "name" field (String/Utf8)
+ let name_field = struct_result
+ .column(1)
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .unwrap();
+ assert_eq!(name_field.value(0), "Alice");
+ assert_eq!(name_field.value(1), "Bob");
+ assert!(name_field.is_null(2)); // null name in row 2
+
+ // Verify "data" field schema has VariantType extension metadata
+ let data_schema_field = struct_result
+ .fields()
+ .iter()
+ .find(|f| f.name() == "data")
+ .unwrap();
+ assert!(
+ data_schema_field
+ .try_extension_type::<VariantType>()
+ .is_ok(),
+ "data field should have VariantType extension metadata"
+ );
+
+ // Verify "data" field (VariantArray)
+ let data_field = struct_result.column(2);
+ // The data field should be a StructArray representing VariantArray's internal structure
+ // It has columns: metadata, value (optional), typed_value (optional)
+ let data_as_struct = data_field.as_any().downcast_ref::<StructArray>();
+ assert!(
+ data_as_struct.is_some(),
+ "data field should be a VariantArray (represented as StructArray)"
+ );
+
+ // Verify we can access the variant values
+ let data_variant_array = VariantArray::try_new(data_field).unwrap();
+ assert_eq!(data_variant_array.len(), 3);
+
+ // Row 0: data = {"score": 95}
+ let data0 = data_variant_array.value(0);
+ let obj0 = data0.as_object().expect("row 0 data should be an object");
+ let score = obj0.get("score").expect("row 0 data should have 'score'");
+ assert_eq!(score.as_int16(), Some(95));
+
+ // Row 1: data = null
+ assert!(
+ data_variant_array.is_null(1) || matches!(data_variant_array.value(1), Variant::Null)
+ );
+
+ // Row 2: data = {"level": 5}
+ let data2 = data_variant_array.value(2);
+ let obj2 = data2.as_object().expect("row 2 data should be an object");
+ let level = obj2.get("level").expect("row 2 data should have 'level'");
+ assert_eq!(level.as_int8(), Some(5));
+ }
+
+ /// Test that requesting a variant field absent in all rows does not panic.
+ /// Regression test: with_extension_type(VariantType) used to panic on NullArray.
+ #[test]
+ fn test_struct_extraction_missing_variant_field_no_panic() {
+ // Data has "id" but NOT "missing_field"
+ let json_strings = vec![r#"{"id": 1}"#, r#"{"id": 2}"#];
+ let string_array: Arc<dyn Array> = Arc::new(StringArray::from(json_strings));
+ let variant_array = json_to_variant(&string_array).unwrap();
+
+ // Request struct with a variant field that doesn't exist in any row
+ let struct_fields = Fields::from(vec![
+ Field::new("id", DataType::Int32, true),
+ Field::new("missing_field", DataType::Struct(Fields::empty()), true)
+ .with_extension_type(VariantType),
+ ]);
+ let struct_type = DataType::Struct(struct_fields);
+
+ let options = GetOptions {
+ path: VariantPath::default(),
+ as_type: Some(Arc::new(Field::new("result", struct_type, true))),
+ cast_options: CastOptions::default(),
+ };
+
+ let variant_array_ref = ArrayRef::from(variant_array);
+ // This should not panic
+ let result = variant_get(&variant_array_ref, options).unwrap();
+
+ let struct_result = result.as_any().downcast_ref::<StructArray>().unwrap();
+ assert_eq!(struct_result.len(), 2);
+ assert_eq!(struct_result.num_columns(), 2);
+
+ // The missing variant field should be all nulls
+ let missing_col = struct_result.column(1);
+ assert_eq!(missing_col.null_count(), missing_col.len());
+
+ // The missing variant field should preserve VariantType extension metadata
+ let missing_schema_field = struct_result
+ .fields()
+ .iter()
+ .find(|f| f.name() == "missing_field")
+ .unwrap();
+ assert!(
+ missing_schema_field
+ .try_extension_type::<VariantType>()
+ .is_ok(),
+ "missing variant field should preserve VariantType extension metadata"
+ );
+
+ // The missing variant field should be a valid VariantArray
+ let missing_variant = VariantArray::try_new(missing_col);
+ assert!(
+ missing_variant.is_ok(),
+ "missing variant field should be a valid VariantArray"
+ );
+ }
Review Comment:
We should add a test that distinguishes handling json `Null` and missing value.
```suggestion
}
/// VariantType field extraction should distinguish:
/// - explicit JSON null => present Variant::Null
/// - missing path => SQL NULL
#[test]
fn test_struct_variant_field_distinguishes_missing_and_variant_null() {
let json_strings = vec![
r#"{"id": 1, "data": null}"#,
r#"{"id": 2}"#,
r#"{"id": 3, "data": {"score": 95}}"#,
];
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -4376,4 +4434,167 @@ mod test {
);
}
}
+
+ /// Test extracting a struct with mixed typed and variant fields.
+ /// Fields with VariantType extension metadata should be extracted as VariantArrays.
+ #[test]
+ fn test_struct_extraction_with_variant_fields() {
+ // Create test data: [{"id": 1, "name": "Alice", "data": {"score": 95}},
+ // {"id": 2, "name": "Bob", "data": null}]
+ let json_strings = vec![
+ r#"{"id": 1, "name": "Alice", "data": {"score": 95}}"#,
+ r#"{"id": 2, "name": "Bob", "data": null}"#,
+ r#"{"id": 3, "name": null, "data": {"level": 5}}"#,
Review Comment:
I think it's self explanatory, or at least there's no need to show the values.
```suggestion
// Create test data
let json_strings = vec![
r#"{"id": 1, "name": "Alice", "data": {"score": 95}}"#,
r#"{"id": 2, "name": "Bob", "data": null}"#,
r#"{"id": 3, "name": null, "data": {"level": 5}}"#,
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3015277130
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -4237,4 +4291,143 @@ mod test {
);
}
}
+
+ /// Test extracting a struct with mixed typed and variant fields.
+ /// Fields with VariantType extension metadata should be extracted as VariantArrays.
+ #[test]
+ fn test_struct_extraction_with_variant_fields() {
+ // Create test data: [{"id": 1, "name": "Alice", "data": {"score": 95}},
+ // {"id": 2, "name": "Bob", "data": null}]
+ let json_strings = vec![
+ r#"{"id": 1, "name": "Alice", "data": {"score": 95}}"#,
+ r#"{"id": 2, "name": "Bob", "data": null}"#,
+ r#"{"id": 3, "name": null, "data": {"level": 5}}"#,
+ ];
+ let string_array: Arc<dyn Array> = Arc::new(StringArray::from(json_strings));
+ let variant_array = json_to_variant(&string_array).unwrap();
+
+ // Request struct where:
+ // - "id" is extracted as Int32
+ // - "name" is extracted as String (Utf8)
+ // - "data" is extracted as Variant (using VariantType extension metadata)
+ let struct_fields = Fields::from(vec![
+ Field::new("id", DataType::Int32, true),
+ Field::new("name", DataType::Utf8, true),
+ // Use VariantType extension metadata to request extraction as VariantArray.
+ // The data type must be Struct to satisfy VariantType::supports_data_type.
+ Field::new("data", DataType::Struct(Fields::empty()), true)
+ .with_extension_type(VariantType),
+ ]);
+ let struct_type = DataType::Struct(struct_fields);
+
+ let options = GetOptions {
+ path: VariantPath::default(),
+ as_type: Some(Arc::new(Field::new("result", struct_type, true))),
+ cast_options: CastOptions::default(),
+ };
+
+ let variant_array_ref = ArrayRef::from(variant_array);
+ let result = variant_get(&variant_array_ref, options).unwrap();
+
+ // Verify the result is a StructArray with 3 fields
+ let struct_result = result.as_any().downcast_ref::<StructArray>().unwrap();
+ assert_eq!(struct_result.len(), 3);
+ assert_eq!(struct_result.num_columns(), 3);
+
+ // Verify "id" field (Int32)
+ let id_field = struct_result
+ .column(0)
+ .as_any()
+ .downcast_ref::<Int32Array>()
+ .unwrap();
+ assert_eq!(id_field.value(0), 1);
+ assert_eq!(id_field.value(1), 2);
+ assert_eq!(id_field.value(2), 3);
+
+ // Verify "name" field (String/Utf8)
+ let name_field = struct_result
+ .column(1)
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .unwrap();
+ assert_eq!(name_field.value(0), "Alice");
+ assert_eq!(name_field.value(1), "Bob");
+ assert!(name_field.is_null(2)); // null name in row 2
+
+ // Verify "data" field schema has VariantType extension metadata
+ let data_schema_field = struct_result
+ .fields()
+ .iter()
+ .find(|f| f.name() == "data")
+ .unwrap();
+ assert!(
+ data_schema_field
+ .try_extension_type::<VariantType>()
+ .is_ok(),
+ "data field should have VariantType extension metadata"
+ );
+
+ // Verify "data" field (VariantArray)
+ let data_field = struct_result.column(2);
+ // The data field should be a StructArray representing VariantArray's internal structure
+ // It has columns: metadata, value (optional), typed_value (optional)
+ let data_as_struct = data_field.as_any().downcast_ref::<StructArray>();
+ assert!(
+ data_as_struct.is_some(),
+ "data field should be a VariantArray (represented as StructArray)"
+ );
+
+ // Verify we can access the variant values
+ let data_variant_array = VariantArray::try_new(data_field).unwrap();
+ assert_eq!(data_variant_array.len(), 3);
+
+ // Row 0: data = {"score": 95}
+ let data0 = data_variant_array.value(0);
+ assert!(matches!(data0, Variant::Object(_)));
Review Comment:
Good idea
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3059150750
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
Review Comment:
A reminder to clean this once #9677 is closed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3058667538
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
Review Comment:
This races "silently" with https://github.com/apache/arrow-rs/pull/9677 -- no obvious (physical) merge conflict.
Should we wait for the other to merge and take advantage of the new API?
Or coordinate with @sdf-jkl to merge this PR first and update the other PR to optimize this new call site?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,12 +88,13 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
+ // In a shredded variant object, every field must be a struct containing
+ // `value` and/or `typed_value` sub-fields. A non-struct field here indicates
+ // malformed shredded data, so we error out regardless of cast safety mode.
let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
+ "Expected shredded variant struct for field '{}', got {}",
Review Comment:
Is this perhaps clearer?
```suggestion
"Physical layout of shredded variant field '{}' should be a struct, got {}",
```
I'm struggling with the right wording to convey that this particular struct is part of shredding's internal layout, unrelated to the logical data type the user supplied. Similar to how maps are physically represented as structs.
(the code comment is very nice and clear btw -- I'm only worried about this error message)
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
+ // so we must update the field's data type and re-attach VariantType.
+ //
+ // When the field is entirely absent, shredded_get_path returns a
+ // NullArray. Construct an all-null VariantArray so the extension
+ // metadata is preserved.
+ let child = if child.data_type() == &DataType::Null {
+ let mut builder = VariantArrayBuilder::new(child.len());
+ for _ in 0..child.len() {
+ builder.append_null();
+ }
Review Comment:
There's been a recent bunch of PR adding (or expanding) support for bulk-appending NULL values to array builders, e.g. [PrimitiveArrayBuilder::append_nulls](https://docs.rs/arrow/latest/arrow/array/struct.PrimitiveBuilder.html#method.append_nulls), [StructBuilder::append_nulls](https://docs.rs/arrow/latest/arrow/array/struct.StructBuilder.html#method.append_nulls), etc. Should we take a follow-on item to add that support to variant array builder as well? (Some builders have `append_values` methods as well).
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
+ // so we must update the field's data type and re-attach VariantType.
+ //
+ // When the field is entirely absent, shredded_get_path returns a
Review Comment:
```suggestion
// Additionally, when the field is entirely absent, shredded_get_path returns a
```
(it took me a minute to grok that these are two mostly-separate issues)
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
Review Comment:
Wait... is it ever (supposed to be) an empty struct?
The extension type blindly accepts any struct, so I guess that does work. And I don't see any code that would help a user create a correct physical variant struct (binary variant, shredded, etc).
Which opens up an interesting design question, because a request for variant_get to return variant could be any of three "flavors":
1. Generic
* "give me back variant, in whatever form it takes"
* If the underlying path is already variant, return it unchanged
* If strongly typed, there's a very fast path to shred it -- especially if the column contains no nulls
* Probably the most common, and also what this PR does (I think)
2. Unshredded
* "give me back binary variant no matter what form the data takes"
* If the underlying path is variant, unshred it as needed
* If strongly typed, convert to binary variant
* Useful for a caller who doesn't want to deal with (or cannot handle) the complexity and physical schema heterogeneity of shredding
3. Shredded
* "give me back shredded variant following the specific shredding schema I provided"
* If the underlying path is variant, shred or reshred as needed
* If the underlying path is strongly typed, shred it using the provided shredding schema
* Useful for a caller who expects a specific format to the data but needs to handle outliers
I guess the best way to handle this is for users to send an empty struct for 1/, with a non-empty struct triggering 2/ and 3/ as appropriate? With the usual recursive pathing to handle each leaf field as efficiently as possible? But we don't currently have a way to pass a shredding schema, so everyone is forced to 1/.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
+ // so we must update the field's data type and re-attach VariantType.
+ //
+ // When the field is entirely absent, shredded_get_path returns a
+ // NullArray. Construct an all-null VariantArray so the extension
+ // metadata is preserved.
+ let child = if child.data_type() == &DataType::Null {
+ let mut builder = VariantArrayBuilder::new(child.len());
+ for _ in 0..child.len() {
+ builder.append_null();
+ }
+ Arc::new(builder.build().into_inner()) as ArrayRef
+ } else {
+ child
+ };
+ let new_field = field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone())
+ .with_extension_type(VariantType);
+ updated_fields.push(new_field);
Review Comment:
nit: We're cloning and then discarding the field's existing data type.
```suggestion
let new_field = Field::new(
field.name().clone(),
child.data_type().clone(),
field.is_nullable(),
);
updated_fields.push(Arc::new(new_field.with_extension_type(VariantType)));
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
Review Comment:
This relates to https://github.com/apache/arrow-rs/pull/9598#discussion_r3022228974, where I was worried that variant extension type would "leak" -- because I had been assuming we covered all three cases above, and that variant extension type was used recursively as the marker to distinguish between strongly typed vs. shredding schemas (which becomes a problem once we recurse into a shredded schema).
In effect, we have several different "flavors" of calls to `shredded_path_get`:
1. No specific type requested (return whatever we find)
2. Strong type requested (parse out the variant values as needed)
3. Specific variant type requested (provided field is a struct with variant extension type metadata)
4. Recursing into the shredded field of a specific requested variant type
The problem is, a strongly typed struct (2/) is indistinguishable from a shredded variant field (4/) -- both fields are structs lacking variant extension type metadata.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3016563038
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +88,14 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray, so it cannot be
+ // followed further. Fall back to the value column if present,
+ // otherwise the path is missing.
Review Comment:
I don't understand this comment. What value column fallback does it refer to? The only caller of this `follow_shredded_path_element` function simply creates an all-null array if we return `missing_path_step()`.
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +224,61 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray. For variant fields, construct an all-null
+ // VariantArray so the extension metadata is preserved.
+ let child = if is_variant_field && child.data_type() == &DataType::Null {
+ let mut builder = VariantArrayBuilder::new(child.len());
+ for _ in 0..child.len() {
+ builder.append_null();
+ }
+ let null_variant = builder.build();
+ Arc::new(null_variant.into_inner()) as ArrayRef
+ } else {
+ child
+ };
+
+ // Update field data type to match the actual child array.
+ // Preserve VariantType extension metadata for variant fields so
+ // downstream consumers can recognize them as Variant columns.
+ let mut new_field = field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone());
Review Comment:
Rescuing https://github.com/apache/arrow-rs/pull/9598#discussion_r2969786655 from github oblivion:
> > Why needed? Doesn't it already have that type?
>
> For example, a VariantArray's internal StructArray has concrete fields like `metadata` and `value`, not empty fields. So we need `with_data_type` to align the field's declared type with the actual child array's type, otherwise `StructArray::try_new` will reject the mismatch.
I think there are two different scenarios here:
1. Caller requested a strongly typed struct. Then the input schema and child data type already match. Even for missing columns, the shredding code returns a properly typed array full of nulls, rather than a `NullArray`. No variant extension type attached.
2. Caller requested a (possibly shredded) variant. Then the input schema and child data type may _not_ match and we have to re-apply it and re-attach the variant extension type.
Right now, the code treats everything as case 2/, which is extra work but otherwise (mostly?) harmless.
Re "mostly harmless": How do we handle recursive variant shredding? For example, if I construct a shredding schema corresponding to `x.y.z::INT`, the resulting "happy path" is `x.typed_value.y.typed_value.z.typed_value: INT`. If I then pass that schema to `variant_get`, I have specifically requested that specific shredding schema, even if that means shredding or unshred-then-reshredding the underlying data whose own shredding schema doesn't match. But only the top-level column `x` has the variant extension type. It is incorrect for `x.typed_value.y` to be annotated as variant. Perhaps we still pass that annotation in order to guide the recursion, but the schema that comes back to the user afterward must not include it.
I'm not sure if we're handling these cases correctly right now?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +88,14 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray, so it cannot be
+ // followed further. Fall back to the value column if present,
+ // otherwise the path is missing.
Review Comment:
Update: I _think_ I finally figured it out... this code is attempting to traverse through a shredded variant object. Say we're following the path `x.y.z` and the current path step is `y`:
1. (L72) The `typed_value` we start with should ideally be a struct. If it's any other type, then `x` was not shredded as struct. Two sub-cases:
1. `x.value` is present. Some entries may be `Variant::Object`. In fact, it's possible that _every_ entry is `Variant::Object` (or `Variant::Null`), e.g. if the data match the query schema but the wrong shredding schema was applied when writing it out. So it's actually incorrect to blow up here, regardless of JSONpath semantics, because we didn't prove there's a problem yet (have to row-shred `x.value` and see what it contains).
2. `x.value` column is just plain missing. This means the column shredded perfectly as some other type, and we know there are no nulls and no structs. This is currently treated as an error case, but we should really just return `missing_path_step()` instead (https://github.com/apache/arrow-rs/issues/9606).
3. (L86) `x.typed_value` was a struct as expected, but `x.typed_value.y` doesn't exist => `missing_path_step()`
4. (L91) The `x.typed_value.y` exists, but it's not a struct. This is a malformed data scenario because we're in a shredded variant object and the spec mandates that _every_ field must be a struct (containing `value` and/or `typed_value` fields). So the original code was correct to error out upon discovering the problem, regardless of JSONpath semantics (it just failed to explain why it was correct).
5. (L101) `x.typed_value.y` is a struct, so we attempt to return a shredding state for its `value` and/or `typed_value` fields.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9598: Support extracting struct fields as Variant using ExtensionType
URL: https://github.com/apache/arrow-rs/pull/9598
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#issuecomment-4101752454
@scovich @sdf-jkl @klion26 Feel free to give me some advice when you have time. Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3015262955
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
+ let child = shredded_get_path(
&target,
&[VariantPathElement::from(field.name().as_str())],
- Some(field),
+ field_as_type,
cast_options,
- )
+ )?;
+
+ // Update field type if it was a Variant marker (extracted as VariantArray).
+ // The actual data type will be the internal structure of VariantArray.
+ // Preserve VariantType extension metadata so downstream consumers
+ // can recognize this field as a Variant column.
+ //
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray (DataType::Null). VariantType only supports
+ // Struct storage, so we must skip the extension in that case.
+ let updated_field =
+ if is_variant_field && matches!(child.data_type(), DataType::Struct(_)) {
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone())
+ .with_extension_type(VariantType)
+ } else if is_variant_field {
+ // Field was requested as Variant but data is all-null;
+ // preserve the data type from the child without extension metadata.
Review Comment:
Good point — it shouldn't strip the extension just because the data is all-null.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3060557898
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ if is_variant_field {
+ // Variant field: the child's actual data type (VariantArray's internal
+ // StructArray) differs from the user's declared type (empty Struct),
+ // so we must update the field's data type and re-attach VariantType.
+ //
+ // When the field is entirely absent, shredded_get_path returns a
+ // NullArray. Construct an all-null VariantArray so the extension
+ // metadata is preserved.
+ let child = if child.data_type() == &DataType::Null {
+ let mut builder = VariantArrayBuilder::new(child.len());
+ for _ in 0..child.len() {
+ builder.append_null();
+ }
Review Comment:
Filed here #9684. Should be a quick one.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r2969786655
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
+ let child = shredded_get_path(
&target,
&[VariantPathElement::from(field.name().as_str())],
- Some(field),
+ field_as_type,
cast_options,
- )
+ )?;
+
+ // Update field type if it was a Variant marker (extracted as VariantArray).
+ // The actual data type will be the internal structure of VariantArray.
+ // Preserve VariantType extension metadata so downstream consumers
+ // can recognize this field as a Variant column.
+ //
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray (DataType::Null). VariantType only supports
+ // Struct storage, so we must skip the extension in that case.
+ let updated_field =
+ if is_variant_field && matches!(child.data_type(), DataType::Struct(_)) {
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone())
Review Comment:
Why needed? Doesn't it already have that type?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
+ let child = shredded_get_path(
&target,
&[VariantPathElement::from(field.name().as_str())],
- Some(field),
+ field_as_type,
cast_options,
- )
+ )?;
+
+ // Update field type if it was a Variant marker (extracted as VariantArray).
+ // The actual data type will be the internal structure of VariantArray.
+ // Preserve VariantType extension metadata so downstream consumers
+ // can recognize this field as a Variant column.
+ //
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray (DataType::Null). VariantType only supports
+ // Struct storage, so we must skip the extension in that case.
+ let updated_field =
+ if is_variant_field && matches!(child.data_type(), DataType::Struct(_)) {
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone())
+ .with_extension_type(VariantType)
+ } else if is_variant_field {
+ // Field was requested as Variant but data is all-null;
+ // preserve the data type from the child without extension metadata.
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone())
+ } else {
+ field.as_ref().clone()
+ };
+ updated_fields.push(updated_field);
+
+ Ok(child)
})
- .collect::<Result<Vec<_>>>()?;
+ .collect();
+ let children = children?;
Review Comment:
This bit seems like a gratuitous change?
```rust
let children = fields...collect::<Result<Vec_>>>()?;
```
becomes
```rust
let children: Result<Vec<_>> = fields...collect();
let children = children?;
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +87,21 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray (VariantArray),
+ // which means it's not shredded further.
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
Review Comment:
* Probably clashes with https://github.com/apache/arrow-rs/issues/9606
(but maybe we want to fix that everywhere in a separate dedicated PR)
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
+ let child = shredded_get_path(
&target,
&[VariantPathElement::from(field.name().as_str())],
- Some(field),
+ field_as_type,
cast_options,
- )
+ )?;
+
+ // Update field type if it was a Variant marker (extracted as VariantArray).
+ // The actual data type will be the internal structure of VariantArray.
+ // Preserve VariantType extension metadata so downstream consumers
+ // can recognize this field as a Variant column.
+ //
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray (DataType::Null). VariantType only supports
+ // Struct storage, so we must skip the extension in that case.
+ let updated_field =
+ if is_variant_field && matches!(child.data_type(), DataType::Struct(_)) {
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone())
+ .with_extension_type(VariantType)
+ } else if is_variant_field {
+ // Field was requested as Variant but data is all-null;
+ // preserve the data type from the child without extension metadata.
Review Comment:
Is this intentional, to strip away e.g. JSON or other extensions that might be present?
Also, why is it correct to change the data type merely because the data are all-null?
Shouldn't it just come back as an all-null `StructArray` with variant annotations?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
Review Comment:
This is a bit of a double negative... trying to decide if it could be clearer with inverted logic?
```rust
let is_strongly_typed = field.try_extension_type::<VariantType>().is_err();
let field_as_type = is_strongly_typed.then(|| field.as_ref());
```
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
+ let child = shredded_get_path(
&target,
&[VariantPathElement::from(field.name().as_str())],
- Some(field),
+ field_as_type,
cast_options,
- )
+ )?;
+
+ // Update field type if it was a Variant marker (extracted as VariantArray).
+ // The actual data type will be the internal structure of VariantArray.
+ // Preserve VariantType extension metadata so downstream consumers
+ // can recognize this field as a Variant column.
+ //
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray (DataType::Null). VariantType only supports
+ // Struct storage, so we must skip the extension in that case.
+ let updated_field =
+ if is_variant_field && matches!(child.data_type(), DataType::Struct(_)) {
+ field
+ .as_ref()
+ .clone()
Review Comment:
This part is the same in all cases. Can we simplify by hoisting out a `let mut new_field` and then:
```rust
let updated_field = if is_strongly_typed {
new_field
} else {
// handle a variant field that may or may not exist in the data (see PR comment below)
};
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#issuecomment-4122970889
Thank you for the review. Looks like #9612 needs to be merged first.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3025624955
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +88,14 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray, so it cannot be
+ // followed further. Fall back to the value column if present,
+ // otherwise the path is missing.
Review Comment:
Great analysis, thank you. In a shredded variant object,
every field must be a struct containing `value` and/or `typed_value` sub-fields per the spec.
A non-struct field here means malformed data, and the original error was correct.
I've reverted to returning an InvalidArgumentError:
let struct_array = field.as_struct_opt().ok_or_else(|| {
ArrowError::InvalidArgumentError(format!(
"Expected shredded variant struct for field '{}', got {}",
name,
field.data_type(),
))
})?;
The error message now names the field for easier debugging.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3015264846
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -216,22 +223,63 @@ fn shredded_get_path(
// Structs are special. Recurse into each field separately, hoping to follow the shredding even
// further, and build up the final struct from those individually shredded results.
if let DataType::Struct(fields) = as_field.data_type() {
- let children = fields
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let children: Result<Vec<_>> = fields
.iter()
.map(|field| {
- shredded_get_path(
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type: Option<&Field> = if is_variant_field {
+ None
+ } else {
+ Some(field.as_ref())
+ };
+ let child = shredded_get_path(
&target,
&[VariantPathElement::from(field.name().as_str())],
- Some(field),
+ field_as_type,
cast_options,
- )
+ )?;
+
+ // Update field type if it was a Variant marker (extracted as VariantArray).
+ // The actual data type will be the internal structure of VariantArray.
+ // Preserve VariantType extension metadata so downstream consumers
+ // can recognize this field as a Variant column.
+ //
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray (DataType::Null). VariantType only supports
+ // Struct storage, so we must skip the extension in that case.
+ let updated_field =
+ if is_variant_field && matches!(child.data_type(), DataType::Struct(_)) {
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone())
Review Comment:
For example, a VariantArray's internal StructArray has concrete fields
like `metadata` and `value`, not empty fields. So we need `with_data_type` to align the
field's declared type with the actual child array's type, otherwise `StructArray::try_new`
will reject the mismatch.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3015283122
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +87,21 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray (VariantArray),
+ // which means it's not shredded further.
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Expected Struct array while following path, got {}",
+ field.data_type(),
+ )));
+ }
+ // In safe mode, return NotShredded to let the caller
+ // handle it via value column.
+ return Ok(ShreddedPathStep::NotShredded);
Review Comment:
You're right. The proper behavior depends on whether a `value` column exists:
- If value column exists → `NotShredded` (data may be in the value column, need per-row extraction)
- If value column is absent → `Missing` (the path truly doesn't exist, return all-null)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3025630995
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +224,61 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
+ if target.typed_value_field().is_none() && !has_variant_fields {
return shred_basic_variant(target, VariantPath::default(), Some(as_field));
}
- let children = fields
- .iter()
- .map(|field| {
- shredded_get_path(
- &target,
- &[VariantPathElement::from(field.name().as_str())],
- Some(field),
- cast_options,
- )
- })
- .collect::<Result<Vec<_>>>()?;
+ let mut updated_fields = Vec::with_capacity(fields.len());
+ let mut children = Vec::with_capacity(fields.len());
+ for field in fields.iter() {
+ // If the field has VariantType extension metadata, extract it as a
+ // VariantArray instead of casting to the declared data type. This allows
+ // callers to request structs where some fields remain as variants.
+ // See test_struct_extraction_with_variant_fields for usage example.
+ let is_variant_field = field.try_extension_type::<VariantType>().is_ok();
+ let field_as_type = (!is_variant_field).then(|| field.as_ref());
+ let child = shredded_get_path(
+ &target,
+ &[VariantPathElement::from(field.name().as_str())],
+ field_as_type,
+ cast_options,
+ )?;
+
+ // When the field is entirely absent in the data, shredded_get_path
+ // returns a NullArray. For variant fields, construct an all-null
+ // VariantArray so the extension metadata is preserved.
+ let child = if is_variant_field && child.data_type() == &DataType::Null {
+ let mut builder = VariantArrayBuilder::new(child.len());
+ for _ in 0..child.len() {
+ builder.append_null();
+ }
+ let null_variant = builder.build();
+ Arc::new(null_variant.into_inner()) as ArrayRef
+ } else {
+ child
+ };
+
+ // Update field data type to match the actual child array.
+ // Preserve VariantType extension metadata for variant fields so
+ // downstream consumers can recognize them as Variant columns.
+ let mut new_field = field
+ .as_ref()
+ .clone()
+ .with_data_type(child.data_type().clone());
Review Comment:
Good question. VariantType does NOT leak to intermediate fields. Here's why:
The `is_variant_field` check is driven by the **user's requested schema**, not by the data's shredding structure. When a user marks field "metadata" with VariantType, we pass `as_type = None` to `shredded_get_path`, which returns a VariantArray directly without recursing into the struct branch. So the recursion stops at that level — inner fields of the VariantArray's internal structure never go through the VariantType annotation path.
For case 1 (strongly typed struct), I've also addressed your observation that `with_data_type` is redundant — strongly typed fields now use the original field directly, since the child data type already matches the requested type.
To make this clearer, I split the handling into two explicit branches:
if is_variant_field {
// Update data type + attach VariantType extension
// (or build all-null VariantArray when field is absent)
} else {
// Strongly typed: use original field as-is
}
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3025624955
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +88,14 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray, so it cannot be
+ // followed further. Fall back to the value column if present,
+ // otherwise the path is missing.
Review Comment:
Great analysis, thank you. In a shredded variant object, every field must be a struct containing `value` and/or `typed_value` sub-fields per the spec.
A non-struct field here means malformed data, and the original error was correct.
I've reverted to returning an InvalidArgumentError:
let struct_array = field.as_struct_opt().ok_or_else(|| {
ArrowError::InvalidArgumentError(format!(
"Expected shredded variant struct for field '{}', got {}",
name,
field.data_type(),
))
})?;
The error message now names the field for easier debugging.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r3060467665
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -223,26 +226,65 @@ fn shredded_get_path(
// For shredded/partially-shredded targets (`typed_value` present), recurse into each field
// separately to take advantage of deeper shredding in child fields.
if let DataType::Struct(fields) = as_field.data_type() {
- if target.typed_value_field().is_none() {
+ let has_variant_fields = fields
+ .iter()
+ .any(|f| f.try_extension_type::<VariantType>().is_ok());
Review Comment:
I'm fine either way. Whichever moves first, the other will follow.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#issuecomment-4157040665
@codephage2020 #9612 was merged
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r2986907761
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -4237,4 +4291,143 @@ mod test {
);
}
}
+
+ /// Test extracting a struct with mixed typed and variant fields.
+ /// Fields with VariantType extension metadata should be extracted as VariantArrays.
+ #[test]
+ fn test_struct_extraction_with_variant_fields() {
+ // Create test data: [{"id": 1, "name": "Alice", "data": {"score": 95}},
+ // {"id": 2, "name": "Bob", "data": null}]
+ let json_strings = vec![
+ r#"{"id": 1, "name": "Alice", "data": {"score": 95}}"#,
+ r#"{"id": 2, "name": "Bob", "data": null}"#,
+ r#"{"id": 3, "name": null, "data": {"level": 5}}"#,
+ ];
+ let string_array: Arc<dyn Array> = Arc::new(StringArray::from(json_strings));
+ let variant_array = json_to_variant(&string_array).unwrap();
+
+ // Request struct where:
+ // - "id" is extracted as Int32
+ // - "name" is extracted as String (Utf8)
+ // - "data" is extracted as Variant (using VariantType extension metadata)
+ let struct_fields = Fields::from(vec![
+ Field::new("id", DataType::Int32, true),
+ Field::new("name", DataType::Utf8, true),
+ // Use VariantType extension metadata to request extraction as VariantArray.
+ // The data type must be Struct to satisfy VariantType::supports_data_type.
+ Field::new("data", DataType::Struct(Fields::empty()), true)
+ .with_extension_type(VariantType),
+ ]);
+ let struct_type = DataType::Struct(struct_fields);
+
+ let options = GetOptions {
+ path: VariantPath::default(),
+ as_type: Some(Arc::new(Field::new("result", struct_type, true))),
+ cast_options: CastOptions::default(),
+ };
+
+ let variant_array_ref = ArrayRef::from(variant_array);
+ let result = variant_get(&variant_array_ref, options).unwrap();
+
+ // Verify the result is a StructArray with 3 fields
+ let struct_result = result.as_any().downcast_ref::<StructArray>().unwrap();
+ assert_eq!(struct_result.len(), 3);
+ assert_eq!(struct_result.num_columns(), 3);
+
+ // Verify "id" field (Int32)
+ let id_field = struct_result
+ .column(0)
+ .as_any()
+ .downcast_ref::<Int32Array>()
+ .unwrap();
+ assert_eq!(id_field.value(0), 1);
+ assert_eq!(id_field.value(1), 2);
+ assert_eq!(id_field.value(2), 3);
+
+ // Verify "name" field (String/Utf8)
+ let name_field = struct_result
+ .column(1)
+ .as_any()
+ .downcast_ref::<StringArray>()
+ .unwrap();
+ assert_eq!(name_field.value(0), "Alice");
+ assert_eq!(name_field.value(1), "Bob");
+ assert!(name_field.is_null(2)); // null name in row 2
+
+ // Verify "data" field schema has VariantType extension metadata
+ let data_schema_field = struct_result
+ .fields()
+ .iter()
+ .find(|f| f.name() == "data")
+ .unwrap();
+ assert!(
+ data_schema_field
+ .try_extension_type::<VariantType>()
+ .is_ok(),
+ "data field should have VariantType extension metadata"
+ );
+
+ // Verify "data" field (VariantArray)
+ let data_field = struct_result.column(2);
+ // The data field should be a StructArray representing VariantArray's internal structure
+ // It has columns: metadata, value (optional), typed_value (optional)
+ let data_as_struct = data_field.as_any().downcast_ref::<StructArray>();
+ assert!(
+ data_as_struct.is_some(),
+ "data field should be a VariantArray (represented as StructArray)"
+ );
+
+ // Verify we can access the variant values
+ let data_variant_array = VariantArray::try_new(data_field).unwrap();
+ assert_eq!(data_variant_array.len(), 3);
+
+ // Row 0: data = {"score": 95}
+ let data0 = data_variant_array.value(0);
+ assert!(matches!(data0, Variant::Object(_)));
Review Comment:
Do we need to assert the value of the variant
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +87,21 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray (VariantArray),
+ // which means it's not shredded further.
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
+ "Expected Struct array while following path, got {}",
+ field.data_type(),
+ )));
+ }
+ // In safe mode, return NotShredded to let the caller
+ // handle it via value column.
+ return Ok(ShreddedPathStep::NotShredded);
Review Comment:
Why does this isn't `Missing`? Correct me if I'm wrong: the code path requests a `VariantPathElement::Field`, which means it's in a struct, and the current field isn't a struct, so the requested `name` is missing?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#issuecomment-4160170940
> @codephage2020 #9612 was merged
Thanks! I'll pick this up again later tonight.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support extracting struct fields as Variant using ExtensionType [arrow-rs]
Posted by "codephage2020 (via GitHub)" <gi...@apache.org>.
codephage2020 commented on code in PR #9598:
URL: https://github.com/apache/arrow-rs/pull/9598#discussion_r2985754739
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -86,15 +87,21 @@ pub(crate) fn follow_shredded_path_element<'a>(
return Ok(missing_path_step());
};
- let struct_array = field.as_struct_opt().ok_or_else(|| {
- // TODO: Should we blow up? Or just end the traversal and let the normal
- // variant pathing code sort out the mess that it must anyway be
- // prepared to handle?
- ArrowError::InvalidArgumentError(format!(
- "Expected Struct array while following path, got {}",
- field.data_type(),
- ))
- })?;
+ // The field might be a VariantArray (StructArray) if shredded,
+ // or it might be a primitive array. Only proceed if it's a StructArray.
+ let Some(struct_array) = field.as_struct_opt() else {
+ // Field exists but is not a StructArray (VariantArray),
+ // which means it's not shredded further.
+ if !cast_options.safe {
+ return Err(ArrowError::CastError(format!(
Review Comment:
Agreed this should be addressed holistically in a separate PR . The current behavior maintains backward compatibility.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org