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..1d248ca4d6eb 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), @@ -477,6 +478,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 +508,29 @@ TEST_F(TestArray, TestMakeArrayOfNull) { } } } + + for (int64_t length : {0, 16}) { + ARROW_SCOPED_TRACE("length = ", length, " (required fields)"); + + auto req = [](auto type) { return field("", std::move(type), /*nullable=*/false); }; + + // union with no nullable fields cannot represent a null + 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)); + 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 d97e2f7f85d9..acc8214e13c8 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,255 @@ 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(); - } - - 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(); - } + // 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 + const bool presizing_zero_buffer_; - 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) + : presizing_zero_buffer_{true}, + type_{type}, + nullable_{nullable}, + length_{length}, + zero_buffer_length_{MayHaveDirectNulls() ? bit_util::BytesForBits(length) : 0} {} - Status Visit(const RunEndEncodedType& type) { - // RunEndEncodedType has no buffers, only child arrays - buffer_length_ = 0; - return Status::OK(); - } + 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 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_); + if (presizing_zero_buffer_) { + ZeroBufferMustBeAtLeast(sizeof(BinaryViewType::c_type) * length_); + return Status::OK(); + } + out_->buffers = {GetValidityBitmap(), *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::TypeError("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 slot from one child. + int64_t child_length = type.mode() == UnionMode::SPARSE ? length_ : 1; + + if (presizing_zero_buffer_) { + // type codes + ZeroBufferMustBeAtLeast(length_); + + if (type.mode() == UnionMode::DENSE) { + // offsets + ZeroBufferMustBeAtLeast(sizeof(int32_t) * length_); + } + + for (const auto& child : type.fields()) { + ZeroBufferMustBeAtLeast(GetZeroBufferLength(child, child_length)); + } + return Status::OK(); + } - // First buffer is always null - out_->buffers[0] = nullptr; + // The validity bitmap is always absent for unions + out_->buffers = {nullptr}; - 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_); + // 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)); } - // 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_; - - child_length = 1; + // offsets + 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(); + // 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_) { + 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::TypeError("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 +591,19 @@ 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(out_, CreateRelated(type.storage_type(), nullable_, length_)); + out_->type = type_; return Status::OK(); } @@ -580,18 +611,44 @@ 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_); + 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()->num_fields()); + out_->dictionary = nullptr; + RETURN_NOT_OK(VisitTypeInline(*type_, this)); + return std::move(out_); + } + + Status CreateChild(int i, int64_t length) { + DCHECK(!presizing_zero_buffer_); + 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(); + } + + 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_; + + int64_t zero_buffer_length_ = 0; + + const std::shared_ptr* zero_buffer_; + MemoryPool* pool_; + std::shared_ptr out_; - std::shared_ptr buffer_; }; class RepeatedArrayFactory { @@ -898,8 +955,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..6125048c8f8a 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) { @@ -297,26 +298,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(i, "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 +326,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 +407,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, /*nullable=*/true, "Dictionary array")); // Validate indices RETURN_NOT_OK(ValidateWithType(*type.index_type())); @@ -465,9 +448,36 @@ 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]; + + return RecurseInto(related_data, + data.type->storage_type()->field(field_index)->nullable(), + description...); + } + + template + Status RecurseInto(const ArrayData& related_data, bool nullable, + const FieldDescription&... description) { + 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(); } Status ValidateLayout(const DataType& type) { @@ -528,7 +538,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 +562,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 (!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"); + } + } + 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"); @@ -566,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 + 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; + } + Status ValidateFixedWidthBuffers() { if (data.length > 0 && !IsBufferValid(1)) { return Status::Invalid("Missing values buffer in non-empty fixed-width array"); @@ -714,13 +737,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 @@ -756,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 @@ -774,10 +789,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 +798,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..b81ea3860b19 100644 --- a/cpp/src/arrow/extension_type.h +++ b/cpp/src/arrow/extension_type.h @@ -43,7 +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_; } + 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 14a05c369e3b..e576cf8c7bc5 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 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