alamb commented on code in PR #10358:
URL: https://github.com/apache/arrow-rs/pull/10358#discussion_r3669237405
##########
parquet-variant-compute/benches/variant_kernels.rs:
##########
@@ -223,6 +239,40 @@ fn create_shredded_utf8_variant_array(size: usize) ->
VariantArray {
.expect("created struct should be a valid shredded variant")
}
+fn create_unshredded_object_variant_array(size: usize) -> VariantArray {
Review Comment:
It would be helpful if you split the change to the benchmark off into a
separate PR I can merge it and then use the benchmarking scripts
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -241,27 +242,72 @@ fn shredded_get_path(
} else {
as_field.map(|f| f.data_type())
};
+ let has_path = !path.is_empty();
let mut builder = make_variant_to_arrow_row_builder(
target.metadata_column(),
path,
as_type,
cast_options,
target.len(),
)?;
- for i in 0..target.len() {
- if target.is_null(i) {
- builder.append_null()?;
- } else if !cast_options.safe {
- let value = target.try_value(i)?;
- builder.append_value(value)?;
- } else {
- let _ = match target.try_value(i) {
- Ok(v) => builder.append_value(v)?,
- Err(_) => {
- builder.append_null()?;
- false // add this to make match arms have the same
return type
+
+ if has_path
+ && target.typed_value_column().is_none()
+ && let Some(value_column) = target.value_column()
+ {
+ let metadata_column = target.metadata_column();
Review Comment:
Given that this function is already pretty long, could you please try and
refactor some of the logic into smaller documented functions that each do some
particular operation?
##########
parquet-variant-compute/src/variant_get.rs:
##########
@@ -241,27 +242,72 @@ fn shredded_get_path(
} else {
as_field.map(|f| f.data_type())
};
+ let has_path = !path.is_empty();
let mut builder = make_variant_to_arrow_row_builder(
target.metadata_column(),
path,
as_type,
cast_options,
target.len(),
)?;
- for i in 0..target.len() {
- if target.is_null(i) {
- builder.append_null()?;
- } else if !cast_options.safe {
- let value = target.try_value(i)?;
- builder.append_value(value)?;
- } else {
- let _ = match target.try_value(i) {
- Ok(v) => builder.append_value(v)?,
- Err(_) => {
- builder.append_null()?;
- false // add this to make match arms have the same
return type
+
+ if has_path
Review Comment:
Could you add some comments to explain the rationale for this condition /
fast path?
--
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]
