From 5f5cc7f4f654f7bac3ae8781cc4916ce89ba2aa7 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Thu, 12 Oct 2023 16:54:31 -0400 Subject: [PATCH 1/5] GH-37072: [C++] MakeArrayOfNulls should respect Field::nullable --- cpp/src/arrow/array/array_run_end_test.cc | 18 +- cpp/src/arrow/array/array_test.cc | 13 + cpp/src/arrow/array/util.cc | 418 +++++++++++++--------- cpp/src/arrow/array/validate.cc | 94 +++-- cpp/src/arrow/extension_type.h | 3 + cpp/src/arrow/type.h | 3 + 6 files changed, 311 insertions(+), 238 deletions(-) diff --git a/cpp/src/arrow/array/array_run_end_test.cc b/cpp/src/arrow/array/array_run_end_test.cc index 6cf50ccade19..1690786ad506 100644 --- a/cpp/src/arrow/array/array_run_end_test.cc +++ b/cpp/src/arrow/array/array_run_end_test.cc @@ -542,9 +542,7 @@ TEST_P(TestRunEndEncodedArray, Validate) { BitmapFromVector({true, false}, &null_bitmap); has_null_buffer->data()->buffers[0] = null_bitmap; EXPECT_RAISES_WITH_MESSAGE_THAT( - Invalid, - ::testing::HasSubstr( - std::string("Invalid: Run end encoded array should not have a null bitmap.")), + Invalid, ::testing::HasSubstr(std::string("should not have a null bitmap")), has_null_buffer->Validate()); auto too_many_children = MakeArray(good_array->data()->Copy()); @@ -570,7 +568,7 @@ TEST_P(TestRunEndEncodedArray, Validate) { values_nullptr->Validate()); auto run_ends_string = MakeArray(good_array->data()->Copy()); - run_ends_string->data()->child_data[0] = values->data(); + run_ends_string->data()->child_data[0] = MakeEmptyArray(utf8()).ValueOrDie()->data(); EXPECT_RAISES_WITH_MESSAGE_THAT( Invalid, ::testing::HasSubstr( @@ -596,10 +594,9 @@ TEST_P(TestRunEndEncodedArray, Validate) { malformed_array->data()->buffers.emplace_back(NULLPTR); EXPECT_RAISES_WITH_MESSAGE_THAT( Invalid, - ::testing::HasSubstr( - std::string( - "Invalid: Run ends array invalid: Expected 2 buffers in array of type ") + - run_end_type->ToString() + ", got 3"), + ::testing::HasSubstr(std::string("Invalid: Run ends array invalid: Invalid: " + "Expected 2 buffers in array of type ") + + run_end_type->ToString() + ", got 3"), run_ends_malformed->Validate()); } @@ -610,8 +607,9 @@ TEST_P(TestRunEndEncodedArray, Validate) { malformed_array->data()->buffers.emplace_back(NULLPTR); EXPECT_RAISES_WITH_MESSAGE_THAT( Invalid, - ::testing::HasSubstr("Invalid: Values array invalid: Expected 2 buffers in array " - "of type int32, got 3"), + ::testing::HasSubstr( + "Invalid: Values array invalid: Invalid: Expected 2 buffers in array " + "of type int32, got 3"), values_malformed->Validate()); } diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index 2cdf94bfab95..1beda6e35189 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -477,6 +477,7 @@ static std::vector> TestArrayUtilitiesAgainstTheseType TEST_F(TestArray, TestMakeArrayOfNull) { for (int64_t length : {0, 1, 16, 133}) { + ARROW_SCOPED_TRACE("length = ", length); for (auto type : TestArrayUtilitiesAgainstTheseTypes()) { ARROW_SCOPED_TRACE("type = ", type->ToString()); ASSERT_OK_AND_ASSIGN(auto array, MakeArrayOfNull(type, length)); @@ -506,6 +507,18 @@ TEST_F(TestArray, TestMakeArrayOfNull) { } } } + + auto req = [](auto type) { return field("", std::move(type), /*nullable=*/false); }; + + // union with no nullable fields cannot represent a null + ASSERT_RAISES(Invalid, MakeArrayOfNull(dense_union({req(int8())}), 1)); + + // struct with no nullable fields has a top level bitmap and can mask them + ASSERT_OK_AND_ASSIGN(auto s, MakeArrayOfNull(struct_({req(int8())}), 1)); + ASSERT_OK(s->ValidateFull()); + + ASSERT_OK_AND_ASSIGN(s, MakeArrayOfNull(struct_({req(dictionary(int8(), int8()))}), 1)); + ASSERT_OK(s->ValidateFull()); } TEST_F(TestArray, TestMakeArrayOfNullUnion) { diff --git a/cpp/src/arrow/array/util.cc b/cpp/src/arrow/array/util.cc index d97e2f7f85d9..27489cae7199 100644 --- a/cpp/src/arrow/array/util.cc +++ b/cpp/src/arrow/array/util.cc @@ -44,6 +44,7 @@ #include "arrow/util/decimal.h" #include "arrow/util/endian.h" #include "arrow/util/logging_internal.h" +#include "arrow/util/range.h" #include "arrow/util/sort_internal.h" #include "arrow/visit_data_inline.h" #include "arrow/visit_type_inline.h" @@ -51,6 +52,7 @@ namespace arrow { using internal::checked_cast; +using internal::Zip; // ---------------------------------------------------------------------- // Loading from ArrayData @@ -330,231 +332,249 @@ static Result> MakeScalarForRunEndValue( return std::make_shared(run_end); } -// get the maximum buffer length required, then allocate a single zeroed buffer -// to use anywhere a buffer is required class NullArrayFactory { public: - struct GetBufferLength { - GetBufferLength(const std::shared_ptr& type, int64_t length) - : type_(*type), length_(length), buffer_length_(bit_util::BytesForBits(length)) {} - - Result Finish() && { - RETURN_NOT_OK(VisitTypeInline(type_, this)); - return buffer_length_; - } - - template ::bytes_required(0))> - Status Visit(const T&) { - return MaxOf(TypeTraits::bytes_required(length_)); - } - - template - enable_if_var_size_list Visit(const T& type) { - // values array may be empty, but there must be at least one offset of 0 - RETURN_NOT_OK(MaxOf(sizeof(typename T::offset_type) * (length_ + 1))); - RETURN_NOT_OK(MaxOf(GetBufferLength(type.value_type(), /*length=*/0))); - return Status::OK(); - } - - template - enable_if_list_view Visit(const T& type) { - RETURN_NOT_OK(MaxOf(sizeof(typename T::offset_type) * length_)); - RETURN_NOT_OK(MaxOf(GetBufferLength(type.value_type(), /*length=*/0))); - return Status::OK(); - } - - template - enable_if_base_binary Visit(const T&) { - // values buffer may be empty, but there must be at least one offset of 0 - return MaxOf(sizeof(typename T::offset_type) * (length_ + 1)); - } - - Status Visit(const BinaryViewType& type) { - return MaxOf(sizeof(BinaryViewType::c_type) * length_); - } - - Status Visit(const FixedSizeListType& type) { - return MaxOf(GetBufferLength(type.value_type(), type.list_size() * length_)); - } - - Status Visit(const FixedSizeBinaryType& type) { - return MaxOf(type.byte_width() * length_); - } - - Status Visit(const StructType& type) { - for (const auto& child : type.fields()) { - RETURN_NOT_OK(MaxOf(GetBufferLength(child->type(), length_))); - } - return Status::OK(); - } - - Status Visit(const SparseUnionType& type) { - // type codes - RETURN_NOT_OK(MaxOf(length_)); - // will create children of the same length as the union - for (const auto& child : type.fields()) { - RETURN_NOT_OK(MaxOf(GetBufferLength(child->type(), length_))); - } - return Status::OK(); - } + // For most types, every buffer in an entirely null array will contain nothing but + // zeroes. For arrays of such types, we can allocate a single buffer and use that in + // every position of the array data. The first stage of visitation handles assessment + // of this buffer's size, the second uses the resulting buffer to build the null array. + // + // The first stage may not allocate from the MemoryPool or raise a failing status. + // + // In the second stage, `zero_buffer_` has been allocated and `out_` has: + // - type = type_ + // - length = length_ + // - null_count = length_ unless current output may have direct nulls, + // 0 otherwise + // - offset = 0 + // - buffers = [] + // - child_data = [nullptr] * type.num_fields() + // - dictionary = nullptr + bool presizing_zero_buffer_; - Status Visit(const DenseUnionType& type) { - // type codes - RETURN_NOT_OK(MaxOf(length_)); - // offsets - RETURN_NOT_OK(MaxOf(sizeof(int32_t) * length_)); - // will create children of length 1 - for (const auto& child : type.fields()) { - RETURN_NOT_OK(MaxOf(GetBufferLength(child->type(), 1))); - } - return Status::OK(); - } + NullArrayFactory(const std::shared_ptr& type, bool nullable, int64_t length) + : presizing_zero_buffer_{true}, + type_{type}, + nullable_{nullable}, + length_{length}, + zero_buffer_length_{MayHaveDirectNulls() ? bit_util::BytesForBits(length) : 0} {} - Status Visit(const DictionaryType& type) { - RETURN_NOT_OK(MaxOf(GetBufferLength(type.value_type(), length_))); - return MaxOf(GetBufferLength(type.index_type(), length_)); - } + NullArrayFactory(const std::shared_ptr& type, bool nullable, int64_t length, + const std::shared_ptr& zero_buffer, MemoryPool* pool) + : presizing_zero_buffer_{false}, + type_{type}, + nullable_{nullable}, + length_{length}, + zero_buffer_length_{MayHaveDirectNulls() ? bit_util::BytesForBits(length) : 0}, + zero_buffer_{&zero_buffer}, + pool_{pool} {} - Status Visit(const RunEndEncodedType& type) { - // RunEndEncodedType has no buffers, only child arrays - buffer_length_ = 0; - return Status::OK(); - } - - Status Visit(const ExtensionType& type) { - // XXX is an extension array's length always == storage length - return MaxOf(GetBufferLength(type.storage_type(), length_)); - } + template + explicit NullArrayFactory(const std::shared_ptr& field, const Args&... args) + : NullArrayFactory{field->type(), field->nullable(), args...} {} - Status Visit(const DataType& type) { - return Status::NotImplemented("construction of all-null ", type); - } + bool MayHaveDirectNulls() const { + if (type_->storage_id() == Type::NA) return true; + return nullable_ && internal::may_have_validity_bitmap(type_->storage_id()); + } - private: - Status MaxOf(GetBufferLength&& other) { - ARROW_ASSIGN_OR_RAISE(int64_t buffer_length, std::move(other).Finish()); - return MaxOf(buffer_length); - } + void ZeroBufferMustBeAtLeast(int64_t length) { + DCHECK(presizing_zero_buffer_); + zero_buffer_length_ = std::max(zero_buffer_length_, length); + } - Status MaxOf(int64_t buffer_length) { - if (buffer_length > buffer_length_) { - buffer_length_ = buffer_length; - } - return Status::OK(); - } + std::shared_ptr GetValidityBitmap() const { + DCHECK(!presizing_zero_buffer_); + return MayHaveDirectNulls() ? *zero_buffer_ : nullptr; + } - const DataType& type_; - int64_t length_, buffer_length_; - }; + static int64_t GetZeroBufferLength(const std::shared_ptr& type, bool nullable, + int64_t length) { + NullArrayFactory factory{type, nullable, length}; + DCHECK_OK(VisitTypeInline(*type, &factory)); + return factory.zero_buffer_length_; + } - NullArrayFactory(MemoryPool* pool, const std::shared_ptr& type, - int64_t length) - : pool_(pool), type_(type), length_(length) {} + static int64_t GetZeroBufferLength(const std::shared_ptr& field, + int64_t length) { + return GetZeroBufferLength(field->type(), field->nullable(), length); + } - Status CreateBuffer() { - if (type_->id() == Type::RUN_END_ENCODED) { - buffer_ = NULLPTR; - return Status::OK(); + Status Visit(const NullType&) { + if (presizing_zero_buffer_) { + // null needs no buffers; don't touch the zero buffer size + } else { + out_->buffers = {nullptr}; } - ARROW_ASSIGN_OR_RAISE(int64_t buffer_length, - GetBufferLength(type_, length_).Finish()); - ARROW_ASSIGN_OR_RAISE(buffer_, AllocateBuffer(buffer_length, pool_)); - std::memset(buffer_->mutable_data(), 0, buffer_->size()); return Status::OK(); } - Result> Create() { - if (buffer_ == nullptr) { - RETURN_NOT_OK(CreateBuffer()); + Status Visit(const BooleanType& type) { + if (presizing_zero_buffer_) { + ZeroBufferMustBeAtLeast(bit_util::BytesForBits(length_)); + return Status::OK(); } - std::vector> child_data(type_->num_fields()); - auto buffer_slice = - buffer_ ? SliceBuffer(buffer_, 0, bit_util::BytesForBits(length_)) : NULLPTR; - out_ = ArrayData::Make(type_, length_, {std::move(buffer_slice)}, child_data, length_, - 0); - RETURN_NOT_OK(VisitTypeInline(*type_, this)); - return out_; - } - - Status Visit(const NullType&) { - out_->buffers.resize(1, nullptr); + out_->buffers = {GetValidityBitmap(), *zero_buffer_}; return Status::OK(); } - Status Visit(const FixedWidthType&) { - out_->buffers.resize(2, buffer_); + Status Visit(const FixedWidthType& type) { + if (presizing_zero_buffer_) { + ZeroBufferMustBeAtLeast(type.byte_width() * length_); + return Status::OK(); + } + out_->buffers = {GetValidityBitmap(), *zero_buffer_}; return Status::OK(); } template enable_if_base_binary Visit(const T&) { - out_->buffers.resize(3, buffer_); + if (presizing_zero_buffer_) { + // values buffer may be empty, but there must be at least one offset of 0 + ZeroBufferMustBeAtLeast(sizeof(typename T::offset_type) * (length_ + 1)); + return Status::OK(); + } + out_->buffers = {GetValidityBitmap(), *zero_buffer_, *zero_buffer_}; return Status::OK(); } Status Visit(const BinaryViewType&) { - out_->buffers.resize(2, buffer_); + out_->buffers.resize(2, *zero_buffer_); return Status::OK(); } template enable_if_var_length_list_like Visit(const T& type) { - out_->buffers.resize(is_list_view(T::type_id) ? 3 : 2, buffer_); - ARROW_ASSIGN_OR_RAISE(out_->child_data[0], CreateChild(type, 0, /*length=*/0)); - return Status::OK(); + constexpr bool kIsView = is_list_view(T::type_id); + if (presizing_zero_buffer_) { + auto offsets_length = length_; + if constexpr (!kIsView) { + // there must be at least one offset of 0 + offsets_length += 1; + } + ZeroBufferMustBeAtLeast(sizeof(typename T::offset_type) * offsets_length); + // include length required for zero length child + ZeroBufferMustBeAtLeast(GetZeroBufferLength(type.value_field(), 0)); + return Status::OK(); + } + if constexpr (!kIsView) { + out_->buffers = {GetValidityBitmap(), *zero_buffer_}; + } else { + out_->buffers = {GetValidityBitmap(), *zero_buffer_, *zero_buffer_}; + } + return CreateChild(0, /*length=*/0); } Status Visit(const FixedSizeListType& type) { - ARROW_ASSIGN_OR_RAISE(out_->child_data[0], - CreateChild(type, 0, length_ * type.list_size())); - return Status::OK(); + if (presizing_zero_buffer_) { + ZeroBufferMustBeAtLeast( + GetZeroBufferLength(type.value_field(), type.list_size() * length_)); + return Status::OK(); + } + out_->buffers = {GetValidityBitmap()}; + return CreateChild(0, type.list_size() * length_); } Status Visit(const StructType& type) { + if (presizing_zero_buffer_) { + for (const auto& child : type.fields()) { + ZeroBufferMustBeAtLeast(GetZeroBufferLength(child, length_)); + } + return Status::OK(); + } + out_->buffers = {GetValidityBitmap()}; for (int i = 0; i < type.num_fields(); ++i) { - ARROW_ASSIGN_OR_RAISE(out_->child_data[i], CreateChild(type, i, length_)); + RETURN_NOT_OK(CreateChild(i, length_)); } return Status::OK(); } + static Result GetIdOfFirstNullableUnionMember(const UnionType& type) { + for (auto [field, id] : Zip(type.fields(), type.type_codes())) { + if (field->nullable()) return id; + } + return Status::Invalid("Cannot produce an array of null ", type, + " because no child field is nullable"); + } + Status Visit(const UnionType& type) { - out_->buffers.resize(2); + // For sparse unions, we create children with the same length as the parent. + // + // For dense unions, we create children with length 1 and have offsets which always + // refer to the first first slot from one child. + int64_t child_length = type.mode() == UnionMode::SPARSE ? length_ : 1; - // First buffer is always null - out_->buffers[0] = nullptr; + if (presizing_zero_buffer_) { + // type codes + ZeroBufferMustBeAtLeast(length_); + + if (type.mode() == UnionMode::DENSE) { + // offsets + ZeroBufferMustBeAtLeast(sizeof(int32_t) * length_); + } - out_->buffers[1] = buffer_; - // buffer_ is zeroed, but 0 may not be a valid type code - if (type.type_codes()[0] != 0) { - ARROW_ASSIGN_OR_RAISE(out_->buffers[1], AllocateBuffer(length_, pool_)); - std::memset(out_->buffers[1]->mutable_data(), type.type_codes()[0], length_); + for (const auto& child : type.fields()) { + ZeroBufferMustBeAtLeast(GetZeroBufferLength(child, child_length)); + } + return Status::OK(); } - // For sparse unions, we now create children with the same length as the - // parent - int64_t child_length = length_; - if (type.mode() == UnionMode::DENSE) { - // For dense unions, we set the offsets to all zero and create children - // with length 1 - out_->buffers.resize(3); - out_->buffers[2] = buffer_; + // The validity bitmap is always absent for unions + out_->buffers = {nullptr}; - child_length = 1; + // Next is the type ids buffer. We may not be able to use zero_buffer_ + // for this since 0 may not be a valid type id. + ARROW_ASSIGN_OR_RAISE(int8_t first_nullable_id, + GetIdOfFirstNullableUnionMember(type)); + if (first_nullable_id == 0) { + out_->buffers.push_back(*zero_buffer_); + } else { + ARROW_ASSIGN_OR_RAISE(auto type_ids, AllocateBuffer(length_, pool_)); + std::memset(type_ids->mutable_data(), first_nullable_id, length_); + out_->buffers.push_back(std::move(type_ids)); + } + + if (type.mode() == UnionMode::DENSE) { + out_->buffers.push_back(*zero_buffer_); } - for (int i = 0; i < type_->num_fields(); ++i) { - ARROW_ASSIGN_OR_RAISE(out_->child_data[i], CreateChild(type, i, child_length)); + + for (int i = 0; i < type.num_fields(); ++i) { + RETURN_NOT_OK(CreateChild(i, child_length)); } return Status::OK(); } Status Visit(const DictionaryType& type) { - out_->buffers.resize(2, buffer_); - ARROW_ASSIGN_OR_RAISE(auto typed_null_dict, MakeArrayOfNull(type.value_type(), 0)); - out_->dictionary = typed_null_dict->data(); + // The dictionary's indices are non-nullable; we can still create an array + // by creating a dictionary which contains a single null. + int64_t dictionary_length = nullable_ ? 0 : 1; + + if (presizing_zero_buffer_) { + ZeroBufferMustBeAtLeast( + GetZeroBufferLength(type.value_type(), /*nullable=*/true, dictionary_length)); + ZeroBufferMustBeAtLeast(GetZeroBufferLength(type.index_type(), nullable_, length_)); + return Status::OK(); + } + + out_->buffers = {GetValidityBitmap(), *zero_buffer_}; + ARROW_ASSIGN_OR_RAISE( + out_->dictionary, + CreateRelated(type.value_type(), /*nullable=*/true, dictionary_length)); return Status::OK(); } Status Visit(const RunEndEncodedType& type) { + if (presizing_zero_buffer_) { + // Run end encoded arrays have no buffers. + return Status::OK(); + } + + out_->buffers = {nullptr}; + if (!type.field(1)->nullable()) { + return Status::Invalid("Cannot produce an array of null ", type, + " because the values field is not nullable"); + } + std::shared_ptr run_ends, values; if (length_ == 0) { ARROW_ASSIGN_OR_RAISE(run_ends, MakeEmptyArray(type.run_end_type(), pool_)); @@ -565,14 +585,21 @@ class NullArrayFactory { ARROW_ASSIGN_OR_RAISE(run_ends, MakeArrayFromScalar(*run_end_scalar, 1, pool_)); ARROW_ASSIGN_OR_RAISE(values, MakeArrayOfNull(type.value_type(), 1, pool_)); } - out_->child_data[0] = run_ends->data(); - out_->child_data[1] = values->data(); + out_->child_data = {run_ends->data(), values->data()}; return Status::OK(); } Status Visit(const ExtensionType& type) { - out_->child_data.resize(type.storage_type()->num_fields()); - RETURN_NOT_OK(VisitTypeInline(*type.storage_type(), this)); + if (presizing_zero_buffer_) { + ZeroBufferMustBeAtLeast( + GetZeroBufferLength(type.storage_type(), nullable_, length_)); + return Status::OK(); + } + + ARROW_ASSIGN_OR_RAISE(auto out, + CreateRelated(type.storage_type(), nullable_, length_)); + *out_ = std::move(*out); + out_->type = type_; return Status::OK(); } @@ -580,18 +607,45 @@ class NullArrayFactory { return Status::NotImplemented("construction of all-null ", type); } - Result> CreateChild(const DataType& type, int i, - int64_t length) { - NullArrayFactory child_factory(pool_, type.field(i)->type(), length); - child_factory.buffer_ = buffer_; - return child_factory.Create(); + Result> Create() && { + DCHECK(!presizing_zero_buffer_); + auto out = std::make_shared(); + out_ = out.get(); + out_->type = type_; + out_->length = length_; + out_->null_count = MayHaveDirectNulls() ? length_ : 0; + out_->offset = 0; + out_->buffers = {}; + out_->child_data.resize(type_->storage_type_ref().num_fields()); + out_->dictionary = nullptr; + RETURN_NOT_OK(VisitTypeInline(*type_, this)); + return out; + } + + Status CreateChild(int i, int64_t length) { + DCHECK(!presizing_zero_buffer_); + const auto& field = type_->storage_type_ref().field(i); + ARROW_ASSIGN_OR_RAISE(out_->child_data[i], + CreateRelated(field->type(), field->nullable(), length)); + return Status::OK(); + } + + Result> CreateRelated( + const std::shared_ptr& related_type, bool nullable, int64_t length) { + return NullArrayFactory{related_type, nullable, length, *zero_buffer_, pool_} + .Create(); } - MemoryPool* pool_; const std::shared_ptr& type_; + bool nullable_; int64_t length_; - std::shared_ptr out_; - std::shared_ptr buffer_; + + int64_t zero_buffer_length_ = 0; + + const std::shared_ptr* zero_buffer_; + MemoryPool* pool_; + + ArrayData* out_; }; class RepeatedArrayFactory { @@ -898,8 +952,14 @@ class RepeatedArrayFactory { Result> MakeArrayOfNull(const std::shared_ptr& type, int64_t length, MemoryPool* pool) { - ARROW_ASSIGN_OR_RAISE(auto data, NullArrayFactory(pool, type, length).Create()); - return MakeArray(data); + int64_t zero_buffer_length = + NullArrayFactory::GetZeroBufferLength(type, /*nullable=*/true, length); + ARROW_ASSIGN_OR_RAISE(std::shared_ptr zero_buffer, + AllocateBuffer(zero_buffer_length, pool)); + std::memset(zero_buffer->mutable_data(), 0, zero_buffer->size()); + return NullArrayFactory{type, /*nullable=*/true, length, zero_buffer, pool} + .Create() + .Map(MakeArray); } Result> MakeArrayFromScalar(const Scalar& scalar, int64_t length, diff --git a/cpp/src/arrow/array/validate.cc b/cpp/src/arrow/array/validate.cc index 16bc9187af46..46ce36239057 100644 --- a/cpp/src/arrow/array/validate.cc +++ b/cpp/src/arrow/array/validate.cc @@ -297,26 +297,16 @@ struct ValidateArrayImpl { ") multiplied by the value size (", list_size, ")"); } - const Status child_valid = RecurseInto(values); - if (!child_valid.ok()) { - return Status::Invalid("Fixed size list child array invalid: ", - child_valid.ToString()); - } - + RETURN_NOT_OK(RecurseIntoField(0, "Fixed size list child array")); return Status::OK(); } Status Visit(const StructType& type) { for (int i = 0; i < type.num_fields(); ++i) { - const auto& field_data = *data.child_data[i]; - // Validate child first, to catch nonsensical length / offset etc. - const Status field_valid = RecurseInto(field_data); - if (!field_valid.ok()) { - return Status::Invalid("Struct child array #", i, - " invalid: ", field_valid.ToString()); - } + RETURN_NOT_OK(RecurseIntoField(0, "Struct child array #", i)); + const auto& field_data = *data.child_data[i]; if (field_data.length < data.length + data.offset) { return Status::Invalid("Struct child array #", i, " has length smaller than expected for struct array (", @@ -335,15 +325,10 @@ struct ValidateArrayImpl { Status Visit(const UnionType& type) { for (int i = 0; i < type.num_fields(); ++i) { - const auto& field_data = *data.child_data[i]; - // Validate children first, to catch nonsensical length / offset etc. - const Status field_valid = RecurseInto(field_data); - if (!field_valid.ok()) { - return Status::Invalid("Union child array #", i, - " invalid: ", field_valid.ToString()); - } + RETURN_NOT_OK(RecurseIntoField(i, "Union child array #", i)); + const auto& field_data = *data.child_data[i]; if (type.mode() == UnionMode::SPARSE && field_data.length < data.length + data.offset) { return Status::Invalid("Sparse union child array #", i, @@ -421,10 +406,7 @@ struct ValidateArrayImpl { return Status::Invalid("Dictionary values must be non-null"); } // Validate dictionary - const Status dict_valid = RecurseInto(*data.dictionary); - if (!dict_valid.ok()) { - return Status::Invalid("Dictionary array invalid: ", dict_valid.ToString()); - } + RETURN_NOT_OK(RecurseInto(*data.dictionary, "Dictionary array")); // Validate indices RETURN_NOT_OK(ValidateWithType(*type.index_type())); @@ -465,9 +447,28 @@ struct ValidateArrayImpl { return data.buffers[index] != nullptr && data.buffers[index]->address() != 0; } - Status RecurseInto(const ArrayData& related_data) { - ValidateArrayImpl impl{related_data, full_validation}; - return impl.Validate(); + template + Status RecurseIntoField(int field_index, const FieldDescription&... description) { + const auto& related_data = *data.child_data[field_index]; + + if (!data.type->storage_type_ref().field(field_index)->nullable()) { + if (related_data.null_count != 0 && related_data.null_count != kUnknownNullCount) { + return Status::Invalid(description..., + " invalid: was non-nullable but had null count ", + related_data.null_count); + } + } + return RecurseInto(related_data, description...); + } + + template + Status RecurseInto(const ArrayData& related_data, + const FieldDescription&... description) { + Status st = ValidateArrayImpl{related_data, full_validation}.Validate(); + if (!st.ok()) { + return st.WithMessage(description..., " invalid: ", st.ToString()); + } + return Status::OK(); } Status ValidateLayout(const DataType& type) { @@ -528,7 +529,7 @@ struct ValidateArrayImpl { } break; case DataTypeLayout::ALWAYS_NULL: - // XXX Should we raise on non-null buffer? + // Raising on non-null buffer is handled in ValidateNulls. continue; default: continue; @@ -552,10 +553,17 @@ struct ValidateArrayImpl { } Status ValidateNulls(const DataType& type) { - if (type.storage_id() != Type::NA && data.null_count > 0 && - data.buffers[0] == nullptr) { - return Status::Invalid("Array of type ", type.ToString(), " has ", data.null_count, - " nulls but no null bitmap"); + if (!HasValidityBitmap(type.storage_id())) { + if (data.buffers[0] != nullptr) { + return Status::Invalid("Array of type ", type.ToString(), + " should not have a null bitmap, but one was present"); + } + } + if (type.storage_id() != Type::NA) { + if (data.null_count > 0 && data.buffers[0] == nullptr) { + return Status::Invalid("Array of type ", type.ToString(), " has ", + data.null_count, " nulls but no null bitmap"); + } } if (data.null_count > data.length) { return Status::Invalid("Null count exceeds array length"); @@ -714,13 +722,10 @@ struct ValidateArrayImpl { template Status ValidateListLike(const ListType& type) { - const ArrayData& values = *data.child_data[0]; - const Status child_valid = RecurseInto(values); - if (!child_valid.ok()) { - return Status::Invalid("List child array invalid: ", child_valid.ToString()); - } + RETURN_NOT_OK(RecurseIntoField(0, "List child array")); // First validate offsets, to make sure the accesses below are valid + const ArrayData& values = *data.child_data[0]; RETURN_NOT_OK(ValidateOffsetsAndSizes(type, values.offset + values.length)); // An empty list array can have 0 offsets @@ -774,10 +779,6 @@ struct ValidateArrayImpl { data.child_data.size()); } - if (data.buffers.size() > 0 && data.buffers[0] != nullptr) { - return Status::Invalid("Run end encoded array should not have a null bitmap."); - } - const auto& run_ends_data = data.child_data[0]; const auto& values_data = data.child_data[1]; @@ -787,15 +788,10 @@ struct ValidateArrayImpl { if (!values_data) { return Status::Invalid("Values array is null pointer"); } + // We must validate child array buffers are valid before making additional checks. - const Status run_ends_valid = RecurseInto(*run_ends_data); - if (!run_ends_valid.ok()) { - return Status::Invalid("Run ends array invalid: ", run_ends_valid.message()); - } - const Status values_valid = RecurseInto(*values_data); - if (!values_valid.ok()) { - return Status::Invalid("Values array invalid: ", values_valid.message()); - } + RETURN_NOT_OK(RecurseIntoField(0, "Run ends array")); + RETURN_NOT_OK(RecurseIntoField(1, "Values array")); RETURN_NOT_OK(ree_util::ValidateRunEndEncodedChildren( type, data.length, run_ends_data, values_data, data.GetNullCount(), data.offset)); diff --git a/cpp/src/arrow/extension_type.h b/cpp/src/arrow/extension_type.h index 38200f42c62e..709787720dc8 100644 --- a/cpp/src/arrow/extension_type.h +++ b/cpp/src/arrow/extension_type.h @@ -45,6 +45,9 @@ class ARROW_EXPORT ExtensionType : public DataType { /// \brief The type of array used to represent this extension type's data const std::shared_ptr& storage_type() const { return storage_type_; } + /// \brief Return a reference to the storage type + const DataType& storage_type_ref() const override { return *storage_type_; } + /// \brief Return the type category of the storage type Type::type storage_id() const override { return storage_type_->id(); } diff --git a/cpp/src/arrow/type.h b/cpp/src/arrow/type.h index 14a05c369e3b..68aafd400827 100644 --- a/cpp/src/arrow/type.h +++ b/cpp/src/arrow/type.h @@ -183,6 +183,9 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this, /// \brief Return the type category of the storage type virtual Type::type storage_id() const { return id_; } + /// \brief Return a reference to the storage type + virtual const DataType& storage_type_ref() const { return *this; } + /// \brief Returns the type's fixed byte width, if any. Returns -1 /// for non-fixed-width types, and should only be used for /// subclasses of FixedWidthType From d16279979d1302244629c28931d6714bae8445d0 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Wed, 18 Oct 2023 10:05:12 -0400 Subject: [PATCH 2/5] review comments, virtual storage_type --- cpp/src/arrow/array/array_test.cc | 23 +++++++++++++++-------- cpp/src/arrow/array/util.cc | 22 ++++++++++------------ cpp/src/arrow/array/validate.cc | 4 ++-- cpp/src/arrow/extension_type.h | 5 +---- cpp/src/arrow/testing/random.cc | 13 ++++++------- cpp/src/arrow/type.cc | 2 +- cpp/src/arrow/type.h | 3 +++ 7 files changed, 38 insertions(+), 34 deletions(-) diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index 1beda6e35189..ed1ff88f53dd 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -464,6 +464,7 @@ static std::vector> TestArrayUtilitiesAgainstTheseType list_view(utf8()), large_list_view(utf8()), dictionary(int32(), utf8()), + struct_({}), struct_({field("a", utf8()), field("b", int32())}), sparse_union(union_fields1, union_type_codes), sparse_union(union_fields2, union_type_codes), @@ -508,17 +509,23 @@ TEST_F(TestArray, TestMakeArrayOfNull) { } } - auto req = [](auto type) { return field("", std::move(type), /*nullable=*/false); }; + for (int64_t length : {0, 16}) { + ARROW_SCOPED_TRACE("length = ", length, " (required fields)"); - // union with no nullable fields cannot represent a null - ASSERT_RAISES(Invalid, MakeArrayOfNull(dense_union({req(int8())}), 1)); + auto req = [](auto type) { return field("", std::move(type), /*nullable=*/false); }; - // struct with no nullable fields has a top level bitmap and can mask them - ASSERT_OK_AND_ASSIGN(auto s, MakeArrayOfNull(struct_({req(int8())}), 1)); - ASSERT_OK(s->ValidateFull()); + // union with no nullable fields cannot represent a null + ASSERT_RAISES(Invalid, MakeArrayOfNull(dense_union({req(int8())}), length)); - ASSERT_OK_AND_ASSIGN(s, MakeArrayOfNull(struct_({req(dictionary(int8(), int8()))}), 1)); - ASSERT_OK(s->ValidateFull()); + // struct with no nullable fields has a top level bitmap and can mask them + ASSERT_OK_AND_ASSIGN(auto s, MakeArrayOfNull(struct_({req(int8())}), length)); + ASSERT_OK(s->ValidateFull()); + + // dictionary with non-nullable indices can use a 1-long dict of null + ASSERT_OK_AND_ASSIGN( + s, MakeArrayOfNull(struct_({req(dictionary(int8(), int8()))}), length)); + ASSERT_OK(s->ValidateFull()); + } } TEST_F(TestArray, TestMakeArrayOfNullUnion) { diff --git a/cpp/src/arrow/array/util.cc b/cpp/src/arrow/array/util.cc index 27489cae7199..d1b42aa2c5de 100644 --- a/cpp/src/arrow/array/util.cc +++ b/cpp/src/arrow/array/util.cc @@ -501,7 +501,7 @@ class NullArrayFactory { // For sparse unions, we create children with the same length as the parent. // // For dense unions, we create children with length 1 and have offsets which always - // refer to the first first slot from one child. + // refer to the first slot from one child. int64_t child_length = type.mode() == UnionMode::SPARSE ? length_ : 1; if (presizing_zero_buffer_) { @@ -545,8 +545,9 @@ class NullArrayFactory { } Status Visit(const DictionaryType& type) { - // The dictionary's indices are non-nullable; we can still create an array - // by creating a dictionary which contains a single null. + // If the dictionary's indices are non-nullable, we can still create an array + // by creating a dictionary which contains a single null. In this case all + // indices will be zero to refer to that null. int64_t dictionary_length = nullable_ ? 0 : 1; if (presizing_zero_buffer_) { @@ -596,9 +597,7 @@ class NullArrayFactory { return Status::OK(); } - ARROW_ASSIGN_OR_RAISE(auto out, - CreateRelated(type.storage_type(), nullable_, length_)); - *out_ = std::move(*out); + ARROW_ASSIGN_OR_RAISE(out_, CreateRelated(type.storage_type(), nullable_, length_)); out_->type = type_; return Status::OK(); } @@ -609,22 +608,21 @@ class NullArrayFactory { Result> Create() && { DCHECK(!presizing_zero_buffer_); - auto out = std::make_shared(); - out_ = out.get(); + out_ = std::make_shared(); out_->type = type_; out_->length = length_; out_->null_count = MayHaveDirectNulls() ? length_ : 0; out_->offset = 0; out_->buffers = {}; - out_->child_data.resize(type_->storage_type_ref().num_fields()); + out_->child_data.resize(type_->storage_type()->num_fields()); out_->dictionary = nullptr; RETURN_NOT_OK(VisitTypeInline(*type_, this)); - return out; + return std::move(out_); } Status CreateChild(int i, int64_t length) { DCHECK(!presizing_zero_buffer_); - const auto& field = type_->storage_type_ref().field(i); + const auto& field = type_->storage_type()->field(i); ARROW_ASSIGN_OR_RAISE(out_->child_data[i], CreateRelated(field->type(), field->nullable(), length)); return Status::OK(); @@ -645,7 +643,7 @@ class NullArrayFactory { const std::shared_ptr* zero_buffer_; MemoryPool* pool_; - ArrayData* out_; + std::shared_ptr out_; }; class RepeatedArrayFactory { diff --git a/cpp/src/arrow/array/validate.cc b/cpp/src/arrow/array/validate.cc index 46ce36239057..2e3fd2833471 100644 --- a/cpp/src/arrow/array/validate.cc +++ b/cpp/src/arrow/array/validate.cc @@ -304,7 +304,7 @@ struct ValidateArrayImpl { Status Visit(const StructType& type) { for (int i = 0; i < type.num_fields(); ++i) { // Validate child first, to catch nonsensical length / offset etc. - RETURN_NOT_OK(RecurseIntoField(0, "Struct child array #", i)); + RETURN_NOT_OK(RecurseIntoField(i, "Struct child array #", i)); const auto& field_data = *data.child_data[i]; if (field_data.length < data.length + data.offset) { @@ -451,7 +451,7 @@ struct ValidateArrayImpl { Status RecurseIntoField(int field_index, const FieldDescription&... description) { const auto& related_data = *data.child_data[field_index]; - if (!data.type->storage_type_ref().field(field_index)->nullable()) { + if (!data.type->storage_type()->field(field_index)->nullable()) { if (related_data.null_count != 0 && related_data.null_count != kUnknownNullCount) { return Status::Invalid(description..., " invalid: was non-nullable but had null count ", diff --git a/cpp/src/arrow/extension_type.h b/cpp/src/arrow/extension_type.h index 709787720dc8..b81ea3860b19 100644 --- a/cpp/src/arrow/extension_type.h +++ b/cpp/src/arrow/extension_type.h @@ -43,10 +43,7 @@ class ARROW_EXPORT ExtensionType : public DataType { static constexpr const char* type_name() { return "extension"; } /// \brief The type of array used to represent this extension type's data - const std::shared_ptr& storage_type() const { return storage_type_; } - - /// \brief Return a reference to the storage type - const DataType& storage_type_ref() const override { return *storage_type_; } + std::shared_ptr storage_type() const override { return storage_type_; } /// \brief Return the type category of the storage type Type::type storage_id() const override { return storage_type_->id(); } diff --git a/cpp/src/arrow/testing/random.cc b/cpp/src/arrow/testing/random.cc index db7a60e4400f..9f648c30bf01 100644 --- a/cpp/src/arrow/testing/random.cc +++ b/cpp/src/arrow/testing/random.cc @@ -47,6 +47,7 @@ #include "arrow/util/key_value_metadata.h" #include "arrow/util/logging_internal.h" #include "arrow/util/pcg_random.h" +#include "arrow/util/range.h" #include "arrow/util/string.h" #include "arrow/util/value_parsing.h" @@ -55,6 +56,7 @@ namespace arrow { using internal::checked_cast; using internal::checked_pointer_cast; using internal::ToChars; +using internal::Zip; using util::Float16; namespace random { @@ -1291,14 +1293,11 @@ std::shared_ptr RandomArrayGenerator::ArrayOf(const Field& field, int64_t case Type::type::STRUCT: { ArrayVector child_arrays(field.type()->num_fields()); - FieldVector child_fields(field.type()->num_fields()); - for (int i = 0; i < field.type()->num_fields(); i++) { - const auto& child_field = field.type()->field(i); - child_arrays[i] = ArrayOf(*child_field, length, alignment, memory_pool); - child_fields[i] = child_field; + for (auto [field, child_array] : Zip(field.type()->fields(), child_arrays)) { + child_array = ArrayOf(*field, length, alignment, memory_pool); } - return *StructArray::Make( - child_arrays, child_fields, + return std::make_shared( + field.type(), length, std::move(child_arrays), NullBitmap(length, null_probability, alignment, memory_pool)); } diff --git a/cpp/src/arrow/type.cc b/cpp/src/arrow/type.cc index e841f0918097..12a9b9e8e6f3 100644 --- a/cpp/src/arrow/type.cc +++ b/cpp/src/arrow/type.cc @@ -903,7 +903,7 @@ std::string Field::ToString(bool show_metadata) const { void PrintTo(const Field& field, std::ostream* os) { *os << field.ToString(); } -DataType::~DataType() {} +DataType::~DataType() = default; bool DataType::Equals(const DataType& other, bool check_metadata) const { return TypeEquals(*this, other, check_metadata); diff --git a/cpp/src/arrow/type.h b/cpp/src/arrow/type.h index 68aafd400827..d33b8fcd69aa 100644 --- a/cpp/src/arrow/type.h +++ b/cpp/src/arrow/type.h @@ -186,6 +186,9 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this, /// \brief Return a reference to the storage type virtual const DataType& storage_type_ref() const { return *this; } + /// \brief Return the storage type + virtual std::shared_ptr storage_type() const { return GetSharedPtr(); } + /// \brief Returns the type's fixed byte width, if any. Returns -1 /// for non-fixed-width types, and should only be used for /// subclasses of FixedWidthType From 896d034c6a76aee004d69b6bb5591ba495eb01cc Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Wed, 18 Oct 2023 10:32:14 -0400 Subject: [PATCH 3/5] cache null_count inside ValidateArrayImpl --- cpp/src/arrow/array/validate.cc | 65 ++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 25 deletions(-) diff --git a/cpp/src/arrow/array/validate.cc b/cpp/src/arrow/array/validate.cc index 2e3fd2833471..a5416a1595cb 100644 --- a/cpp/src/arrow/array/validate.cc +++ b/cpp/src/arrow/array/validate.cc @@ -107,6 +107,7 @@ struct BoundsChecker { struct ValidateArrayImpl { const ArrayData& data; const bool full_validation; + int64_t null_count = kUnknownNullCount; Status Validate() { if (data.type == nullptr) { @@ -406,7 +407,7 @@ struct ValidateArrayImpl { return Status::Invalid("Dictionary values must be non-null"); } // Validate dictionary - RETURN_NOT_OK(RecurseInto(*data.dictionary, "Dictionary array")); + RETURN_NOT_OK(RecurseInto(*data.dictionary, /*nullable=*/true, "Dictionary array")); // Validate indices RETURN_NOT_OK(ValidateWithType(*type.index_type())); @@ -451,21 +452,29 @@ struct ValidateArrayImpl { Status RecurseIntoField(int field_index, const FieldDescription&... description) { const auto& related_data = *data.child_data[field_index]; - if (!data.type->storage_type()->field(field_index)->nullable()) { - if (related_data.null_count != 0 && related_data.null_count != kUnknownNullCount) { - return Status::Invalid(description..., - " invalid: was non-nullable but had null count ", - related_data.null_count); - } - } - return RecurseInto(related_data, description...); + return RecurseInto(related_data, + data.type->storage_type()->field(field_index)->nullable(), + description...); } template - Status RecurseInto(const ArrayData& related_data, + Status RecurseInto(const ArrayData& related_data, bool nullable, const FieldDescription&... description) { - Status st = ValidateArrayImpl{related_data, full_validation}.Validate(); - if (!st.ok()) { + ValidateArrayImpl related_validator{related_data, full_validation}; + + if (!nullable) { + int64_t related_null_count = related_data.null_count; + if (full_validation && related_null_count == kUnknownNullCount) { + related_null_count = related_validator.GetNullCount(); + } + + if (related_null_count != 0 && related_null_count != kUnknownNullCount) { + return Status::Invalid(description..., " invalid: was non-nullable but had ", + related_null_count, " nulls"); + } + } + + if (Status st = related_validator.Validate(); !st.ok()) { return st.WithMessage(description..., " invalid: ", st.ToString()); } return Status::OK(); @@ -553,7 +562,7 @@ struct ValidateArrayImpl { } Status ValidateNulls(const DataType& type) { - if (!HasValidityBitmap(type.storage_id())) { + if (!may_have_validity_bitmap(type.storage_id())) { if (data.buffers[0] != nullptr) { return Status::Invalid("Array of type ", type.ToString(), " should not have a null bitmap, but one was present"); @@ -574,26 +583,32 @@ struct ValidateArrayImpl { if (full_validation) { if (data.null_count != kUnknownNullCount) { - int64_t actual_null_count; - if (may_have_validity_bitmap(data.type->id()) && data.buffers[0]) { - // Do not call GetNullCount() as it would also set the `null_count` member - actual_null_count = data.length - CountSetBits(data.buffers[0]->data(), - data.offset, data.length); - } else if (data.type->storage_id() == Type::NA) { - actual_null_count = data.length; - } else { - actual_null_count = 0; - } - if (actual_null_count != data.null_count) { + if (GetNullCount() != data.null_count) { return Status::Invalid("null_count value (", data.null_count, ") doesn't match actual number of nulls in array (", - actual_null_count, ")"); + null_count, ")"); } } } return Status::OK(); } + int64_t GetNullCount() { + if (null_count != kUnknownNullCount) return null_count; + + if (may_have_validity_bitmap(data.type->id()) && data.buffers[0]) { + // Do not call GetNullCount() as it would also set the `null_count` member + return null_count = data.length - + CountSetBits(data.buffers[0]->data(), data.offset, data.length); + } + + if (data.type->storage_id() == Type::NA) { + return null_count = data.length; + } + + return null_count = 0; + } + Status ValidateFixedWidthBuffers() { if (data.length > 0 && !IsBufferValid(1)) { return Status::Invalid("Missing values buffer in non-empty fixed-width array"); From dd2e93027af841309fb7867ef00b65287a9ede0d Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Thu, 16 Nov 2023 09:28:18 -0500 Subject: [PATCH 4/5] rebase cleanup --- cpp/src/arrow/array/util.cc | 6 +++++- cpp/src/arrow/array/validate.cc | 9 ++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/cpp/src/arrow/array/util.cc b/cpp/src/arrow/array/util.cc index d1b42aa2c5de..3c3f21060c54 100644 --- a/cpp/src/arrow/array/util.cc +++ b/cpp/src/arrow/array/util.cc @@ -439,7 +439,11 @@ class NullArrayFactory { } Status Visit(const BinaryViewType&) { - out_->buffers.resize(2, *zero_buffer_); + if (presizing_zero_buffer_) { + ZeroBufferMustBeAtLeast(sizeof(BinaryViewType::c_type) * length_); + return Status::OK(); + } + out_->buffers = {GetValidityBitmap(), *zero_buffer_}; return Status::OK(); } diff --git a/cpp/src/arrow/array/validate.cc b/cpp/src/arrow/array/validate.cc index a5416a1595cb..2f04bd7f55fe 100644 --- a/cpp/src/arrow/array/validate.cc +++ b/cpp/src/arrow/array/validate.cc @@ -776,14 +776,9 @@ struct ValidateArrayImpl { template Status ValidateListView(const ListViewType& type) { - const ArrayData& values = *data.child_data[0]; - const Status child_valid = RecurseInto(values); - if (!child_valid.ok()) { - return Status::Invalid("List-view child array is invalid: ", - child_valid.ToString()); - } + RETURN_NOT_OK(RecurseIntoField(0, "List-view child array")); // For list-views, sizes are validated together with offsets. - return ValidateOffsetsAndSizes(type, /*offset_limit=*/values.length); + return ValidateOffsetsAndSizes(type, /*offset_limit=*/data.child_data[0]->length); } template From 442342bb5405b36eec33e1b7999b968a5b30d842 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Wed, 13 Dec 2023 12:44:00 -0500 Subject: [PATCH 5/5] review comments --- cpp/src/arrow/array/array_test.cc | 7 ++++++- cpp/src/arrow/array/util.cc | 11 ++++++----- cpp/src/arrow/array/validate.cc | 14 +++++++------- cpp/src/arrow/type.h | 3 --- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/cpp/src/arrow/array/array_test.cc b/cpp/src/arrow/array/array_test.cc index ed1ff88f53dd..1d248ca4d6eb 100644 --- a/cpp/src/arrow/array/array_test.cc +++ b/cpp/src/arrow/array/array_test.cc @@ -515,7 +515,12 @@ TEST_F(TestArray, TestMakeArrayOfNull) { auto req = [](auto type) { return field("", std::move(type), /*nullable=*/false); }; // union with no nullable fields cannot represent a null - ASSERT_RAISES(Invalid, MakeArrayOfNull(dense_union({req(int8())}), length)); + ASSERT_RAISES(TypeError, MakeArrayOfNull(dense_union({req(int8())}), length)); + // run end encoded with non nullable child cannot represent a null + // (not directly constructible, but not invalid per Columnar.rst) + auto ree = run_end_encoded(int16(), utf8()); + const_cast(ree->fields())[1] = req(utf8()); + ASSERT_RAISES(TypeError, MakeArrayOfNull(ree, length)); // struct with no nullable fields has a top level bitmap and can mask them ASSERT_OK_AND_ASSIGN(auto s, MakeArrayOfNull(struct_({req(int8())}), length)); diff --git a/cpp/src/arrow/array/util.cc b/cpp/src/arrow/array/util.cc index 3c3f21060c54..acc8214e13c8 100644 --- a/cpp/src/arrow/array/util.cc +++ b/cpp/src/arrow/array/util.cc @@ -350,7 +350,7 @@ class NullArrayFactory { // - buffers = [] // - child_data = [nullptr] * type.num_fields() // - dictionary = nullptr - bool presizing_zero_buffer_; + const bool presizing_zero_buffer_; NullArrayFactory(const std::shared_ptr& type, bool nullable, int64_t length) : presizing_zero_buffer_{true}, @@ -497,8 +497,8 @@ class NullArrayFactory { for (auto [field, id] : Zip(type.fields(), type.type_codes())) { if (field->nullable()) return id; } - return Status::Invalid("Cannot produce an array of null ", type, - " because no child field is nullable"); + return Status::TypeError("Cannot produce an array of null ", type, + " because no child field is nullable"); } Status Visit(const UnionType& type) { @@ -539,6 +539,7 @@ class NullArrayFactory { } if (type.mode() == UnionMode::DENSE) { + // offsets out_->buffers.push_back(*zero_buffer_); } @@ -576,8 +577,8 @@ class NullArrayFactory { out_->buffers = {nullptr}; if (!type.field(1)->nullable()) { - return Status::Invalid("Cannot produce an array of null ", type, - " because the values field is not nullable"); + return Status::TypeError("Cannot produce an array of null ", type, + " because the values field is not nullable"); } std::shared_ptr run_ends, values; diff --git a/cpp/src/arrow/array/validate.cc b/cpp/src/arrow/array/validate.cc index 2f04bd7f55fe..6125048c8f8a 100644 --- a/cpp/src/arrow/array/validate.cc +++ b/cpp/src/arrow/array/validate.cc @@ -598,15 +598,15 @@ struct ValidateArrayImpl { if (may_have_validity_bitmap(data.type->id()) && data.buffers[0]) { // Do not call GetNullCount() as it would also set the `null_count` member - return null_count = data.length - - CountSetBits(data.buffers[0]->data(), data.offset, data.length); - } - - if (data.type->storage_id() == Type::NA) { - return null_count = data.length; + null_count = + data.length - CountSetBits(data.buffers[0]->data(), data.offset, data.length); + } else if (data.type->storage_id() == Type::NA) { + null_count = data.length; + } else { + null_count = 0; } - return null_count = 0; + return null_count; } Status ValidateFixedWidthBuffers() { diff --git a/cpp/src/arrow/type.h b/cpp/src/arrow/type.h index d33b8fcd69aa..e576cf8c7bc5 100644 --- a/cpp/src/arrow/type.h +++ b/cpp/src/arrow/type.h @@ -183,9 +183,6 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this, /// \brief Return the type category of the storage type virtual Type::type storage_id() const { return id_; } - /// \brief Return a reference to the storage type - virtual const DataType& storage_type_ref() const { return *this; } - /// \brief Return the storage type virtual std::shared_ptr storage_type() const { return GetSharedPtr(); }