ARROW-10301: [C++][Compute] Implement "all" reduction kernel for boolean data by arw2019 · Pull Request #8474 · apache/arrow

arw2019 · 2020-10-15T19:38:31Z

No description provided.

github-actions · 2020-10-15T19:49:58Z

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

emkornfield · 2020-10-28T03:22:59Z

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

arw2019 · 2020-10-28T04:43:02Z

@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 · 2020-11-02T18:41:28Z

Ready for re-review. CI all green

jorisvandenbossche

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 · 2020-11-13T13:35:54Z

cpp/src/arrow/compute/kernels/aggregate_basic.cc

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

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 · 2020-11-14T02:53:35Z

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 · 2020-11-24T17:48:10Z

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

arw2019 · 2020-11-24T22:06:18Z

cpp/src/arrow/util/bit_block_counter.h

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?

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

arw2019 · 2020-11-24T22:12:44Z

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

xref #8474 (comment) is a question about OptionalBinaryBitBlockCounter

codecov-io · 2020-11-25T00:52:57Z

Codecov Report

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

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

+1. Rebased, will merge.

arw2019 · 2020-11-25T23:09:20Z

Thanks @pitrou @jorisvandenbossche for reviewing!!!

kszucs force-pushed the master branch from 953009f to 04660f8 Compare October 19, 2020 18:00

kszucs force-pushed the ARROW-10301 branch from f368cea to e5143f2 Compare October 19, 2020 22:32

arw2019 force-pushed the ARROW-10301 branch from e5143f2 to 41e2a7c Compare October 26, 2020 04:19

arw2019 marked this pull request as draft October 28, 2020 04:43

arw2019 force-pushed the ARROW-10301 branch from 41e2a7c to 16a8926 Compare November 2, 2020 16:34

arw2019 marked this pull request as ready for review November 2, 2020 18:30

arw2019 force-pushed the ARROW-10301 branch from 4dfe5dc to 18271d5 Compare November 7, 2020 04:22

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 force-pushed the ARROW-10301 branch 2 times, most recently from e365c34 to 0020eb2 Compare November 11, 2020 22:49

arw2019 marked this pull request as draft November 11, 2020 23:16

arw2019 force-pushed the ARROW-10301 branch from 7862216 to e6d7627 Compare November 12, 2020 20:17

arw2019 marked this pull request as ready for review November 13, 2020 00:59

jorisvandenbossche reviewed Nov 13, 2020

View reviewed changes

arw2019 force-pushed the ARROW-10301 branch from 68bc3ef to 5ef9cff Compare November 24, 2020 19:07

arw2019 commented Nov 24, 2020

View reviewed changes

arw2019 and others added 4 commits November 25, 2020 09:48

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

a9f814d

resolve conflict

566c429

linting

5525d44

Nits

3acdf1c

pitrou force-pushed the ARROW-10301 branch from 5ef9cff to 3acdf1c Compare November 25, 2020 08:51

pitrou approved these changes Nov 25, 2020

View reviewed changes

pitrou closed this in db20c7a Nov 25, 2020

arw2019 deleted the ARROW-10301 branch November 25, 2020 23:09

asfimport mentioned this pull request May 25, 2021

[C++] Add "all" boolean reducing kernel #26292

Closed

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

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

Conversation

arw2019 commented Oct 15, 2020

Uh oh!

github-actions bot commented Oct 15, 2020

Uh oh!

emkornfield commented Oct 28, 2020

Uh oh!

arw2019 commented Oct 28, 2020

Uh oh!

arw2019 commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

arw2019 Nov 14, 2020

Choose a reason for hiding this comment

Uh oh!

arw2019 commented Nov 14, 2020

Uh oh!

pitrou commented Nov 24, 2020

Uh oh!

arw2019 Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

pitrou Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

arw2019 commented Nov 24, 2020

Uh oh!

codecov-io commented Nov 25, 2020

Codecov Report

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

arw2019 commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arw2019 commented Nov 2, 2020 •

edited

Loading