Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-4038: [Rust] Implement boolean AND, OR, NOT array ops #3189

Closed
wants to merge 4 commits into from

Conversation

andygrove
Copy link
Member

  • Implements boolean AND, OR, NOT operations in array_ops
  • Removes all uses of unwrap() in array_ops and replaces with ?
  • Improve error messages

@andygrove andygrove changed the title Implement boolean AND, OR, NOT array ops ARROR-4038: [Rust] Implement boolean AND, OR, NOT array ops Dec 17, 2018
@andygrove
Copy link
Member Author

@paddyhoran @sunchao Could I get a review please. Rust tests are passing but Python tests failed with a timeout.

Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andygrove . It seems there is no well-established rules to define what's the result should be when one of the operand is null. Maybe we should add document for each public methods and clearly describe the behavior. This can be in a different PR of course.

Another minor thing is, in operators such as gt_eq, we define:

    bool_op(left, right, |a, b| match (a, b) {
        (None, _) => false,
        (_, None) => true,
        (Some(aa), Some(bb)) => aa >= bb,
    })

However this means None and None is also mapped to false, is that right? This perhaps is unexpected.

@codecov-io
Copy link

Codecov Report

Merging #3189 into master will increase coverage by 1.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3189      +/-   ##
==========================================
+ Coverage   86.41%   87.56%   +1.14%     
==========================================
  Files         504      450      -54     
  Lines       69587    66065    -3522     
==========================================
- Hits        60137    57853    -2284     
+ Misses       9353     8212    -1141     
+ Partials       97        0      -97
Impacted Files Coverage Δ
cpp/src/arrow/compute/compute-test.cc 93.1% <0%> (-6.34%) ⬇️
python/pyarrow/feather.py 86.95% <0%> (-2.46%) ⬇️
cpp/src/arrow/python/helpers.cc 77.54% <0%> (-2.12%) ⬇️
python/pyarrow/tests/test_feather.py 96.7% <0%> (-1.16%) ⬇️
python/pyarrow/tests/conftest.py 73.91% <0%> (-0.78%) ⬇️
cpp/src/arrow/compute/kernels/cast.cc 91.47% <0%> (-0.4%) ⬇️
python/pyarrow/tests/test_convert_pandas.py 95.01% <0%> (-0.12%) ⬇️
cpp/src/parquet/arrow/reader.cc 84.72% <0%> (-0.11%) ⬇️
cpp/src/parquet/arrow/arrow-reader-writer-test.cc 95.92% <0%> (-0.04%) ⬇️
cpp/src/arrow/io/hdfs.cc 0.31% <0%> (-0.02%) ⬇️
... and 104 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 0936938...69518d7. Read the comment docs.

@andygrove
Copy link
Member Author

Thanks @sunchao I addressed your feedback. Good points.

Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @andygrove .

@andygrove andygrove changed the title ARROR-4038: [Rust] Implement boolean AND, OR, NOT array ops ARROW-4038: [Rust] Implement boolean AND, OR, NOT array ops Dec 20, 2018
@andygrove andygrove closed this in 1a8c8f0 Dec 20, 2018
romainfrancois pushed a commit to romainfrancois/arrow that referenced this pull request Jan 3, 2019
- Implements boolean AND, OR, NOT operations in `array_ops`
- Removes all uses of `unwrap()` in array_ops and replaces with `?`
- Improve error messages

Author: Andy Grove <andygrove73@gmail.com>

Closes apache#3189 from andygrove/ARROW-4038 and squashes the following commits:

69518d7 <Andy Grove> add tests
a38d9a9 <Andy Grove> add docs for all array_ops and add explicit handling for case where both sides are null
661e2af <Andy Grove> improve error message
36b9171 <Andy Grove> Implement boolean AND, OR, NOT operations, remove unwraps and improve error message
@andygrove andygrove deleted the ARROW-4038 branch March 30, 2019 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants