Skip to content

GH-50249: [C++] Add ArrayBuilder::IsValid and ArrayBuilder::IsNull #50402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andishgar wants to merge 2 commits into
base: main
Choose a base branch
from
+46 −0
Open

GH-50249: [C++] Add ArrayBuilder::IsValid and ArrayBuilder::IsNull #50402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d05bfb
Append IsNull and IsVaili with relevant tests
andishgar Jul 7, 2026
b419a90
use switch_case instead of if
andishgar Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions cpp/src/arrow/array/array_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,33 @@ TEST_F(TestBuilder, TestResizeDownsize) {
ASSERT_GE(500, builder.capacity());
ASSERT_EQ(500, builder.length());
}
namespace {

void AssertBuilderValidityAtIndices(const ArrayBuilder& builder,
const std::vector<int64_t>& indices,
const std::vector<bool>& expected_validity) {
for (size_t i = 0; i < indices.size(); ++i) {
ASSERT_EQ(expected_validity[i], builder.IsValid(indices[i]));
}
}

} // namespace

TEST_F(TestBuilder, TestIsNull) {
NullBuilder null_builder(pool_);
ASSERT_OK(null_builder.AppendNull());
ASSERT_OK(null_builder.Append(nullptr));
AssertBuilderValidityAtIndices(null_builder, {0, 1}, {false, false});
Comment thread
andishgar marked this conversation as resolved.

Int32Builder int32_builder(pool_);
ASSERT_OK(int32_builder.Append(0));
ASSERT_OK(int32_builder.AppendNull());
ASSERT_OK(int32_builder.Append(2));
ASSERT_OK(int32_builder.AppendNulls(3));
ASSERT_OK(int32_builder.Append(6));
ASSERT_OK(int32_builder.AppendEmptyValues(3));
AssertBuilderValidityAtIndices(int32_builder, {0, 1, 4, 7}, {true, false, false, true});
}

template <typename Attrs>
class TestPrimitiveBuilder : public TestBuilder {
Expand Down
19 changes: 19 additions & 0 deletions cpp/src/arrow/array/builder_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Comment on lines +119 to +120

@pitrou pitrou Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not exactly that it does not work, it's that it doesn't account for logical nulls.

Suggested change
/// 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).
/// Unlike Array::IsNull, this method does not account for logical nulls
/// for types that do not have a top-level validity bitmap
/// (Union and Run-End Encoded 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:
Comment on lines +124 to +126

@pitrou pitrou Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three should return true perhaps?

@pitrou pitrou Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @zanmato1984 for opinions

@zanmato1984 zanmato1984 Jul 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
default:
return bit_util::GetBit(null_bitmap_builder_.data(), i);
}
}

virtual int64_t length() const { return length_; }
int64_t null_count() const { return null_count_; }
int64_t capacity() const { return capacity_; }
Expand Down
Loading