ARROW-13136: [C++] Add coalesce function

[lidavidm]

No description provided.

[github-actions]

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

[lidavidm]

Hmm, this has some Valgrind errors - taking a look.

[lidavidm]

@github-actions crossbow submit conda-cpp-valgrind

[lidavidm]

@github-actions crossbow submit test-conda-cpp-valgrind

[github-actions]

Revision: 08c63756efc1d4d950b4d45a6b0cd6feb30cdd60

Submitted crossbow builds: ursacomputing/crossbow @ actions-560

Task	Status
test-conda-cpp-valgrind

[lidavidm]

@bkietz do you have time to review this? Do we want to add a benchmark here?

[bkietz]

Thanks for doing this!

Yes, we definitely want a benchmark here. A few other comments:

[bkietz]

I think we should always be able to preallocate the validity bitmap in addition to the data/offsets buffer, which will enable the preallocate_contiguous_ optimization for fixed width types.

Suggested change

	kernel.null_handling = NullHandling::COMPUTED_NO_PREALLOCATE;
	kernel.mem_allocation = MemAllocation::PREALLOCATE;
	kernel.null_handling = NullHandling::COMPUTED_PREALLOCATE;
	kernel.mem_allocation = MemAllocation::PREALLOCATE;
	if (var width type) {
	kernel.can_write_into_slices = false;
	}

[bkietz]

Suggested change

	std::shared_ptr<Array> temp_output;
	RETURN_NOT_OK(builder.Finish(&temp_output));
	ARROW_ASSIGN_OR_RAISE(auto temp_output, builder.Finish());

[lidavidm]

Performance is fairly meh. perf shows a decent amount of time spent in CopyValues/CopyOneValue just re-examining the Datum variant, calling Buffer::data(), etc.

-------------------------------------------------------------------------------------
Benchmark Time CPU Iterations UserCounters...
-------------------------------------------------------------------------------------
CoalesceBench64/1048576/0 19254992 ns 19255104 ns 37 bytes_per_second=1.62295G/s
CoalesceBench64/1048576/99 19281752 ns 19281409 ns 37 bytes_per_second=1.62058G/s

Trying an approach based on VisitSetBitRunsVoid may be beneficial, and/or manually hoisting the scalar-vs-array detection and having separate CopyScalarValues and CopyArrayValues.

[lidavidm]

Alright, this buys us ~50% more performance by specializing the common case

--------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations UserCounters...
--------------------------------------------------------------------------------------------
CoalesceBench64/1048576/0 13335778 ns 13335766 ns 52 bytes_per_second=2.34332G/s
CoalesceBench64/1048576/99 13562011 ns 13561826 ns 51 bytes_per_second=2.30404G/s
CoalesceNonNullBench64/1048576/0 13552482 ns 13552397 ns 48 bytes_per_second=2.30587G/s
CoalesceNonNullBench64/1048576/99 13572235 ns 13572002 ns 51 bytes_per_second=2.30232G/s

[bkietz]

Trying an approach based on VisitSetBitRunsVoid

IIUC this would require a varargs version of OptionalBitBlockCounter or Bitmap::VisitWords, which would probably be generally useful as varargs compute functions continue to proliferate

[lidavidm]

Basically, there was a lot of overhead from the fallback loop of "for offset in range(block size), if bit is set, copy one element" because 1) the 'copy one element' function used CopyBitmap which has a ton of overhead for copying one bit and 2) unboxing the array every time was costly when done in a loop like that (e.g. the profiler showed that even Buffer::data()'s check for whether the buffer is on-CPU was hot). But now I've specialized things to avoid most of that overhead.

The reason why I wanted something like VisitSetBitRunsVoid was to go a step further and always try to perform block copies instead of falling back to one-element-at-a-time-copies. But yes, then it needs to be able to combine two bitmaps with AndNot (we want runs of bits where !output_valid & input_valid)

[lidavidm]

I would kind of prefer to get all these kernels merged and consolidated before I start trying to microoptimize them, though, given they've been around for a while and all use similar helper code (that's now starting to diverge slightly once I look at optimizing).

[lidavidm]

@ursabot please benchmark lang=C++

[ursabot]

Benchmark runs are scheduled for baseline = 9c6d417 and contender = e32cf48c8f5f38ed5bbf69eb5d2ea8eda43d2b98. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Provided benchmark filters do not have any benchmark groups to be executed on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2 (mimalloc)
[Skipped ⚠️ Only ['Python', 'R'] langs are supported on ursa-i9-9960x] ursa-i9-9960x (mimalloc)
[Finished ⬇️0.53% ⬆️0.05%] ursa-thinkcentre-m75q (mimalloc)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

[bkietz]

This LGTM, but could you rewrite a bit for readability?

[bkietz]

Could you de-nest some of this branching by extracting some functions and intersperse some whitespace and comments? This is a little difficult to read. Something like:

Suggested change

	if ((datum.is_scalar() && datum.scalar()->is_valid) ||
	(datum.is_array() && !datum.array()->MayHaveNulls())) {
	BitBlockCounter counter(out_valid, out_offset, batch.length);
	int64_t offset = 0;
	while (offset < batch.length) {
	constauto block = counter.NextWord();
	if (block.NoneSet()) {
	CopyValues<Type>(datum, offset, block.length, out_valid, out_values,
	out_offset + offset);
	} elseif (!block.AllSet()) {
	for (int64_t j = 0; j < block.length; ++j) {
	if (!BitUtil::GetBit(out_valid, out_offset + offset + j)) {
	CopyValues<Type>(datum, offset + j, 1, out_valid, out_values,
	out_offset + offset + j);
	}
	}
	}
	offset += block.length;
	}
	break;
	} elseif (datum.is_array()) {
	if ((datum.is_scalar() && datum.scalar()->is_valid) ||
	(datum.is_array() && !datum.array()->MayHaveNulls())) {
	// all-valid scalar or array
	CopyValuesAllValid<Type>(datum, batch.length, out_valid, out_values, out_offset);
	break;
	}
	// null scalar; skip
	if (datum.is_scalar()) continue;

[lidavidm]

Broke up the main function a bit.

[lidavidm]

Rebased (wow, that was more painful than I wanted it to be)

[lidavidm]

Rebased again to fix the conflict with the make_struct change.

[bkietz]

LGTM, thanks!