You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "klion26 (via GitHub)" <gi...@apache.org> on 2025/10/17 07:11:41 UTC
[PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
klion26 opened a new pull request, #8638:
URL: https://github.com/apache/arrow-rs/pull/8638
# Which issue does this PR close?
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
- Closes #8637.
- Support Variant to Arrow for Null/Time/Decimlal{4,8,16}
# What changes are included in this PR?
- Add logic in `typed_value_to_variant`/`PrimitiveVariantToArrowRowBuilder` for `Null/Time/Decimal{4,8,16}`
- Implement `PrimitiveFromVariant` for `Time64MicrosecondType`
- Add tests to cover the added logic
-
# Are these changes tested?
Added some tests
# Are there any user-facing changes?
No
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2443676460
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1004,6 +1036,19 @@ fn typed_value_to_variant<'a>(
index
)
}
+ DataType::Time64(TimeUnit::Microsecond) => {
Review Comment:
Only support the `Time64(TimeUnit::Microsecond)` here because that [variant spec](https://github.com/apache/parquet-format/blob/master/VariantShredding.md#shredded-value-types) said that `Time` is `TimeUnit::Microsecond`.
<img width="770" height="674" alt="Image" src="https://github.com/user-attachments/assets/fa1af260-6622-49f5-ae77-4a8950dcfe12" />
##########
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:
> NullArray (with DataType::Null) is not the same as an array (of some arbitrary other type) full of nulls tho?
`new_null_array` with different types are different, the following unit test will fail
```
let first = new_null_array(&DataType::Int32, 3);
let second = new_null_array(&DataType::Timestamp(TimeUnit::Nanosecond, None), 3);
assert_eq!(*first, second.clone());
```
Use `NullArray` instead of `new_null_array` in the changed code because the compiler will panic with `the trait bound Result<Arc<dyn Array>, ArrowError>: Array is not satisfied` if I use `new_null_array` in `FakeNullBuilder::finish`
##########
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:
Thanks for this suggestion, fixed.
Ah, found that we can pass in _different_ types here(`VariantDecimal16` vs `Variant::Decimal16`), it is cool.
##########
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:
I think it is ok because `nanosecond()` will return `u32`, and the result will be truncated to zero.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "friendlymatthew (via GitHub)" <gi...@apache.org>.
friendlymatthew commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2448811263
##########
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).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal64(_, s) => {
+ generic_conversion_single_value!(
+ Decimal64Type,
+ as_primitive,
+ |v| VariantDecimal8::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal128(_, s) => {
+ generic_conversion_single_value!(
+ Decimal128Type,
+ as_primitive,
+ |v| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
Review Comment:
> We may need a follow-up item to track turning this into a proper Result instead of silently converting to NULL on failure?
I think this makes a lot of sense.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#issuecomment-3432857916
🚀 thanks @liamzwbao @klion26 and @scovich
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2438641209
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -920,17 +921,12 @@ fn typed_value_to_variant<'a>(
panic!("Invalid variant, conflicting value and typed_value");
}
match data_type {
+ DataType::Null => Variant::Null,
DataType::Boolean => {
let boolean_array = typed_value.as_boolean();
let value = boolean_array.value(index);
Variant::from(value)
}
- DataType::Date32 => {
Review Comment:
This has been adjusted to be grouped together with the Timestamp/Time.
##########
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:
The function is needed by the `define_variant_to_primitive_builder` macro.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2449568933
##########
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).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal64(_, s) => {
+ generic_conversion_single_value!(
+ Decimal64Type,
+ as_primitive,
+ |v| VariantDecimal8::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal128(_, s) => {
+ generic_conversion_single_value!(
+ Decimal128Type,
+ as_primitive,
+ |v| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
Review Comment:
> We may need a follow-up item to track turning this into a proper Result instead of silently converting to NULL on failure? That would require VariantArray::value to return a Result<Variant>, which is a biggish change but honestly seems appropriate given that we can't guarantee the input shredding is even physically valid, let alone logically valid? It seems too sharp an edge to just panic.
>
> @alamb -- thoughts?
How about we add a `try_value()` method that returns a Result and just have `value()` unwrap the result?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2441083307
##########
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:
The NullArray is kind of strange -- maybe rather than creating a full on builder for it, we could just keep count and then create the output array with https://docs.rs/arrow/latest/arrow/array/fn.new_null_array.html ?
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1004,6 +1036,19 @@ fn typed_value_to_variant<'a>(
index
)
}
+ DataType::Time64(TimeUnit::Microsecond) => {
Review Comment:
Another possible type is `DataType::Time64(TimeUnit::Nanosecond)`
There is also a Time32 type in arrow: https://docs.rs/arrow/latest/arrow/datatypes/enum.DataType.html#variant.Time32
I am not sure if we want to handle that too
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2448800909
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -545,3 +560,24 @@ impl VariantToBinaryVariantArrowRowBuilder {
Ok(ArrayRef::from(variant_array))
}
}
+
+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
+ }
+}
+
+define_variant_to_primitive_builder!(
+ struct VariantToNullArrowRowBuilder<'a>
+ |capacity| -> FakeNullBuilder { FakeNullBuilder::new(capacity) },
+ |value| value.as_null(),
Review Comment:
This is more obvious: the result is always `Variant::Null`; updated the code.
As macro can't receive `_` for `ident` (need change to pat and more), changed to `_value`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2445564261
##########
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:
Yeah, it accepts any `V: impl IntoVariant`
##########
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:
Yeah, it accepts any `V: IntoVariant`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2448802612
##########
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).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal64(_, s) => {
+ generic_conversion_single_value!(
+ Decimal64Type,
+ as_primitive,
+ |v| VariantDecimal8::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal128(_, s) => {
+ generic_conversion_single_value!(
+ Decimal128Type,
+ as_primitive,
+ |v| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
Review Comment:
Filed an issue #8672 to track this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2441371283
##########
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:
`NullArray` (with `DataType::Null`) is not the same as an array (of some arbitrary other type) full of nulls tho? But if we know the intended type, then I agree that `new_null_array` is better (since `NullArray::is_null` always returns false = weird)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2445573930
##########
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).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal64(_, s) => {
+ generic_conversion_single_value!(
+ Decimal64Type,
+ as_primitive,
+ |v| VariantDecimal8::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
+ typed_value,
+ index
+ )
+ }
+ DataType::Decimal128(_, s) => {
+ generic_conversion_single_value!(
+ Decimal128Type,
+ as_primitive,
+ |v| VariantDecimal16::try_new(v, *s as u8).map_or(Variant::Null, Variant::from),
Review Comment:
We may need a follow-up item to track turning this into a proper Result instead of silently converting to NULL on failure? That would require `VariantArray::value` to return a `Result<Variant>`, which is a biggish change but honestly seems appropriate given that we can't guarantee the input shredding is even physically valid, let alone logically valid? It seems too sharp an edge to just panic.
@alamb -- thoughts?
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -545,3 +560,24 @@ impl VariantToBinaryVariantArrowRowBuilder {
Ok(ArrayRef::from(variant_array))
}
}
+
+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
+ }
+}
+
+define_variant_to_primitive_builder!(
+ struct VariantToNullArrowRowBuilder<'a>
+ |capacity| -> FakeNullBuilder { FakeNullBuilder::new(capacity) },
+ |value| value.as_null(),
Review Comment:
Should we just hard-wire `|_| Some(Variant::Null)` here?
All input variant subtypes ultimately produce the same outcome because both append_null and append_value are no-ops.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Implement primitive type access for null/time/decimal* [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #8638:
URL: https://github.com/apache/arrow-rs/pull/8638#discussion_r2449912180
##########
parquet-variant-compute/src/variant_array.rs:
##########
@@ -1004,6 +1036,19 @@ fn typed_value_to_variant<'a>(
index
)
}
+ DataType::Time64(TimeUnit::Microsecond) => {
Review Comment:
supernit: perhaps we can move this branch to just below Date32 so that the order aligns the spec.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org