add support for f16 #888

jimexist · 2021-10-31T06:21:18Z

Which issue does this PR close?

Closes #890

Rationale for this change

float16 was not properly supported

What changes are included in this PR?

include optional feature f16 to use half crate to implement f16

Are there any user-facing changes?

codecov-commenter · 2021-10-31T06:48:47Z

Codecov Report

Merging #888 (3845a11) into master (898924f) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #888      +/-   ##
==========================================
- Coverage   82.45%   82.42%   -0.03%     
==========================================
  Files         168      168              
  Lines       48231    48244      +13     
==========================================
  Hits        39767    39767              
- Misses       8464     8477      +13

Impacted Files	Coverage Δ
arrow/src/alloc/types.rs	`0.00% <0.00%> (ø)`
arrow/src/array/array.rs	`83.13% <0.00%> (-0.25%)`	⬇️
arrow/src/array/data.rs	`72.92% <0.00%> (-1.20%)`	⬇️
arrow/src/array/equal/mod.rs	`93.13% <0.00%> (-0.33%)`	⬇️
arrow/src/array/transform/mod.rs	`85.16% <0.00%> (-0.36%)`	⬇️
arrow/src/datatypes/native.rs	`72.91% <0.00%> (-1.56%)`	⬇️
arrow/src/datatypes/types.rs	`88.88% <ø> (ø)`
arrow/src/json/reader.rs	`83.75% <ø> (+0.05%)`	⬆️
arrow/src/util/data_gen.rs	`77.57% <0.00%> (+0.46%)`	⬆️
arrow/src/datatypes/datatype.rs	`65.36% <0.00%> (-0.44%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 898924f...3845a11. Read the comment docs.

jimexist · 2021-10-31T08:38:04Z

arrow/src/datatypes/numeric.rs

@@ -333,6 +335,8 @@ make_numeric_type!(UInt8Type, u8, u8x64, m8x64);
 make_numeric_type!(UInt16Type, u16, u16x32, m16x32);
 make_numeric_type!(UInt32Type, u32, u32x16, m32x16);
 make_numeric_type!(UInt64Type, u64, u64x8, m64x8);
+#[cfg(feature = "f16")]
+make_numeric_type!(Float16Type, f16, f16x32, m16x32);


I wish there were support for this but apparently there isn't

alamb

Looks pretty cool to me. I am not sure how widely used the Float16 type is but seems like a reasonable feature to add

I think it would be good to add some basic tests for Float16Array (like, for example, creating it from an iterator) as well as an example as a doc comment).

I also was wondering if there was some way to avoid the sprinkled #[cfg(feature = "f16")] throughout the code (for example, can we possible always define Float16Array even if the f16 type is not defined, but provide some implementation that always errors when it is constructed.

Then we could have one or two #[cfg(feature = "f16")] checks rather than them throughout the code

alamb · 2021-11-03T18:49:45Z

arrow/src/json/reader.rs

@@ -1015,7 +1015,15 @@ impl Decoder {
            DataType::UInt32 => self.read_primitive_list_values::<UInt32Type>(rows),
            DataType::UInt64 => self.read_primitive_list_values::<UInt64Type>(rows),
            DataType::Float16 => {
-                return Err(ArrowError::JsonError("Float16 not supported".to_string()))
+                #[cfg(feature = "f16")]


this seems redundant -- both branches do the same thing. Was this an oversight? Or perhaps just a future TODO that will be clearer when separated like this?

Same question applies below

martin-g · 2021-11-04T09:39:20Z

arrow/src/array/data.rs

+            }
+            #[cfg(not(feature = "f16"))]
+            {
+                unimplemented!()


unimplemented!("Float16 datatype not supported") as in the other places

jimexist · 2021-11-20T16:04:15Z

Looks pretty cool to me. I am not sure how widely used the Float16 type is but seems like a reasonable feature to add

I think it would be good to add some basic tests for Float16Array (like, for example, creating it from an iterator) as well as an example as a doc comment).

I also was wondering if there was some way to avoid the sprinkled #[cfg(feature = "f16")] throughout the code (for example, can we possible always define Float16Array even if the f16 type is not defined, but provide some implementation that always errors when it is constructed.

Then we could have one or two #[cfg(feature = "f16")] checks rather than them throughout the code

can we possible always define Float16Array even if the f16 type is not defined, but provide some implementation that always errors when it is constructed.

are you saying that we should allow for runtime error rather than compile time failures?

alamb

Looks pretty cool to me -- thanks @jimexist

One major questions for other reviewers: Are we ok with adding a new dependency (on half?) -- in the IOx project, it turns out we already have half because serde_cbor depends on it, because criterion depends on it 🤷

I think some basic tests are in order -- perhaps a doctest showing how to construct an F16Array perhaps?

jimexist · 2021-11-23T00:20:22Z

Looks pretty cool to me -- thanks @jimexist

One major questions for other reviewers: Are we ok with adding a new dependency (on half?) -- in the IOx project, it turns out we already have half because serde_cbor depends on it, because criterion depends on it 🤷

I think some basic tests are in order -- perhaps a doctest showing how to construct an F16Array perhaps?

do you mean this: https://github.com/apache/arrow-rs/pull/888/files#diff-dd779d74625591eb0e2a5648a76db5b714f4135ee094afd77e87a584f9f264fbR199?

alamb · 2021-11-23T12:17:26Z

arrow/src/array/mod.rs

@@ -192,6 +192,14 @@ pub type UInt64Array = PrimitiveArray<UInt64Type>;
 ///
 /// # Example: Using `collect`
 /// ```
