zanmato1984 commented on code in PR #50402:
URL: https://github.com/apache/arrow/pull/50402#discussion_r3538939334
##########
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:
I don't think `IsValid` has a well-defined logical semantic for Union/RLE
builders at the base `ArrayBuilder` level. Returning false is misleading since
it makes every slot look null, while returning true only means there is no
top-level null bitmap to invalidate the slot.
So I think we either need to narrow the API semantics/name to
physical/top-level validity, e.g. `IsPhysicalValid` or `IsTopLevelValid`, or
make these builders unsupported for this API instead of pretending to answer
logical validity.
--
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]
