ARROW-3880: [Rust] Implement simple math operations for numeric arrays by andygrove · Pull Request #3033 · apache/arrow

andygrove · 2018-11-26T13:45:58Z

No description provided.

codecov-io · 2018-11-26T14:25:53Z

Codecov Report

Merging #3033 into master will increase coverage by 3.43%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3033      +/-   ##
==========================================
+ Coverage      87%   90.43%   +3.43%     
==========================================
  Files         496       13     -483     
  Lines       70563     2092   -68471     
==========================================
- Hits        61393     1892   -59501     
+ Misses       9069      200    -8869     
+ Partials      101        0     -101

Impacted Files	Coverage Δ
rust/src/lib.rs	`100% <ø> (ø)`	⬆️
rust/src/array.rs	`90.97% <100%> (+1.25%)`	⬆️
rust/src/tensor.rs	`93.11% <100%> (ø)`	⬆️
rust/src/csv/reader.rs	`87.5% <100%> (+0.12%)`	⬆️
rust/src/error.rs	`75% <100%> (+8.33%)`	⬆️
rust/src/buffer.rs	`93.53% <100%> (+0.02%)`	⬆️
python/pyarrow/ipc.pxi
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/plasma/client.cc
... and 479 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 427a219...06bbc4a. Read the comment docs.

andygrove · 2018-11-27T14:39:38Z

@paddyhoran @sunchao @kszucs Looking for a review please

paddyhoran · 2018-11-27T16:11:19Z

+            }
+
+            pub fn divide(&self, other: &PrimitiveArray<$native_ty>) -> PrimitiveArray<$native_ty> {
+                self.math_helper(other, |a, b| a / b)


Should we check if b is zero and push a null if it is?

I believe divide by zero should be an error, as it would be usually

However, we don't want a panic ... so probably divide should return a Result<> and should check first for any 0 values before attempting the operation

Yea, maybe returning a Result is more appropriate.

This is tricky ... the code is generic and using macros which makes it hard to check for literal zero ... I'm going to have to think about this some more. I probably have to refactor this a bit.

We could use something like this.

In ARROW-3878 @sunchao is introducing the following:
pub trait ArrowNumericType: ArrowPrimitiveType {}

We could make this:
pub trait ArrowNumericType: ArrowPrimitiveType + Zero {} and you could make the above generic over ArrowNumericType.

I think it's fine to merge as is and open a JIRA for the follow up work, as once ARROW-3878 is merged the solution will change.

Thanks @paddyhoran ... that num crate made things much nicer and now we no longer panic on divide by zero. Glad to see ArrowNumericType coming along ... I actually tried to implement that myself this week and failed.

sunchao · 2018-11-27T22:35:04Z

+                self.math_helper(other, |a, b| a - b)
+            }
+
+            pub fn multiply(


does this work for boolean types as well?

No, this is only implemented for the numeric types (as with the existing min and max methods).

sunchao · 2018-11-27T22:36:58Z

+                    if self.is_null(i) || other.is_null(i) {
+                        b.push_null().unwrap();
+                    } else {
+                        b.push(op(self.value(index), other.value(index))).unwrap();


should we check length: what if other.len < self.len?

Good point. I've added a check for that.

andygrove · 2018-12-05T02:52:56Z

    }

+    //TODO: need help here ... 
+


@paddyhoran @sunchao I wonder if you can help me figure this out. Since the change to use specialization, I don't know how to make this work for numeric types but not boolean types. I'll keep trying but I feel I am missing something.

andygrove · 2018-12-05T04:30:21Z

Well, I made it work using macros .... probably not ideal, but gives me the functionality I want.

sunchao · 2018-12-05T05:15:46Z

    }
 }

