[Variant] Add try_value/value for VariantArray #8719
Conversation
The `try_value` will return Result<Variant, ArrowError> and `value` unwrap from `try_value`
| } | ||
|
|
||
| fn deserialize_metadata(_metadata: Option<&str>) -> Result<Self::Metadata, ArrowError> { | ||
| fn deserialize_metadata(_metadata: Option<&str>) -> Result<Self::Metadata> { |
There was a problem hiding this comment.
The modifications here were made because arrow::error::Result was introduced.
| let result = variant_get(&variant_array, options).unwrap(); | ||
| assert_eq!(3, result.len()); | ||
|
|
||
| for i in 0..3 { |
There was a problem hiding this comment.
The result is all null instead of an Array of Variant::Null because
builder.append_value(target.try_value(i)).unwrap_or(Variant::Null)inshredded_get_pathwill callbuilder.append_valuewithVariant::Null- the builder is
VariantToPrimitiveArrowRowBuilder, and will callT::from_variant(value)inbuilder.append_value - then will call
PrimitiveFromVariantforTime64Microsecond - This will
Variant::as_time_utccurrently, and will returnNoneas the input is notVariant::Time(_)
Not sure if we need to change the return value of variant_get here from result.is_null(i) to Variant::Null
There was a problem hiding this comment.
Seems we need to return an arrow null instead of Variant::Null here, as the doc of CastOptions, /// If true, return error on conversion failure. If false, insert null for failed conversions., seems the return
| builder.append_value(value)?; | ||
| } else { | ||
| builder.append_value(target.value(i))?; | ||
| builder.append_value(target.try_value(i).unwrap_or(Variant::Null))?; |
There was a problem hiding this comment.
| builder.append_value(target.try_value(i).unwrap_or(Variant::Null))?; | |
| match target.try_value(i) { | |
| Ok(v) => builder.append_value(v)?, | |
| Err(_) => builder.append_null()?, | |
| } |
uses append_null() because it is a bit smarter
| let v = arr.value($index); | ||
| match ($cast_fn)(v) { | ||
| Ok(var) => Ok(Variant::from(var)), | ||
| Err(e) => Err(ArrowError::CastError(format!("Cast failed: {e}"))), |
There was a problem hiding this comment.
| Err(e) => Err(ArrowError::CastError(format!("Cast failed: {e}"))), | |
| Err(e) => Err(ArrowError::CastError(format!( | |
| "Cast failed at index {idx} (array type: {ty}): {e}", | |
| idx = $index, | |
| ty = <$t as ArrowPrimitiveType>::DATA_TYPE | |
| ))), |
to give some more details in the error message
| let v = arr.value($index); | ||
| match ($cast_fn)(v) { | ||
| Ok(var) => Ok(Variant::from(var)), | ||
| Err(e) => Err(ArrowError::CastError(format!("Cast failed: {e}"))), |
| let result = variant_get(&variant_array, options).unwrap(); | ||
| assert_eq!(3, result.len()); | ||
|
|
||
| for i in 0..3 { |
There was a problem hiding this comment.
Seems we need to return an arrow null instead of Variant::Null here, as the doc of CastOptions, /// If true, return error on conversion failure. If false, insert null for failed conversions., seems the return
There was a problem hiding this comment.
Thank you @klion26 and @martin-g -- this looks good to me
cc @scovich or @liamzwbao in case you would like to review as well
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
It's strange that the |
|
Tried to run the benchmark on my laptap with the following steps, the results were roughly the same
result |
I agree -- it seems like maybe measurement error -- I have queued up another run |
| 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 | ||
| } | ||
| }; |
There was a problem hiding this comment.
This might be a bit better as it drops the value early
| 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 | |
| } | |
| }; | |
| match target.try_value(i) { | |
| Ok(v) => { | |
| let _ = builder.append_value(v)?; | |
| } | |
| Err(_) => builder.append_null()?, | |
| } |
There was a problem hiding this comment.
I tried this, but changed to the current solution.
We need to change the return type for builder.append_null(), (we need both match arms to return the same type), and we need to return Ok(false) for builder.append_null() to satisfy the semantics here, but the builder.append_null always succeeds(builder.append_value() will return Ok(true) if succeed). It seems weird to have different semantics for the return result of builder.append_value and builder.append_null.
|
🤖 |
|
🤖: Benchmark completed Details
|
|
On rerun there appears to be no performance difference, so merging this one in |
|
Thank you @klion26 @martin-g and @liamzwbao |
|
@alamb @martin-g @liamzwbao Thanks for the review and merging! |
5 participants
Which issue does this PR close?
VariantArray::valueto return aResult<Variant>#8672 .What changes are included in this PR?
try_value/valuefunction forVariantArrayVariantArray::try_valueAre these changes tested?
Covered by existing tests and added new tests
Are there any user-facing changes?
Yes, add a new function for
VariantArray::try_value, and theVariantArray::valuechanged to panic from returningVariant::Nullif there is some cast error.