Skip to content

ARROW-10301: [C++][Compute] Implement "all" reduction kernel for boolean data #8474

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
Closed
arw2019 wants to merge 4 commits into from
Closed

ARROW-10301: [C++][Compute] Implement "all" reduction kernel for boolean data #8474

arw2019 wants to merge 4 commits into from

Conversation

@arw2019
Copy link
Contributor

@arw2019 arw2019 commented Oct 15, 2020

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 15, 2020

https://issues.apache.org/jira/browse/ARROW-10301

@emkornfield
Copy link
Contributor

emkornfield commented Oct 28, 2020

@arw2019 it looks like quite a few CI failures? Is this still WIP?

@arw2019
Copy link
Contributor Author

arw2019 commented Oct 28, 2020

@arw2019 it looks like quite a few CI failures? Is this still WIP?

Failures crept in at the recent rebase (was all green before). Marking as draft while I debug & will comment when ready for review

@arw2019 arw2019 marked this pull request as draft October 28, 2020 04:43
@arw2019 arw2019 marked this pull request as ready for review November 2, 2020 18:30
@arw2019
Copy link
Contributor Author

arw2019 commented Nov 2, 2020 •
edited
Loading

Ready for re-review. CI all green

@arw2019 arw2019 changed the title ARROW-10301: [C++] Implement "all" reduction kernel for boolean data ARROW-10301: [C++][Compute] Implement "all" reduction kernel for boolean data Nov 7, 2020
@arw2019 arw2019 force-pushed the ARROW-10301 branch 2 times, most recently from e365c34 to 0020eb2 Compare November 11, 2020 22:49
@arw2019 arw2019 marked this pull request as draft November 11, 2020 23:16
@arw2019 arw2019 marked this pull request as ready for review November 13, 2020 00:59
jorisvandenbossche
jorisvandenbossche reviewed Nov 13, 2020
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Looks good to me (although not a C++ kernel expert ;)), but the docs / tests looks good.

Since this is similar to #8294, and most review on the code happened there, does it makes sense to get that PR merged first?

jorisvandenbossche
jorisvandenbossche reviewed Nov 13, 2020
View reviewed changes
cpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 13, 2020

Choose a reason for hiding this comment

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

Just wondering but should it in theory not be a "AndNot" instead of "OrNot" ?

Copy link
Contributor Author

@arw2019 arw2019 Nov 14, 2020

Choose a reason for hiding this comment

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

I named it after the bit operation we're using, which in this case is

arrow/cpp/src/arrow/util/bit_block_counter.h

Lines 196 to 197 in 7efb373

/// \brief Computes "x | ~y" block for each available run of bits.
BitBlockCount NextOrNotWord();

@arw2019
Copy link
Contributor Author

arw2019 commented Nov 14, 2020

Since this is similar to #8294, and most review on the code happened there, does it makes sense to get that PR merged first?

Yes, agreed - since this follows the pattern in #8294 very closely

@pitrou
Copy link
Member

pitrou commented Nov 24, 2020

@arw2019 Do you want to revisit this now that the "any" kernel has been merged?

arw2019
arw2019 commented Nov 24, 2020
View reviewed changes
cpp/src/arrow/util/bit_block_counter.h Outdated
Copy link
Contributor Author

@arw2019 arw2019 Nov 24, 2020

Choose a reason for hiding this comment

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

I'm duplicating code here. NextOrBlock is identical to

arrow/cpp/src/arrow/util/bit_block_counter.h

Lines 223 to 231 in bd10727

BitBlockCount NextAndBlock() {
static constexpr int64_t kMaxBlockSize = std::numeric_limits<int16_t>::max();
switch (has_bitmap_) {
case HasBitmap::BOTH: {
BitBlockCount block = binary_counter_.NextAndWord();
position_ += block.length;
return block;
}
case HasBitmap::ONE: {

except for case HasBitmap::BOTH: where NextOrNotWord is called. Is there a good way to avoid the repetition?

Copy link
Member

@pitrou pitrou Nov 25, 2020

Choose a reason for hiding this comment

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

I think it's ok for now, we can revisit later if desired.

@arw2019
Copy link
Contributor Author

arw2019 commented Nov 24, 2020

@pitrou rebased + green so ready for re-review.

xref #8474 (comment) is a question about OptionalBinaryBitBlockCounter

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #8474 (bd10727) into master (42564e4) will decrease coverage by 0.21%.
The diff coverage is 40.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8474      +/-   ##
==========================================
- Coverage   84.40%   84.18%   -0.22%     
==========================================
  Files         186      186              
  Lines       45241    45445     +204     
==========================================
+ Hits        38184    38260      +76     
- Misses       7057     7185     +128
Impacted Files Coverage Δ
rust/benchmarks/src/bin/tpch.rs 0.00% <0.00%> (ø)
rust/datafusion/src/physical_plan/expressions.rs 83.42% <ø> (ø)
rust/datafusion/src/physical_plan/planner.rs 71.81% <0.00%> (-5.64%) ⬇️
rust/datafusion/src/optimizer/utils.rs 58.89% <4.83%> (-14.90%) ⬇️
rust/datafusion/src/logical_plan/expr.rs 70.68% <48.78%> (-8.27%) ⬇️
rust/arrow/src/compute/kernels/comparison.rs 96.27% <96.42%> (+0.49%) ⬆️
rust/datafusion/src/optimizer/filter_push_down.rs 99.11% <100.00%> (+<0.01%) ⬆️
...t/datafusion/src/optimizer/projection_push_down.rs 87.84% <100.00%> (-0.15%) ⬇️
rust/parquet/src/encodings/encoding.rs 92.99% <0.00%> (-0.18%) ⬇️
rust/arrow/src/array/transform/mod.rs 70.61% <0.00%> (+1.52%) ⬆️

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 b699541...5ef9cff. Read the comment docs.

pitrou
pitrou approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1. Rebased, will merge.

@pitrou pitrou closed this in db20c7a Nov 25, 2020
@arw2019
Copy link
Contributor Author

arw2019 commented Nov 25, 2020

Thanks @pitrou @jorisvandenbossche for reviewing!!!

@arw2019 arw2019 deleted the ARROW-10301 branch November 25, 2020 23:09
@asfimport asfimport mentioned this pull request May 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

@pitrou pitrou pitrou 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.

5 participants

@arw2019 @emkornfield @pitrou @codecov-io @jorisvandenbossche