[Variant] Implement DataType::Float16 => Variant::Float#8073
[Variant] Implement DataType::Float16 => Variant::Float#8073alamb merged 1 commit intoapache:mainfrom
DataType::Float16 => Variant::Float#8073Conversation
|
|
||
| /// Convert the input array to a `VariantArray` row by row, | ||
| /// transforming each element with `cast_fn` | ||
| macro_rules! cast_conversion { |
There was a problem hiding this 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.
There was a problem hiding this 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.
| [dependencies] | ||
| arrow = { workspace = true } | ||
| arrow-schema = { workspace = true } | ||
| half = { version = "2.1", default-features = false } |
There was a problem hiding this comment.
Needed to reference f16 in the code and in the tests.
There was a problem hiding this 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
| primitive_conversion!(UInt64Type, input, builder); | ||
| } | ||
| DataType::Float16 => { | ||
| cast_conversion!(Float16Type, |v: f16| -> f32 { v.into() }, input, builder); |
There was a problem hiding this comment.
Casted f16 to f32 so that the value can be wrapped by Variant::Float.
There was a problem hiding this 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
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @superserious-dev
Given the helpfulness of this macro I am going to merge this PR in now to avoid conflicts / hopefully others can use it
| [dependencies] | ||
| arrow = { workspace = true } | ||
| arrow-schema = { workspace = true } | ||
| half = { version = "2.1", default-features = false } |
There was a problem hiding this 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
| primitive_conversion!(UInt64Type, input, builder); | ||
| } | ||
| DataType::Float16 => { | ||
| cast_conversion!(Float16Type, |v: f16| -> f32 { v.into() }, input, builder); |
There was a problem hiding this 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
alamb
changed the title
DataType::Float16 => Variant::FloatDataType::Float16 => Variant::Float
2 participants
Which issue does this PR close?
DataType::Float16support forcast_to_variantkernel #8057Rationale for this change
Adds Float16 conversion to the
cast_to_variantkernelWhat changes are included in this PR?
DataType::Float16=>Variant::FloatAre these changes tested?
Yes, additional unit tests have been added.
Are there any user-facing changes?
Yes, adds new type conversion to kernel