sdf-jkl commented on code in PR #10157:
URL: https://github.com/apache/arrow-rs/pull/10157#discussion_r3449075250
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -1046,11 +1091,16 @@ where
cast_options,
capacity,
NullValue::ArrayElement,
+ false,
Review Comment:
```suggestion
true,
```
if we keep `false` here it preserves the old too lenient cast semantics.
The branch explicitly checks if `shredded` is `true`.
Can check with (doesn't pass):
```rust
// `VariantToListArrowRowBuilder::try_new` hardcodes `shred = false` for
its element
// builder (see variant_to_arrow.rs). The struct-field path threads
`shred` correctly, so this
// asymmetry is invisible without a nested-list test.
#[test]
fn test_array_shredding_does_not_cast_bool_element_to_int() {
let input = build_variant_array(vec![VariantRow::List(vec![
VariantValue::from(1i64),
VariantValue::from(true),
VariantValue::from(2i64),
])]);
let list_schema = DataType::List(Arc::new(Field::new("item",
DataType::Int64, true)));
let result = shred_variant(&input, &list_schema).unwrap();
assert_eq!(result.len(), 1);
assert_list_structure_and_elements::<Int64Type, i32>(
&result,
1,
&[0, 3],
&[Some(3)],
&[None],
(
// element 1 (the bool) must NOT be shredded into
typed_value ...
&[Some(1), None, Some(2)],
// ... it must be preserved verbatim in the residual `value`
instead.
&[None, Some(Variant::BooleanTrue), None],
),
);
}
```
##########
parquet-variant/src/variant.rs:
##########
@@ -1399,27 +1177,25 @@ impl<'m, 'v> Variant<'m, 'v> {
/// let v2 = Variant::from(std::f64::consts::PI);
/// assert_eq!(v2.as_f32(), Some(std::f32::consts::PI));
///
- /// // and from boolean variant
- /// let v3 = Variant::BooleanTrue;
- /// assert_eq!(v3.as_f32(), Some(1.0));
- ///
- /// // and return inf if overflow
- /// let v4 = Variant::from(f64::MAX);
- /// assert_eq!(v4.as_f32(), Some(f32::INFINITY));
+ /// // but not from integers
+ /// let v3 = Variant::from(16777215i64);
+ /// assert_eq!(v3.as_f32(), None);
///
- /// // but not from other variants
- /// let v5 = Variant::from("hello!");
- /// assert_eq!(v5.as_f32(), None);
+ /// // or not from other variants
+ /// let v4 = Variant::from("hello!");
+ /// assert_eq!(v4.as_f32(), None);
/// ```
pub fn as_f32(&self) -> Option<f32> {
- self.as_num()
+ match *self {
+ Variant::Float(i) => Some(i),
+ Variant::Double(i) => Some(i as f32),
Review Comment:
Should `shred` do lossy narrowing `f64` to `f32`?
##########
parquet-variant/src/variant.rs:
##########
@@ -159,25 +151,6 @@ impl Deref for ShortString<'_> {
/// [specification]:
https://github.com/apache/parquet-format/blob/master/VariantEncoding.md
/// [Variant Shredding specification]:
https://github.com/apache/parquet-format/blob/master/VariantShredding.md
///
-/// # Casting Semantics
Review Comment:
I originally thought it's a wrong idea to decouple cast from
`parquet-variant` crate, but now I think it's very similar to how `arrow-array`
and `arrow-cast` are decoupled.
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -1151,11 +1201,16 @@ impl<'a> VariantToFixedSizeListArrowRowBuilder<'a> {
cast_options,
capacity,
NullValue::ArrayElement,
+ false,
Review Comment:
```suggestion
true,
```
same as above
--
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]
