GH-34361: [C++] Fix the handling of logical nulls for types without bitmaps like Unions and Run-End Encoded #34408

felipecrv · 2023-03-01T22:44:00Z

Bonus: add ArrayData::IsValid() to make it consistent with Array and ArraySpan.

Rationale for this change

This is the proposed fix to #34361 plus the addition of more APIs dealing with validity/nullity.

What changes are included in this PR?

This PR changes the behavior of IsNull and IsValid in Array, ArrayData, and ArraySpan.

It preserves the behavior of MayHaveNulls, GetNullCount and introduces new member functions to Array, ArrayData, and ArraySpan:

bool HasValidityBitmap() const
bool MayHaveLogicalNulls() const
int64_t ComputeLogicalNullCount() const

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes. See above.

Breakage with these changes can only happen if users rely on IsNull(i) always returning true for union types, but we have users reporting that the current behavior or broken #34315. This is why the behavior of IsNull and IsValid is changing.,

This PR contains a "Critical Fix".

Closes: [C++] Handling "logical" nulls: add GetLogicalNullCount / update IsNull() to check for logical null #34361

github-actions · 2023-03-01T22:44:23Z

Closes: [C++] Handling "logical" nulls: add GetLogicalNullCount / update IsNull() to check for logical null #34361

jorisvandenbossche

The skeleton looks good to me

cpp/src/arrow/array/data.h

felipecrv · 2023-03-02T21:04:26Z

@jorisvandenbossche @westonpace @zeroshade this now has the implementation of all the new functions, changes in behavior, and tests. Ready for review.

westonpace

A few minor nits but this looks pretty well thought out

cpp/src/arrow/array/array_test.cc

cpp/src/arrow/array/data.h

cpp/src/arrow/util/union_util.cc

cpp/src/arrow/array/data.h

westonpace · 2023-03-07T21:13:30Z

Is this ready for another review? Please check "Re-request review" if so. Although I notice it has the awaiting change review label. I wonder if that is automatic anytime a commit is pushed.

felipecrv · 2023-03-13T14:53:34Z

@westonpace I'm a bit lost on how these labels change and what they actully mean, but the PR is ready for review again.

westonpace · 2023-03-14T22:38:32Z

@ursabot please benchmark

ursabot · 2023-03-14T22:38:39Z

Benchmark runs are scheduled for baseline = fc95019 and contender = ceacb2ef16a455cf73d6ad31d82928f185646597. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️2.64% ⬆️0.64%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️2.55% ⬆️0.64%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] ceacb2ef ec2-t3-xlarge-us-east-2
[Finished] ceacb2ef test-mac-arm
[Finished] ceacb2ef ursa-i9-9960x
[Finished] ceacb2ef ursa-thinkcentre-m75q
[Finished] fc950192 ec2-t3-xlarge-us-east-2
[Failed] fc950192 test-mac-arm
[Finished] fc950192 ursa-i9-9960x
[Finished] fc950192 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

westonpace

A few more questions. I also (hopefully) triggered conbench just to double check impact (though I don't know if we have any null-specific benchmarks).

cpp/src/arrow/array/array_test.cc

westonpace · 2023-03-14T22:34:34Z

cpp/src/arrow/array/array_test.cc

Do we have existing tests like this for sparse and dense union? (given we now have distinct paths for those)

I mentioned you in PR comments pointing to code that tests IsNull/Valid for unions.

westonpace · 2023-03-14T22:36:40Z

cpp/src/arrow/array/data.h

What prevents these functions from being .cc only function in an anonymous namespace? It seems like the only need is because we have functions later in data.h that could be in a .cc file.

They are called from IsNull and IsValid which are inline and having the switch on the type ID inlined allows the compiler to inline the highly predictable branches into the loop bodies from which these functions are called.

I could introduce an IsValidFallback function that handles the case when the validity buffer is NULLPTR, then all these could become .cc only.

I think it's ok how it is. I just don't ever have a good sense for when it's justified for leaving something in the header file for performance reasons. Typically I think it would be nice for there to be some benchmark to point to. Otherwise "it seems like this is important for performance" becomes too subjective of a criteria to apply.

The existing benchmarks indicated some regression (maybe due to union arrays being part of IsNull benchmarks) so I will investigate more carefully.

- Add REE to TestNullCount tests - Test new null handling functions in TestMakeArrayOfNull

ursabot · 2023-04-07T10:22:39Z

Benchmark runs are scheduled for baseline = 84e5430 and contender = fde31ed. fde31ed is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.56% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] fde31ed1 ec2-t3-xlarge-us-east-2
[Failed] fde31ed1 test-mac-arm
[Finished] fde31ed1 ursa-i9-9960x
[Failed] fde31ed1 ursa-thinkcentre-m75q
[Finished] 84e54308 ec2-t3-xlarge-us-east-2
[Failed] 84e54308 test-mac-arm
[Finished] 84e54308 ursa-i9-9960x
[Failed] 84e54308 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