+macro_rules! def_numeric_math_ops {


Instead of macro, you can do:

impl<T: ArrowNumericType> PrimitiveArray<T> where T::Native: Add<Output = T::Native> + Sub<Output = T::Native> + Mul<Output = T::Native> + Div<Output = T::Native> + Zero, { // actual impl }

sunchao · 2018-12-05T05:17:46Z

 // specific language governing permissions and limitations
 // under the License.

+use num::Zero;


nit: can you move this line below std and above the use array_data::{ArrayData, ArrayDataRef};?

sunchao · 2018-12-05T05:18:51Z

+macro_rules! def_numeric_math_ops {
+    ( $ty:ident, $native_ty:ident ) => {
+        impl PrimitiveArray<$ty> {
+            fn math_op<F>(self, other: &PrimitiveArray<$ty>, op: F) -> Result<PrimitiveArray<$ty>>


I'm thinking whether we should expose some basic functions on primitive array, such as map, fold, etc. The math_op can be generalized to accept any function that takes two arrays. In functional world, it can be achieved via zip plus map, but I'm not sure how to implement zip on multiple primitive arrays..

andygrove · 2018-12-05T15:07:25Z

@sunchao Thanks for the help, I re-implemented using generics. Should be good now. I agree with supporting zip and map and will experiment with that and create a separate PR if I figure it out.

sunchao

Thanks @andygrove . Code-wise this looks good to me. However I don't have enough knowledge to decide whether this will be good addition to the core array functionalities. Seems other languages are not doing this. Another option could be to make a math module and implement these there? seems these functions are not accessing array internals.

Maybe @wesm , @xhochy , @kszucs can help reviewing this too?

sunchao · 2018-12-06T21:30:49Z

+        })
+    }
+
+    pub fn lt_eq(self, other: &PrimitiveArray<T>) -> Result<PrimitiveArray<BooleanType>> {


nit: could we use BooleanArray instead of PrimitiveArray<BooleanType>? same below.

sunchao · 2018-12-06T21:31:10Z

+                "Cannot perform math operation on two batches of different length".to_string(),
+            ));
+        }
+        let mut b = PrimitiveArrayBuilder::<BooleanType>::new(self.len());


nit: could we use BooleanArrayBuilder instead of PrimitiveArrayBuilder::<BooleanType>?

andygrove · 2018-12-07T12:34:22Z

@sunchao Maybe you are right .. I have started a discussion on the mailing list about this.

andygrove · 2018-12-10T13:43:33Z

@sunchao I moved the math and comparison operators out of array and into a new array_ops source file.

sunchao

LGTM. Thanks @andygrove !

paddyhoran · 2018-12-11T01:23:54Z

+1 LGTM, thanks @andygrove

wesm · 2018-12-11T01:37:58Z

Needs rebase

andygrove added 2 commits November 26, 2018 06:45

Implement simple math operations for numeric arrays

12dc05b

cargo fmt

dcad28a

paddyhoran reviewed Nov 27, 2018

View reviewed changes

sunchao reviewed Nov 27, 2018

View reviewed changes

Improve error handling

1ea98cf

paddyhoran approved these changes Nov 28, 2018

View reviewed changes

andygrove added 4 commits November 27, 2018 20:18

improve handling of divide by zero, format for rust nightly

06bbc4a

merge from master

2a43b3f

merge from master, comment out new methods

a2b87e2

save work

5876fb7

andygrove commented Dec 5, 2018

View reviewed changes

re-implement using macros

405c63e

fix formatting

729cd9a

sunchao reviewed Dec 5, 2018

View reviewed changes

andygrove added 2 commits December 5, 2018 07:32

Merge branch 'master' into ARROW-3880

963def6

re-implement with generics

42c68af

add comparison operations

5ed5f6e

sunchao reviewed Dec 6, 2018

View reviewed changes

andygrove added 5 commits December 9, 2018 19:47

merge from master and fix conflicts

adfe4b0

Address PR feedback

7946142

Move math ops into new array_ops source file

7a5975e

move comparison ops to array_ops

f8bfb41

code cleanup

0c77c61

Move min and max to array_ops

afb3518

sunchao approved these changes Dec 10, 2018

View reviewed changes

paddyhoran approved these changes Dec 11, 2018

View reviewed changes

merge from master

17cd418

andygrove closed this in 2428945 Dec 11, 2018

andygrove deleted the ARROW-3880 branch March 30, 2019 22:33

andygrove restored the ARROW-3880 branch March 30, 2019 22:34

andygrove deleted the ARROW-3880 branch March 30, 2019 22:34

asfimport mentioned this pull request Dec 12, 2018

[Rust] PrimitiveArray<T> should support simple math operations #15884

Closed

Conversation

andygrove commented Nov 26, 2018

Uh oh!

codecov-io commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andygrove commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Dec 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Dec 5, 2018

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Dec 7, 2018

Uh oh!

andygrove commented Dec 10, 2018

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

paddyhoran commented Dec 11, 2018

Uh oh!

wesm commented Dec 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Nov 26, 2018 •

edited

Loading