scovich commented on code in PR #8673:
URL: https://github.com/apache/arrow-rs/pull/8673#discussion_r2449414192
##########
parquet-variant-compute/src/shred_variant.rs:
##########
@@ -400,6 +400,7 @@ mod tests {
let typed_value_field = result
.typed_value_field()
.unwrap()
+ .1
Review Comment:
Just glancing at this code in isolation, I would guess that .0 is the field
and .1 is the array?
But that requires knowing context (ie of this PR)
If this usage will show up often, is it worth returning a newtype instead of
a tuple?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -643,11 +654,22 @@ impl ShreddedVariantFieldArray {
) -> Self {
let mut builder = StructArrayBuilder::new();
if let Some(value) = value.clone() {
- builder = builder.with_field("value", Arc::new(value), true);
- }
- if let Some(typed_value) = typed_value.clone() {
- builder = builder.with_field("typed_value", typed_value, true);
+ builder = builder.with_column("value", Arc::new(value), true);
}
+
+ let typed_value = if let Some(typed_value_array) = typed_value.clone()
{
+ let field_ref = Arc::new(Field::new(
+ "typed_value",
+ typed_value_array.data_type().clone(),
+ true,
+ ));
+ builder = builder.with_field(field_ref.clone(),
typed_value_array.clone());
+
+ Some((field_ref, typed_value_array))
Review Comment:
because of the dual typing (one in field and one in array), this code is
technically no longer infallible -- the data types could disagree. Not sure the
best way to address that issue as long as we're passing a full-blown field.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -643,11 +654,22 @@ impl ShreddedVariantFieldArray {
) -> Self {
let mut builder = StructArrayBuilder::new();
if let Some(value) = value.clone() {
- builder = builder.with_field("value", Arc::new(value), true);
- }
- if let Some(typed_value) = typed_value.clone() {
- builder = builder.with_field("typed_value", typed_value, true);
+ builder = builder.with_column("value", Arc::new(value), true);
}
+
+ let typed_value = if let Some(typed_value_array) = typed_value.clone()
{
+ let field_ref = Arc::new(Field::new(
+ "typed_value",
+ typed_value_array.data_type().clone(),
+ true,
+ ));
+ builder = builder.with_field(field_ref.clone(),
typed_value_array.clone());
+
+ Some((field_ref, typed_value_array))
Review Comment:
Also, this logic is duplicated with the variant array constructor above.
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -289,13 +289,24 @@ impl VariantArray {
nulls: Option<NullBuffer>,
) -> Self {
let mut builder =
- StructArrayBuilder::new().with_field("metadata",
Arc::new(metadata.clone()), false);
+ StructArrayBuilder::new().with_column("metadata",
Arc::new(metadata.clone()), false);
if let Some(value) = value.clone() {
- builder = builder.with_field("value", Arc::new(value), true);
- }
- if let Some(typed_value) = typed_value.clone() {
- builder = builder.with_field("typed_value", typed_value, true);
+ builder = builder.with_column("value", Arc::new(value), true);
}
+
+ let typed_value = if let Some(typed_value_array) = typed_value.clone()
{
+ let field_ref = Arc::new(Field::new(
+ "typed_value",
+ typed_value_array.data_type().clone(),
+ true,
+ ));
+ builder = builder.with_field(field_ref.clone(),
typed_value_array.clone());
+
+ Some((field_ref, typed_value_array))
+ } else {
+ None
+ };
Review Comment:
nit: this is just `Option::map` (no `?` to complicate things)
--
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]
