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

felipecrv · 2023-04-14T23:08:23Z

Rationale for this change

See #35141.

What changes are included in this PR?

Add missing ARROW_EXPORT
Make code style consistent in all IsNull/IsValid member functions
Add IsNullFast<ArrowType> and IsValidFast<ArrowType>

Are these changes tested?

Yes.

Are there any user-facing changes?

New member functions added to ArrayData / Array / ArraySpan.

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

github-actions · 2023-04-14T23:08:46Z

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

felipecrv · 2023-04-14T23:08:58Z

Please suggest a better name for these function if you can think of one.

mapleFU · 2023-04-17T03:35:09Z

cpp/src/arrow/array/data.h

+
+  template <typename ArrowType>
+  inline bool IsValidFast(int64_t i) const {
+    if constexpr (ArrowType::type_id == Type::NA) {


Do we need to add a DCHECK to check that ArrowType is equal to the real type in the Array?

I was tempted, but it adds too high of an overhead for this function as it's used from tight loops and is expected to be fully inlined. Even considering that it would be affecting only debug builds.

Maybe we just need care the release performance? An array using outerside ArrowType without checking is so dangerous...

Debug perf can make CI run much slower. Imagine changing the code from IsNull to IsNullFast and getting a slower build in return. I'm counting on people defaulting to use IsNull (safe) instead of IsNullFast. The latter is for people working on kernels which usually do many unsafe things and have more carefully written code.

@mapleFU note that all the special branches perform runtime type checks, so this is not completely unchecked.

Okay, though it's still a bit wired for me. I think we can hear about others idea.

To be more clear: the body of these member functions in the .cc file, where logging.h can be used, contains runtime checks.

} else if constexpr (ArrowType::type_id == Type::SPARSE_UNION) { return !IsNullSparseUnion(i); } else if constexpr (ArrowType::type_id == Type::DENSE_UNION) { return !IsNullDenseUnion(i); } else if constexpr (ArrowType::type_id == Type::RUN_END_ENCODED) { return !IsNullRunEndEncoded(i);

wgtmac · 2023-04-20T01:37:41Z

cpp/src/arrow/array/data.h

    if (buffers[0] != NULLPTR) {
      return bit_util::GetBit(buffers[0]->data(), i + offset);
    }
    const auto type = this->type->id();
    if (type == Type::SPARSE_UNION) {
      return !internal::IsNullSparseUnion(*this, i);
-    }
-    if (type == Type::DENSE_UNION) {
+    } else if (type == Type::DENSE_UNION) {


nit: this line of change seems to be complained by clang-tidy.

Not on my editor. Where did you see this?

https://clang.llvm.org/extra/clang-tidy/checks/readability/else-after-return.html

But this check is not enabled on Arrow's .clang-tidy and I don't think putting an else after a return is always bad.

In this specific function, the use of else is not creating unnecessary indentation levels.

Sounds good. I was overwhelmed by this kind of warning in the internal toolchain from my employer. :)

wgtmac · 2023-04-20T01:42:10Z

cpp/src/arrow/array/data.h

+      if (buffers[0] != NULLPTR) {
+        return bit_util::GetBit(buffers[0]->data(), i + offset);
+      }
+      return null_count.load() != length;


Maybe a dumb question: if the validity bitmap does not exist, shouldn't null_count be either 0 or -1 instead of other values?

NA is a known type that has no validity bitmap and null_count set to length. So in theory, this is very possible.

mapleFU · 2023-05-03T16:26:48Z

cpp/src/arrow/array/array_base.h

@@ -69,16 +69,37 @@ class ARROW_EXPORT Array {
    // a potential inner-branch removal.
    if (type_id() == Type::SPARSE_UNION) {
      return !internal::IsNullSparseUnion(*data_, i);
-    }


Why change it to if else? I think it's not necessary here
clang-tidy has a check about this style ( https://clang.llvm.org/extra/clang-tidy/checks/readability/else-after-return.html ), although we don't enable this check, I think just return without if else is ok

More compact. In sync with the if constexpr used in the other cases. Arrow's .clang-tidy doesn't complain about this. No extra indentation levels are introduced because of this.

mapleFU · 2023-05-03T16:28:35Z

cpp/src/arrow/array/data.h

+
+  template <typename ArrowType>
+  inline bool IsValidFast(int64_t i) const {
+    if constexpr (ArrowType::type_id == Type::NA) {


Okay, though it's still a bit wired for me. I think we can hear about others idea.

felipecrv · 2023-05-03T17:06:04Z

@westonpace what do you think about this change? cc @zeroshade @benibus

kernels that call IsNull/IsValid in loops are usually templated on the data type. They can benefit by using a version of these functions that can if constexpr on the type to dispatch the correct call.

pitrou · 2023-05-09T15:39:24Z

I'm skeptical about this. Did you find any kernels that call IsNull or IsValid? We have more efficient loop constructs for scanning entire null bitmaps, such as BitRunReader and BitBlockCounter.

(this is also handled internally by VisitArraySpanInline)

felipecrv · 2023-05-10T15:45:35Z

We have more efficient loop constructs for scanning entire null bitmaps, such as BitRunReader and BitBlockCounter.

I'm aware, but these don't exist/work for REE and Union arrays (and potentially new formats in the future) as described in
#34361.

I added a fix that required adding branches to IsNull and IsValid. See #34408. The ifs are inlined, so compiler might even be able to elide them, but these versions would guarantee that.

Did you find any kernels that call IsNull or IsValid?

Yes, but only as fallback for the types that don't have validity defined by a single bitmap. For instance, my fix of the "hash_count" kernel has a fallback to IsNull that is currently what makes union arrays give correct results https://github.com/apache/arrow/pull/35129/files#diff-c50313f1d80194615c5674cb64a1437c272a76da19027dabf8fb61ebffd7e2afR408

For REEs, I wrote a completely custom counting loop.

Since I already have a switch there, it would be nice to tell the IsNull function exactly what type of array it's dealing with.

pitrou · 2023-05-10T15:51:39Z

I'm aware, but these don't exist/work for REE and Union arrays (and potentially new formats in the future)

REE and Union are two different cases. For REE, it is better to iterate on runs, not logical values (exactly what you did for the "hash_count" kernel).

The ifs are inlined, so compiler might even be able to elide them, but these versions would guarantee that.

I'm not sure we care about compile-time guarantees here, since it's just a performance concern.

felipecrv · 2023-05-10T17:48:50Z

I'm not sure we care about compile-time guarantees here, since it's just a performance concern.

The compile-time guarantee being that the generated code has a certain shape affecting performance and binary size. But as I see it, my case is weak. I will rebase, remove the first commit, add some use-cases and close the PR that I might re-open later.

github-actions bot added the Component: C++ label Apr 14, 2023

github-actions bot added the awaiting review Awaiting review label Apr 14, 2023

felipecrv force-pushed the is_null_dispatch branch from 8506111 to 6c75516 Compare April 14, 2023 23:26

mapleFU reviewed Apr 17, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 17, 2023

wgtmac reviewed Apr 20, 2023

View reviewed changes

felipecrv force-pushed the is_null_dispatch branch from 6c75516 to c8d7891 Compare April 25, 2023 17:04

felipecrv mentioned this pull request May 2, 2023

GH-29887: [C++] Implement dictionary array sorting #35280

Merged

felipecrv force-pushed the is_null_dispatch branch from c8d7891 to 6bf55b5 Compare May 3, 2023 15:44

mapleFU reviewed May 3, 2023

View reviewed changes

felipecrv marked this pull request as ready for review May 3, 2023 17:02

felipecrv added 2 commits May 19, 2023 15:22

Make all IsNull/IsValid impls follow the same code template

2f4fe8a

Add IsNullFast and IsValidFast to ArrayData/Array/ArraySpan

e80c59b

felipecrv force-pushed the is_null_dispatch branch from 6bf55b5 to e80c59b Compare May 19, 2023 18:25

felipecrv closed this May 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

felipecrv commented Apr 14, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Apr 14, 2023

felipecrv commented Apr 14, 2023

mapleFU Apr 17, 2023

felipecrv Apr 17, 2023

mapleFU Apr 19, 2023

felipecrv Apr 24, 2023

felipecrv Apr 26, 2023

mapleFU May 3, 2023

felipecrv May 3, 2023

wgtmac Apr 20, 2023

felipecrv Apr 24, 2023

wgtmac Apr 25, 2023

felipecrv Apr 25, 2023

felipecrv Apr 25, 2023

wgtmac Apr 26, 2023

wgtmac Apr 20, 2023

felipecrv Apr 24, 2023

mapleFU May 3, 2023

felipecrv May 3, 2023

mapleFU May 3, 2023

felipecrv commented May 3, 2023

pitrou commented May 9, 2023

felipecrv commented May 10, 2023

pitrou commented May 10, 2023

felipecrv commented May 10, 2023

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

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

Conversation

felipecrv commented Apr 14, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Apr 14, 2023

felipecrv commented Apr 14, 2023

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

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

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

felipecrv commented May 3, 2023

pitrou commented May 9, 2023

felipecrv commented May 10, 2023

pitrou commented May 10, 2023

felipecrv commented May 10, 2023

felipecrv commented Apr 14, 2023 •

edited by github-actions bot

Loading