scovich commented on code in PR #8521:
URL: https://github.com/apache/arrow-rs/pull/8521#discussion_r2393067849
##########
parquet-variant-compute/src/arrow_to_variant.rs:
##########
@@ -38,6 +38,98 @@ use parquet_variant::{
use std::collections::HashMap;
use std::ops::Range;
+// ============================================================================
+// Shared traits and helpers for Arrow-to-Variant conversion
+// ============================================================================
+
+/// Zero-cost trait for converting Arrow array values to Variant
+pub(crate) trait ArrowToVariant: Array {
+ fn append_to_variant_builder(
+ &self,
+ builder: &mut impl VariantBuilderExt,
+ index: usize,
+ ) -> Result<(), ArrowError>;
+}
+
+/// Macro to define ArrowToVariant implementations with optional value
transformation
+macro_rules! define_arrow_to_variant {
Review Comment:
One thing I don't love is we now have _two_ complex macros that feel kind of
similar. This new macro simplified the `unshred_variant` code but has ~no
effect on the `cast_to_variant` code in this file. In fact, the builder
definition didn't even change (L397 on the left side of the diff):
```rust
define_row_builder!(
struct PrimitiveArrowToVariantBuilder<'a, T: ArrowPrimitiveType>
where T::Native: Into<Variant<'a, 'a>>,
|array| -> PrimitiveArray<T> { array.as_primitive() }
);
```
So in retrospect, I'm not sure what duplication this PR _actually_
eliminated?
Seems like it just moved some definitions around.
Any ideas on a better approach?
--
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]
