scovich commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2442520553
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -545,3 +557,21 @@ impl VariantToBinaryVariantArrowRowBuilder {
Ok(ArrayRef::from(variant_array))
}
}
+
+trait AppendValueTrait {
+ // NullBuilder will always append `()`
+ fn append_value(&mut self, v: ());
+}
+
+impl AppendValueTrait for NullBuilder {
+ fn append_value(&mut self, _v: ()) {
+ self.append_null();
+ }
+}
+
+define_variant_to_primitive_builder!(
+ struct VariantToNullArrowRowBuilder<'a>
+ |_capacity| -> NullBuilder { NullBuilder::new() },
+ | v | v.as_null(),
Review Comment:
nit: odd spacing there?
```suggestion
|v| v.as_null(),
```
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -968,6 +964,42 @@ fn typed_value_to_variant<'a>(
DataType::Float64 => {
primitive_conversion_single_value!(Float64Type, typed_value, index)
}
+ DataType::Decimal32(_, s) => {
+ generic_conversion_single_value!(
+ Decimal32Type,
+ as_primitive,
+ |v| VariantDecimal4::try_new(v, *s as u8).unwrap(),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal64(_, s) => {
+ generic_conversion_single_value!(
+ Decimal64Type,
+ as_primitive,
+ |v| VariantDecimal8::try_new(v, *s as u8).unwrap(),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal128(_, s) => {
+ generic_conversion_single_value!(
+ Decimal128Type,
+ as_primitive,
+ |v| VariantDecimal16::try_new(v, *s as u8).unwrap(),
Review Comment:
Are we _sure_ these `try_new` calls will never fail?
I mean, we _are_ unshredding shredded data, but it's also... data... which
is _not_ trustworthy.
For safety, suggest to do something like:
```suggestion
|v| VariantDecimal16::try_new(v, *s as
u8).map_or(Variant::Null, Variant::from),
```
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -89,6 +90,9 @@ impl_primitive_from_variant!(
as_naive_date,
datatypes::Date32Type::from_naive_date
);
+impl_primitive_from_variant!(datatypes::Time64MicrosecondType, as_time_utc,
|v| {
+ (v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64
Review Comment:
Is it normal to take the floor instead of rounding?
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -545,3 +557,21 @@ impl VariantToBinaryVariantArrowRowBuilder {
Ok(ArrayRef::from(variant_array))
}
}
+
+trait AppendValueTrait {
+ // NullBuilder will always append `()`
+ fn append_value(&mut self, v: ());
+}
+
+impl AppendValueTrait for NullBuilder {
Review Comment:
Ok, now that I'm actually looking at the code (and not just reacting to the
comments) --
It seems like we don't actually need a `NullBuilder` at all? We know the
capacity, so just use
[NullArray::new](https://docs.rs/arrow/latest/arrow/array/struct.NullArray.html#method.new):
```rust
struct FakeNullBuilder(NullArray);
impl FakeNullBuilder {
fn new(capacity: usize) -> Self {
Self(NullArray::new(capacity)
}
fn append_value(&mut self, _: ()) {}
fn append_null(&mut self) {}
fn finish(self) -> NullArray {
self.0
}
}
```
... and then the macro invocation us just:
```rust
|capacity| -> FakeNullBuilder { FakeNullBuilder::new(capacity) },
```
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1004,6 +1036,19 @@ fn typed_value_to_variant<'a>(
index
)
}
+ DataType::Time64(TimeUnit::Microsecond) => {
+ generic_conversion_single_value!(
+ Time64MicrosecondType,
+ as_primitive,
+ |v| NaiveTime::from_num_seconds_from_midnight_opt(
+ (v / 1_000_000) as u32,
+ (v % 1_000_000) as u32 * 1000
+ )
+ .unwrap(),
Review Comment:
As above -- are we absolutely _sure_ the conversion will never fail? Or do
we need a `Variant::Null` fallback?
--
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]
