[GitHub] [arrow-rs] alamb commented on a diff in pull request #2810: Implement ArrowNumericType for Float16Type

[GitBox <gi...@apache.org>]

alamb commented on code in PR #2810:
URL: https://github.com/apache/arrow-rs/pull/2810#discussion_r985789922


##########
arrow/src/compute/kernels/comparison.rs:
##########
@@ -1938,7 +1938,7 @@ where
     let simd_right = T::init(right);
 
     // safety: result is newly created above, always written as a T below

Review Comment:
   we probably don't need the `//safety` comments anymore as we are using a `safe` API now



##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -333,8 +333,7 @@ where
             // process data in chunks of 64 elements since we also get 64 bits of validity information at a time
 
             // safety: result is newly created above, always written as a T below
-            let mut result_chunks =
-                unsafe { result.typed_data_mut().chunks_exact_mut(64) };
+            let mut result_chunks = result.typed_data_mut().chunks_exact_mut(64);

Review Comment:
   👍  -- as below we probably can remove the old `//safety` comments too



##########
arrow/src/datatypes/numeric.rs:
##########
@@ -366,6 +366,102 @@ make_numeric_type!(DurationMillisecondType, i64, i64x8, m64x8);
 make_numeric_type!(DurationMicrosecondType, i64, i64x8, m64x8);
 make_numeric_type!(DurationNanosecondType, i64, i64x8, m64x8);
 
+#[cfg(not(feature = "simd"))]
+impl ArrowNumericType for Float16Type {}
+
+#[cfg(feature = "simd")]
+impl ArrowNumericType for Float16Type {

Review Comment:
   Perhaps an expert in SIMD / GPU could give this a look as well (no need to hold merging up) -- I though f16 was primarily useful when doing SIMD/GPU so perhaps there some special support here we could use
   
   In any event, this seems like a good step forward to me 👍 
   
   Perhaps @HaoYang670 or @Jimexist  (who originally contributed `f16` support in https://github.com/apache/arrow-rs/issues/890) might have some ideas



##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -2898,4 +2897,26 @@ mod tests {
         let division_by_zero = divide_scalar_opt_dyn::<Int32Type>(&a, 0);
         assert_eq!(&expected, &division_by_zero.unwrap());
     }
+
+    #[test]
+    fn test_sum_f16() {
+        let a = Float16Array::from_iter_values([
+            f16::from_f32(0.1),
+            f16::from_f32(0.2),
+            f16::from_f32(1.5),
+            f16::from_f32(-0.1),
+        ]);
+        let b = Float16Array::from_iter_values([
+            f16::from_f32(5.1),
+            f16::from_f32(6.2),
+            f16::from_f32(-1.),
+            f16::from_f32(-2.1),
+        ]);
+        let expected = Float16Array::from_iter_values(
+            a.values().iter().zip(b.values()).map(|(a, b)| a + b),
+        );
+
+        let c = add(&a, &b).unwrap();

Review Comment:
   ❤️ 



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