scovich commented on code in PR #9791:
URL: https://github.com/apache/arrow-rs/pull/9791#discussion_r3126438440
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -190,7 +188,7 @@ impl<'a> UnshredVariantRowBuilder<'a> {
($enum_variant:ident, $cast_fn:ident) => {
Self::$enum_variant(UnshredPrimitiveRowBuilder::new(
value,
- typed_value.$cast_fn(),
+ typed_value.$cast_fn().clone(),
Review Comment:
Not sure I understand -- why would a cast return a borrowed value that needs
cloning?
##########
parquet-variant-compute/src/lib.rs:
##########
@@ -51,7 +51,7 @@ mod variant_array_builder;
mod variant_get;
mod variant_to_arrow;
-pub use variant_array::{BorrowedShreddingState, ShreddingState, VariantArray,
VariantType};
Review Comment:
aside: Was this already an unused import? I wonder why clippy didn't flag it
when it became unused?
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -257,27 +275,30 @@ impl<'a> UnshredVariantRowBuilder<'a> {
}
DataType::Struct(_) =>
Self::Struct(StructUnshredVariantBuilder::try_new(
value,
- typed_value.as_struct(),
+ typed_value.as_struct().clone(),
)?),
DataType::List(_) => Self::List(ListUnshredVariantBuilder::try_new(
value,
- typed_value.as_list(),
+ typed_value.as_list::<i32>().clone(),
Review Comment:
Similar question for these -- cost of cloning struct and list arrays?
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -203,13 +201,25 @@ impl<'a> UnshredVariantRowBuilder<'a> {
DataType::Float32 => primitive_builder!(PrimitiveFloat32,
as_primitive),
DataType::Float64 => primitive_builder!(PrimitiveFloat64,
as_primitive),
DataType::Decimal32(p, s) if
VariantDecimal4::is_valid_precision_and_scale(p, s) => {
- Self::Decimal32(DecimalUnshredRowBuilder::new(value,
typed_value, *s as _))
+ Self::Decimal32(DecimalUnshredRowBuilder::new(
+ value,
+ typed_value.as_primitive().clone(),
Review Comment:
How expensive is it to clone a `PrimitiveArray`? It's not directly an Arc?
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -394,13 +405,13 @@ macro_rules! handle_unshredded_case {
}
/// Generic unshred builder that works with any Array implementing
AppendToVariantBuilder
-struct UnshredPrimitiveRowBuilder<'a, T> {
- value: Option<&'a ArrayRef>,
- typed_value: &'a T,
+struct UnshredPrimitiveRowBuilder<T> {
+ value: Option<ArrayRef>,
+ typed_value: T,
Review Comment:
Changes like this one seem unrelated to shared shredding state that needed
an `Option<&ArrayRef>`? Even if `value` needs to be cloned, we could still keep
a borrowed reference to `typed_value` which is anyway a bare type?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