jorisvandenbossche · 2023-04-12T13:32:35Z

cpp/src/arrow/util/union_util.cc

+    const int8_t child_id = sparse_union_type->child_ids()[types[span.offset + i]];
+
+    null_count += span.child_data[child_id].IsNull(i);


Did you test this with a sliced array? (don't directly see it in the tests)
Because in #35036, I based my implementation on this function, and I am finding that I need to remove/add the offset like so:

Suggested change

const int8_t child_id = sparse_union_type->child_ids()[types[span.offset + i]];

null_count += span.child_data[child_id].IsNull(i);

const int8_t child_id = sparse_union_type->child_ids()[types[i]];

null_count += span.child_data[child_id].IsNull(span.offset + i);

So types, which is the result of Span.GetValues() already takes into account the span's offset, but IsNull() is called on the child_data, and accessing child_data gives the original array data without offset already taken into account.

There is a high chance the existing tests didn't cover sliced union arrays and an even higher chance I messed up here because I had just read the Arrow spec on Unions to fix this.

It's not so much about the Arrow spec on Unions, but rather about the small details of our APIs (Span::GetValues returning sliced values or not, Span::child_data returning sliced data or not?)

GetValues applies slices. Direct access doesn't. I think that makes sense, but it's hard to keep in mind at all times.

child_ids() is unclear to me because it's a cache if I recall correctly.

…rvers Now that all the benchmark builds are running again. :fingers-crossed: This reverts commit fde31ed.

…hout bitmaps like Unions and Run-End Encoded (apache#34408) Bonus: add `ArrayData::IsValid()` to make it consistent with `Array` and `ArraySpan`. ### Rationale for this change This is the proposed fix to apache#34361 plus the addition of more APIs dealing with validity/nullity. ### What changes are included in this PR? This PR changes the behavior of `IsNull` and `IsValid` in `Array`, `ArrayData`, and `ArraySpan`. It preserves the behavior of `MayHaveNulls`, `GetNullCount` and introduces new member functions to `Array`, `ArrayData`, and `ArraySpan`: - `bool HasValidityBitmap() const` - `bool MayHaveLogicalNulls() const` - `int64_t ComputeLogicalNullCount() const` ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. See above. Breakage with these changes can only happen if users rely on `IsNull(i)` always returning `true` for union types, but we have users reporting that the current behavior or broken apache#34315. This is why the behavior of `IsNull` and `IsValid` is changing., **This PR contains a "Critical Fix".** * Closes: apache#34361 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Matt Topol <[email protected]>

github-actions bot added Component: C++ awaiting review Awaiting review labels Mar 1, 2023

jorisvandenbossche reviewed Mar 2, 2023

View reviewed changes

cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Mar 2, 2023

felipecrv force-pushed the null_fix branch from 5ac4dd3 to ce363c4 Compare March 2, 2023 21:03

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 2, 2023

felipecrv changed the title ~~GH-34361: [C++] work-in-progress: Add skeleton of the new APIs for handling null checks correctly for all types~~ GH-34361: [C++] Fix the handling of logical nulls for types without bitmaps like Unions and Run-End Encoded Mar 3, 2023

westonpace reviewed Mar 3, 2023

View reviewed changes

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

cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved

cpp/src/arrow/util/union_util.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Mar 3, 2023

felipecrv force-pushed the null_fix branch from 33a39d8 to 34950f9 Compare March 7, 2023 00:35

westonpace reviewed Mar 7, 2023

View reviewed changes

cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 7, 2023

felipecrv requested review from jorisvandenbossche and westonpace and removed request for jorisvandenbossche and westonpace March 7, 2023 23:15

westonpace reviewed Mar 14, 2023

View reviewed changes

felipecrv added 9 commits April 6, 2023 11:20

Add union_util.cc/h with functions for calculating logical null count

111ad6c

Adopt HasValidityBitmap/MayHaveLogicalNulls

b07f9a2

Add implementation that changes how nulls are checked on unions and REEs

a9b3194

Fix ArrayTest to reflect changes in IsNull/IsValid and add more checks

ce56514

- Add REE to TestNullCount tests - Test new null handling functions in TestMakeArrayOfNull

s/ASSERT_FALSE/ASSERT_NE

8f07725

Better fallback logic in MayHaveLogicalNulls()

4512297

Export internal functions called from inline functions

e5c77a1

Fix doxygen comment

6ea0ab7

Use MakeScalar from scalar.h

fa593d7

felipecrv force-pushed the null_fix branch from dbf4bb5 to fa593d7 Compare April 6, 2023 14:20

felipecrv marked this pull request as ready for review April 6, 2023 14:27

zeroshade approved these changes Apr 6, 2023

View reviewed changes

zeroshade merged commit fde31ed into apache:main Apr 6, 2023

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 6, 2023

felipecrv deleted the null_fix branch April 6, 2023 15:37

felipecrv mentioned this pull request Apr 12, 2023

[C++] Fix handling of logical nulls by compute kernels processing run-end encoded arrays or arrays of unions #35058

Open

4 tasks

jorisvandenbossche reviewed Apr 12, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Apr 12, 2023

felipecrv mentioned this pull request Apr 14, 2023

[C++] Add versions of IsNull/IsValid that take an ArrowType tparam so implementation can be statically dispatched #35141

Open

felipecrv added a commit to felipecrv/arrow that referenced this pull request May 2, 2023

Revert apache#34408 to see how it affects benchmarks running in CI se…

25c0ed9

…rvers Now that all the benchmark builds are running again. :fingers-crossed: This reverts commit fde31ed.

felipecrv mentioned this pull request May 10, 2023

GH-35141: [C++] Versions of IsNull/IsValid that don't branch on type #35149

Closed

jorisvandenbossche mentioned this pull request Nov 13, 2023

GH-38457: [C++] Support LogicalNullCount for DictionaryArray #38681

Merged

felipecrv mentioned this pull request Apr 10, 2024

[C++] A way to assert that arrays of a certain type have the validity of the values defined by the validity bitmap #41114

Closed

		const int8_t child_id = sparse_union_type->child_ids()[types[span.offset + i]];

		null_count += span.child_data[child_id].IsNull(i);

GH-34361: [C++] Fix the handling of logical nulls for types without bitmaps like Unions and Run-End Encoded #34408

GH-34361: [C++] Fix the handling of logical nulls for types without bitmaps like Unions and Run-End Encoded #34408

Uh oh!

Conversation

felipecrv commented Mar 1, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Mar 1, 2023

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felipecrv commented Mar 2, 2023

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonpace commented Mar 7, 2023

Uh oh!

felipecrv commented Mar 13, 2023

Uh oh!

westonpace commented Mar 14, 2023

Uh oh!

ursabot commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipecrv Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ursabot commented Apr 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

felipecrv commented Mar 1, 2023 •

edited by github-actions bot

Loading

ursabot commented Mar 14, 2023 •

edited

Loading

felipecrv Mar 15, 2023 •

edited

Loading