You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "liamzwbao (via GitHub)" <gi...@apache.org> on 2025/08/21 01:35:54 UTC
[PR] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
liamzwbao opened a new pull request, #8196:
URL: https://github.com/apache/arrow-rs/pull/8196
# Which issue does this PR close?
- Closes #8195.
# Rationale for this change
# What changes are included in this PR?
Implement `DataType::Union` for `cast_to_variant`
# Are these changes tested?
Yes
# Are there any user-facing changes?
New cast type supported
--
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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196#discussion_r2291833636
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -544,8 +530,41 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
Ok(builder.build())
}
-/// Generic function to process run-end encoded arrays
-fn process_run_end_encoded<R: RunEndIndexType>(
+/// Convert union arrays
+fn convert_union(
+ fields: &UnionFields,
+ input: &dyn Array,
+ builder: &mut VariantArrayBuilder,
+) -> Result<(), ArrowError> {
+ let union_array = input.as_union();
+
+ // Convert each child array to variant arrays
+ let mut child_variant_arrays = HashMap::new();
+ for (type_id, _) in fields.iter() {
+ let child_array = union_array.child(type_id);
+ let child_variant_array = cast_to_variant(child_array.as_ref())?;
+ child_variant_arrays.insert(type_id, child_variant_array);
+ }
+
+ // Process each element in the union array
+ for i in 0..union_array.len() {
+ let type_id = union_array.type_id(i);
+ let value_offset = union_array.value_offset(i);
+
+ if let Some(child_variant_array) = child_variant_arrays.get(&type_id) {
+ let value = child_variant_array.value(value_offset);
+ builder.append_variant(value);
+ } else {
+ // This should not happen in a valid union, but handle gracefully
Review Comment:
👍
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -544,8 +530,41 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
Ok(builder.build())
}
-/// Generic function to process run-end encoded arrays
-fn process_run_end_encoded<R: RunEndIndexType>(
+/// Convert union arrays
+fn convert_union(
+ fields: &UnionFields,
+ input: &dyn Array,
+ builder: &mut VariantArrayBuilder,
+) -> Result<(), ArrowError> {
+ let union_array = input.as_union();
+
+ // Convert each child array to variant arrays
+ let mut child_variant_arrays = HashMap::new();
+ for (type_id, _) in fields.iter() {
Review Comment:
I don't understand what you are suggesting
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -1620,6 +1667,93 @@ mod tests {
assert_eq!(obj4.get("age"), None);
}
+ #[test]
+ fn test_cast_to_variant_union_sparse() {
Review Comment:
could we please add a test for a UnionArray where the child element is null? So that the output VariantArray has a null as well?
--
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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196#issuecomment-3216711935
🚀
--
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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196#discussion_r2289658156
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -579,6 +598,34 @@ fn process_run_end_encoded<R: RunEndIndexType>(
Ok(())
}
+/// Convert dictionary encoded arrays
+fn convert_dictionary_encoded(
Review Comment:
This change just moves the code from the `Dictionary` branch into this helper function. Since all the impls use slightly different coding styles, we could do a follow-up PR to make them consistent once all the variant cast implementations are complete.
--
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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196#discussion_r2292558498
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -544,8 +530,41 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
Ok(builder.build())
}
-/// Generic function to process run-end encoded arrays
-fn process_run_end_encoded<R: RunEndIndexType>(
+/// Convert union arrays
+fn convert_union(
+ fields: &UnionFields,
+ input: &dyn Array,
+ builder: &mut VariantArrayBuilder,
+) -> Result<(), ArrowError> {
+ let union_array = input.as_union();
+
+ // Convert each child array to variant arrays
+ let mut child_variant_arrays = HashMap::new();
+ for (type_id, _) in fields.iter() {
Review Comment:
Ah, yes, we'll use the same child_variant_array many times. The current would have a better performance
--
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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196#discussion_r2290442934
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -544,8 +530,41 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
Ok(builder.build())
}
-/// Generic function to process run-end encoded arrays
-fn process_run_end_encoded<R: RunEndIndexType>(
+/// Convert union arrays
+fn convert_union(
+ fields: &UnionFields,
+ input: &dyn Array,
+ builder: &mut VariantArrayBuilder,
+) -> Result<(), ArrowError> {
+ let union_array = input.as_union();
+
+ // Convert each child array to variant arrays
+ let mut child_variant_arrays = HashMap::new();
+ for (type_id, _) in fields.iter() {
Review Comment:
Do we need to merge the two passes into 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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196#discussion_r2292454215
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -544,8 +530,41 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
Ok(builder.build())
}
-/// Generic function to process run-end encoded arrays
-fn process_run_end_encoded<R: RunEndIndexType>(
+/// Convert union arrays
+fn convert_union(
+ fields: &UnionFields,
+ input: &dyn Array,
+ builder: &mut VariantArrayBuilder,
+) -> Result<(), ArrowError> {
+ let union_array = input.as_union();
+
+ // Convert each child array to variant arrays
+ let mut child_variant_arrays = HashMap::new();
+ for (type_id, _) in fields.iter() {
Review Comment:
Do you mean using one loop instead of two? That way we will compute the child array of each type_id on demand.
But IIUC, we will use all the child arrays in the end if it's a valid union, and I think precomute all the child arrays may benefit the lookup in the second loop.
--
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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196
--
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] [Variant]: Implement `DataType::Union` support for `cast_to_variant` kernel [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8196:
URL: https://github.com/apache/arrow-rs/pull/8196#discussion_r2292454215
##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -544,8 +530,41 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
Ok(builder.build())
}
-/// Generic function to process run-end encoded arrays
-fn process_run_end_encoded<R: RunEndIndexType>(
+/// Convert union arrays
+fn convert_union(
+ fields: &UnionFields,
+ input: &dyn Array,
+ builder: &mut VariantArrayBuilder,
+) -> Result<(), ArrowError> {
+ let union_array = input.as_union();
+
+ // Convert each child array to variant arrays
+ let mut child_variant_arrays = HashMap::new();
+ for (type_id, _) in fields.iter() {
Review Comment:
Do you mean using one loop instead of two? That way we will compute the child array of each type_id on demand.
But IIUC, we will use all the child arrays anyway if it's a valid union, and I think precomute all the child arrays may benefit the lookup in the second loop.
--
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