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.

Sign up for GitHub

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

Jump to bottom

Add (simd) modulus op #317

Merged
merged 4 commits into from
Jun 4, 2021
Merged

Add (simd) modulus op #317

merged 4 commits into from
Jun 4, 2021

Conversation

gangliao
Copy link
Contributor

Which issue does this PR close?

#98
apache/datafusion#99

Rationale for this change

What changes are included in this PR?

support modulus operations.

Are there any user-facing changes?

no.

@gangliao
Copy link
Contributor Author

After this PR is merged, I will submit a new PR to arrow-datafusion in order to support the modulus clause in SQL.

@gangliao
Copy link
Contributor Author

@jorgecarleitao First-time contributors need a maintainer to approve running workflows.

@gangliao gangliao mentioned this pull request May 18, 2021
Closed
nevi-me
nevi-me reviewed May 19, 2021
View reviewed changes
arrow/src/compute/kernels/arithmetic.rs
let null_bit_buffer =
combine_option_bitmap(left.data_ref(), right.data_ref(), left.len())?;

let buffer = if let Some(b) = &null_bit_buffer {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to check if the null count > 0, as there could be a buffer even though all values are non-null

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2021 •
edited
Loading

Codecov Report

Merging #317 (2d55a0d) into master (c863a2c) will increase coverage by 0.06%.
The diff coverage is 78.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   82.52%   82.59%   +0.06%     
==========================================
  Files         162      162              
  Lines       44007    44344     +337     
==========================================
+ Hits        36316    36624     +308     
- Misses       7691     7720      +29
Impacted Files Coverage Δ
arrow/src/compute/kernels/arithmetic.rs 84.14% <78.69%> (-3.23%) ⬇️
parquet/src/encodings/decoding.rs 92.47% <0.00%> (-0.02%) ⬇️
arrow/src/error.rs 8.88% <0.00%> (ø)
arrow/src/csv/writer.rs 82.64% <0.00%> (ø)
parquet/src/column/writer.rs 93.29% <0.00%> (ø)
arrow/src/array/raw_pointer.rs 100.00% <0.00%> (ø)
arrow/src/array/array_string.rs 96.05% <0.00%> (ø)
arrow/src/util/string_writer.rs 73.33% <0.00%> (ø)
arrow/src/array/array_boolean.rs 90.90% <0.00%> (ø)
arrow/src/array/array_primitive.rs 92.93% <0.00%> (ø)
... and 13 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 c863a2c...2d55a0d. Read the comment docs.

jorgecarleitao
jorgecarleitao reviewed May 25, 2021
View reviewed changes
arrow/src/error.rs Outdated Show resolved Hide resolved
alamb
alamb approved these changes May 29, 2021
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This looks great to me @gangliao -- I am sorry I did not have a chance to review this carefully previously.

I reviewed all test cases carefully and they look good. Very impressive work 👍

arrow/src/compute/kernels/arithmetic.rs
let a = Int32Array::from(vec![15, 15, 8, 1, 9]);
let b = Int32Array::from(vec![5, 6, 8, 9, 1]);
let c = modulus(&a, &b).unwrap();
assert_eq!(0, c.value(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to compare the c in fewer lines using something like assert_eq!(c, Int32Array:from(vec![3. 0, 2, 3, 4]), which also has the advantage if ensuring that c.len() == 5. This way is fine too

arrow/src/compute/kernels/arithmetic.rs Show resolved Hide resolved
@gangliao
Copy link
Contributor Author

gangliao commented Jun 4, 2021

Can anyone merge this PR so that I can proceed with other PR in arrow-datafusion? @alamb @jorgecarleitao

@alamb alamb merged commit 0a16574 into apache:master Jun 4, 2021
@alamb
Copy link
Contributor

alamb commented Jun 4, 2021

Sorry @gangliao !

alamb pushed a commit that referenced this pull request Jun 4, 2021
@gangliao @alamb
Add (simd) modulus op (#317)
2b172d4 
* Add (simd) modulus op

* fix typo

* fix feature = "simd"

* revert ModulusByZero
@alamb alamb mentioned this pull request Jun 4, 2021
Merged
alamb added a commit that referenced this pull request Jun 5, 2021
@alamb @gangliao
Add (simd) modulus op (#317) (#410)
8059cf5 
* Add (simd) modulus op

* fix typo

* fix feature = "simd"

* revert ModulusByZero

Co-authored-by: Gang Liao <gangliao@umd.edu>
@gangliao gangliao mentioned this pull request Jun 5, 2021
Closed
@alamb alamb mentioned this pull request Jun 10, 2021
Merged
@gangliao gangliao mentioned this pull request Jun 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roee88 roee88 roee88 left review comments

@nevi-me nevi-me nevi-me left review comments

@jorgecarleitao jorgecarleitao jorgecarleitao left review comments

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gangliao @codecov-commenter @alamb @roee88 @nevi-me @jorgecarleitao