You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "superserious-dev (via GitHub)" <gi...@apache.org> on 2025/08/06 22:49:58 UTC

[PR] Implement `DataType::Float16` => `Variant::Float` [arrow-rs]

superserious-dev opened a new pull request, #8073:
URL: https://github.com/apache/arrow-rs/pull/8073

   # Which issue does this PR close?
   
   - Closes #8057
   
   # Rationale for this change
   Adds Float16 conversion to the `cast_to_variant` kernel
   
   # What changes are included in this PR?
   
   - a macro to make converting array type that require a cast simpler
   - conversion of `DataType::Float16` => `Variant::Float`
   
   # Are these changes tested?
   
   Yes, additional unit tests have been added.
   
   # Are there any user-facing changes?
   
   Yes, adds new type conversion to kernel
   


-- 
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] Implement `DataType::Float16` => `Variant::Float` [arrow-rs]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8073:
URL: https://github.com/apache/arrow-rs/pull/8073


-- 
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] Implement `DataType::Float16` => `Variant::Float` [arrow-rs]

Posted by "superserious-dev (via GitHub)" <gi...@apache.org>.
superserious-dev commented on code in PR #8073:
URL: https://github.com/apache/arrow-rs/pull/8073#discussion_r2258488318


##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -92,6 +109,9 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
         DataType::UInt64 => {
             primitive_conversion!(UInt64Type, input, builder);
         }
+        DataType::Float16 => {
+            cast_conversion!(Float16Type, |v: f16| -> f32 { v.into() }, input, builder);

Review Comment:
   Casted f16 to f32 so that the value can be wrapped by Variant::Float.



-- 
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] Implement `DataType::Float16` => `Variant::Float` [arrow-rs]

Posted by "superserious-dev (via GitHub)" <gi...@apache.org>.
superserious-dev commented on code in PR #8073:
URL: https://github.com/apache/arrow-rs/pull/8073#discussion_r2258938677


##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -39,6 +40,22 @@ macro_rules! primitive_conversion {
     }};
 }
 
+/// Convert the input array to a `VariantArray` row by row,
+/// transforming each element with `cast_fn`
+macro_rules! cast_conversion {

Review Comment:
   I'm working on a couple of these issues in parallel and made some additional tweaks to the macro here: https://github.com/apache/arrow-rs/pull/8074/files.



-- 
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] Implement `DataType::Float16` => `Variant::Float` [arrow-rs]

Posted by "superserious-dev (via GitHub)" <gi...@apache.org>.
superserious-dev commented on code in PR #8073:
URL: https://github.com/apache/arrow-rs/pull/8073#discussion_r2258483206


##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -39,6 +40,22 @@ macro_rules! primitive_conversion {
     }};
 }
 
+/// Convert the input array to a `VariantArray` row by row,
+/// transforming each element with `cast_fn`
+macro_rules! cast_conversion {

Review Comment:
   This macro applies `cast_fn` to each element before converting it to a Variant. Some of the remaining types will require casting before being accepted by `Variant::from`.
   
   We could also add the `cast_fn` argument to `primitive_conversion!` macro and not add this macro. It would require passing something like `|v| v` (ie. a no-op function) to the existing primitive conversions that don't require casts.



##########
parquet-variant-compute/Cargo.toml:
##########
@@ -33,6 +33,7 @@ rust-version = { workspace = true }
 [dependencies]
 arrow = { workspace = true }
 arrow-schema = { workspace = true }
+half = { version = "2.1", default-features = false }

Review Comment:
   Needed to reference `f16` in the code and in the tests.



-- 
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] Implement `DataType::Float16` => `Variant::Float` [arrow-rs]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8073:
URL: https://github.com/apache/arrow-rs/pull/8073#discussion_r2259872716


##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -92,6 +109,9 @@ pub fn cast_to_variant(input: &dyn Array) -> Result<VariantArray, ArrowError> {
         DataType::UInt64 => {
             primitive_conversion!(UInt64Type, input, builder);
         }
+        DataType::Float16 => {
+            cast_conversion!(Float16Type, |v: f16| -> f32 { v.into() }, input, builder);

Review Comment:
   Makes sense to me. 
   
   In general, getting a macro that knows how to convert various Arrow types to Variant I think is an important building block



##########
parquet-variant-compute/Cargo.toml:
##########
@@ -33,6 +33,7 @@ rust-version = { workspace = true }
 [dependencies]
 arrow = { workspace = true }
 arrow-schema = { workspace = true }
+half = { version = "2.1", default-features = false }

Review Comment:
   Yeah -- we should probably directly export he `f16` type from the arrow crate (`pub use`) to avoid having users explicitly have to `use` half . Maybe as a follow on PR



-- 
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