andishgar commented on code in PR #50402:
URL: https://github.com/apache/arrow/pull/50402#discussion_r4116251763
##########
cpp/src/arrow/array/builder_base.h:
##########
@@ -111,6 +111,25 @@ class ARROW_EXPORT ArrayBuilder {
int num_children() const { return static_cast<int>(children_.size()); }
+ /// \brief Return true if value at index is null. Does not boundscheck
+ bool IsNull(int64_t i) const { return !IsValid(i); }
+
+ /// \brief Return true if value at index is valid (not null). Does not
+ /// boundscheck.
+ /// Note that this method does not work for types that do not have a
+ /// top-level validity bitmap (Union and Run-End Encoded (RLE) types).
+ bool IsValid(int64_t i) const {
+ switch (type()->id()) {
+ case Type::NA:
+ case Type::SPARSE_UNION:
+ case Type::DENSE_UNION:
+ case Type::RUN_END_ENCODED:
Review Comment:
@pitrou Sorry for the long delay in getting back to you.
A few notes:
1. Regarding this:
> Since `Array::IsValid` looks up the physical validity bitmap,
`arrow::Array::IsValid` checks whether an element is null for REE and
Union types as well. See
[here](https://github.com/apache/arrow/blob/a99c14ed5ee9dd183b60e66c0daa5301d75dcb4b/cpp/src/arrow/array/array_base.h#L63-L81).
2. Regarding [this line of
code](https://github.com/andishgar/arrow/blob/b419a901dd195b7611eb1fd2babbce5ed13f817e/cpp/src/arrow/array/builder_base.h#L122),
as shown
[here](https://github.com/apache/arrow/blob/a99c14ed5ee9dd183b60e66c0daa5301d75dcb4b/cpp/src/arrow/array/builder_nested.h#L609-L618),
[here](https://github.com/apache/arrow/blob/a99c14ed5ee9dd183b60e66c0daa5301d75dcb4b/cpp/src/arrow/array/builder_nested.cc#L307-L314),
and
[here](https://github.com/apache/arrow/blob/a99c14ed5ee9dd183b60e66c0daa5301d75dcb4b/cpp/src/arrow/array/builder_union.cc#L113-L120),
the nested type is constructed on each invocation of
`arrow::ArrayBuilder::type()`. This can be particularly costly for Union types,
since each construction allocates 512 bytes, as shown
[here](https://github.com/apache/arrow/blob/a99c14ed5ee9dd183b60e66c0daa5301d75dcb4b/cpp/src/arrow/type.cc#L1231).
Given this, I see two possible approaches:
1. If we want `arrow::ArrayBuilder::IsValid` to work only for physical
nulls, I suggest merging [PR
#51567](https://github.com/apache/arrow/pull/51567) first to enable `type_id()`
and avoid the extra allocation.
2. Alternatively, we could make the method virtual and enable it to handle
logical nulls.
Which approach do you suggest?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