+/// # use arrow::array::Float16Array;
+/// use half::f16;
+/// let arr : Float16Array = [Some(f16::from_f64(1.0)), Some(f16::from_f64(2.0))].into_iter().collect();


alamb

Looks good -- thanks @jimexist

jimexist · 2021-11-23T14:01:00Z

can I possibly get another stamp on this? I guess introducing half isn't going to be obviously all upside

alamb

Sorry -- I meant to approve this earlier. I think half is ok (as it is a transitive dependency for criterion)

jimexist · 2021-11-29T13:08:37Z

@alamb with no further reviews and concerns i have merged this pull request

alamb · 2021-11-29T13:45:30Z

Nice work @jimexist 👍

github-actions bot added the arrow Changes to the arrow crate label Oct 31, 2021

jimexist force-pushed the add-support-f16 branch from d584029 to 2e89d6a Compare October 31, 2021 06:23

jimexist commented Oct 31, 2021

View reviewed changes

jimexist force-pushed the add-support-f16 branch 7 times, most recently from 4b9ff7c to daade70 Compare November 3, 2021 06:52

alamb reviewed Nov 3, 2021

View reviewed changes

martin-g reviewed Nov 4, 2021

View reviewed changes

jimexist force-pushed the add-support-f16 branch from daade70 to 90120d6 Compare November 20, 2021 16:02

jimexist force-pushed the add-support-f16 branch 2 times, most recently from bb744df to f46b312 Compare November 20, 2021 16:26

jimexist requested a review from alamb November 20, 2021 16:26

jimexist force-pushed the add-support-f16 branch 3 times, most recently from 61133b7 to 312b9c5 Compare November 21, 2021 06:50

alamb reviewed Nov 22, 2021

View reviewed changes

jimexist force-pushed the add-support-f16 branch from 312b9c5 to e51c1d9 Compare November 23, 2021 03:38

add support for f16

1dced1c

jimexist force-pushed the add-support-f16 branch from e51c1d9 to 1dced1c Compare November 23, 2021 03:42

alamb reviewed Nov 23, 2021

View reviewed changes

alamb approved these changes Nov 23, 2021

View reviewed changes

jimexist requested a review from alamb November 23, 2021 15:56

jimexist requested review from jorgecarleitao and Dandandan November 23, 2021 15:56

alamb approved these changes Nov 23, 2021

View reviewed changes

jimexist merged commit f6908bf into apache:master Nov 29, 2021

jimexist deleted the add-support-f16 branch November 29, 2021 13:08

alamb mentioned this pull request Aug 31, 2022

DataType::is_numeric should match the is_numeric function in Datafusion. #2611

Closed

anjakefala mentioned this pull request Oct 24, 2023

Add Float16/Half-float logical type to Parquet #4986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for f16 #888

add support for f16 #888

jimexist commented Oct 31, 2021 •

edited

Loading

codecov-commenter commented Oct 31, 2021 •

edited

Loading

jimexist Oct 31, 2021

alamb left a comment

alamb Nov 3, 2021

alamb Nov 3, 2021

martin-g Nov 4, 2021

jimexist commented Nov 20, 2021

alamb left a comment

jimexist commented Nov 23, 2021

alamb Nov 23, 2021

alamb left a comment

jimexist commented Nov 23, 2021

alamb left a comment •

edited

Loading

jimexist commented Nov 29, 2021

alamb commented Nov 29, 2021

add support for f16 #888

add support for f16 #888

Conversation

jimexist commented Oct 31, 2021 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Oct 31, 2021 • edited Loading

Codecov Report

jimexist Oct 31, 2021

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 3, 2021

Choose a reason for hiding this comment

alamb Nov 3, 2021

Choose a reason for hiding this comment

martin-g Nov 4, 2021

Choose a reason for hiding this comment

jimexist commented Nov 20, 2021

alamb left a comment

Choose a reason for hiding this comment

jimexist commented Nov 23, 2021

alamb Nov 23, 2021

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

jimexist commented Nov 23, 2021

alamb left a comment • edited Loading

Choose a reason for hiding this comment

jimexist commented Nov 29, 2021

alamb commented Nov 29, 2021

jimexist commented Oct 31, 2021 •

edited

Loading

codecov-commenter commented Oct 31, 2021 •

edited

Loading

alamb left a comment •

edited

Loading