GH-41016: [C++] Fix null count check in BooleanArray.true_count() #41070

jorisvandenbossche · 2024-04-08T12:52:02Z

Rationale for this change

Loading the null_count attribute doesn't take into account the possible value of -1, leading to a code path where the validity buffer is accessed, but which is not necessarily present in that case.

What changes are included in this PR?

Use data->MayHaveNulls() instead of data->null_count.load()

Are these changes tested?

Yes

GitHub Issue: [C++] BooleanArray true_count crashing in case of unknown null count (null_count = -1) without validity buffer #41016

github-actions · 2024-04-08T12:52:29Z

⚠️ GitHub issue #41016 has been automatically assigned in GitHub to PR creator.

kou

+1

cpp/src/arrow/array/array_test.cc

mapleFU · 2024-04-09T05:52:20Z

cpp/src/arrow/array/array_primitive.cc

@@ -56,7 +56,7 @@ int64_t BooleanArray::false_count() const {
 }

 int64_t BooleanArray::true_count() const {
-  if (data_->null_count.load() != 0) {
+  if (data_->GetNullCount() != 0) {


so the problem is that validity buffer might not exists, which might cause segment fault here?

Yes (in general, I think we should never use ArrayData.null_count directly, unless in some low level code that deals directly with that, like the implementation of GetNullCount)

Will approve after #41070 (comment) fixed

By the way, should this be in 16.0?

jorisvandenbossche · 2024-04-09T12:56:59Z

By the way, should this be in 16.0?

Given it fixes a segfault, might be good to tag as 16.0 yes.

felipecrv · 2024-04-10T01:30:52Z

cpp/src/arrow/array/array_primitive.cc

-  if (data_->null_count.load() != 0) {
+  if (data_->GetNullCount() != 0) {


Use data_->MayHaveNulls() instead. It will do the right thing without triggering the bitmap scan -- that would be extra work that doesn't even speed-up the rest of this function (except for maybe warming up the cache).

https://github.com/apache/arrow/blob/main/cpp/src/arrow/array/data.h#L287-L291

Aha, so it's because a boolean check always faster than count?

Thanks @felipecrv!

Aha, so it's because a boolean check always faster than count?

@mapleFU GetNullCount might scan the entire bitmap if null_count == kUnknownNullCount. It's O(length) while MayHaveNulls() is a O(1) check.

Yeah that I mean it doesn't need to get the exactly count here, just quick check whether it "MayHasNull"

cpp/src/arrow/array/array_test.cc

pitrou · 2024-04-10T16:19:19Z

cpp/src/arrow/array/array_test.cc

+  // GH-41016 true_count() with array without validity buffer with null_count of -1
+  auto data = ArrayFromJSON(boolean(), "[true, false, true]")->data();
+  data->null_count = -1;
+  auto arr_unknown_null_count = std::make_shared<BooleanArray>(data);


Not sure why we're redoing this after ArrayFromJSON already returned an array. Can't you just reuse the original array?

Ah, yes of course. For some reason I had in my head I needed to recreate an array object with the changed null count, but I can of course just update the null_count of the the array inplace. Updated.

pitrou · 2024-04-15T14:58:27Z

@github-actions crossbow submit -g cpp

github-actions · 2024-04-15T15:01:04Z

Revision: ffb346d

Submitted crossbow builds: ursacomputing/crossbow @ actions-fab48da664

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

…1070) ### Rationale for this change Loading the `null_count` attribute doesn't take into account the possible value of -1, leading to a code path where the validity buffer is accessed, but which is not necessarily present in that case. ### What changes are included in this PR? Use `data->MayHaveNulls()` instead of `data->null_count.load()` ### Are these changes tested? Yes * GitHub Issue: #41016 Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

conbench-apache-arrow · 2024-04-16T10:46:15Z

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 729dcb8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

…() (apache#41070) ### Rationale for this change Loading the `null_count` attribute doesn't take into account the possible value of -1, leading to a code path where the validity buffer is accessed, but which is not necessarily present in that case. ### What changes are included in this PR? Use `data->MayHaveNulls()` instead of `data->null_count.load()` ### Are these changes tested? Yes * GitHub Issue: apache#41016 Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Apr 8, 2024

kou approved these changes Apr 8, 2024

View reviewed changes

cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Apr 8, 2024

mapleFU reviewed Apr 9, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge awaiting changes Awaiting changes labels Apr 9, 2024

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 9, 2024

felipecrv requested changes Apr 10, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 10, 2024

mapleFU approved these changes Apr 10, 2024

View reviewed changes

pitrou reviewed Apr 10, 2024

View reviewed changes

cpp/src/arrow/array/array_test.cc Outdated Show resolved Hide resolved

pitrou reviewed Apr 10, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 11, 2024

jorisvandenbossche added 3 commits April 15, 2024 16:57

apacheGH-41016: [C++] Fix null count check in BooleanArray.true_count()

0996ef8

update test

168dbed

use MayhaveNulls()

616a37b

pitrou force-pushed the gh-41016 branch from c8fd12c to ffb346d Compare April 15, 2024 14:57

pitrou approved these changes Apr 15, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 15, 2024

address feedback

9ae54ae

pitrou force-pushed the gh-41016 branch from ffb346d to 9ae54ae Compare April 15, 2024 15:01

pitrou merged commit 729dcb8 into apache:main Apr 15, 2024
33 of 35 checks passed

pitrou removed the awaiting change review Awaiting change review label Apr 15, 2024

pitrou mentioned this pull request Apr 15, 2024

[C++] BooleanArray true_count crashing in case of unknown null count (null_count = -1) without validity buffer #41016

Closed

jorisvandenbossche deleted the gh-41016 branch April 15, 2024 16:25

felipecrv mentioned this pull request Apr 15, 2024

[C++][Python] Casting map causes pyarrow to crash #38553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41016: [C++] Fix null count check in BooleanArray.true_count() #41070

GH-41016: [C++] Fix null count check in BooleanArray.true_count() #41070

jorisvandenbossche commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024

kou left a comment

mapleFU Apr 9, 2024

jorisvandenbossche Apr 9, 2024

mapleFU Apr 9, 2024

jorisvandenbossche commented Apr 9, 2024

felipecrv Apr 10, 2024

mapleFU Apr 10, 2024

jorisvandenbossche Apr 10, 2024

felipecrv Apr 11, 2024

mapleFU Apr 11, 2024

pitrou Apr 10, 2024

jorisvandenbossche Apr 11, 2024

pitrou commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

conbench-apache-arrow bot commented Apr 16, 2024

		if (data_->null_count.load() != 0) {
		if (data_->GetNullCount() != 0) {

GH-41016: [C++] Fix null count check in BooleanArray.true_count() #41070

GH-41016: [C++] Fix null count check in BooleanArray.true_count() #41070

Conversation

jorisvandenbossche commented Apr 8, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

github-actions bot commented Apr 8, 2024

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

conbench-apache-arrow bot commented Apr 16, 2024

jorisvandenbossche commented Apr 8, 2024 •

edited

Loading