diff --git a/cpp/build-support/lint_cpp_cli.py b/cpp/build-support/lint_cpp_cli.py index 78f3eea808a..81fba673983 100644 --- a/cpp/build-support/lint_cpp_cli.py +++ b/cpp/build-support/lint_cpp_cli.py @@ -28,6 +28,7 @@ _STRIP_COMMENT_REGEX = re.compile('(.+)?(?=//)') +_NULLPTR_REGEX = re.compile(r'.*\bnullptr\b.*') def _strip_comments(line): @@ -41,7 +42,7 @@ def _strip_comments(line): def lint_file(path): fail_rules = [ (lambda x: '' in x, 'Uses '), - (lambda x: 'nullptr' in x, 'Uses nullptr') + (lambda x: re.match(_NULLPTR_REGEX, x), 'Uses nullptr') ] with open(path) as f: diff --git a/cpp/src/arrow/adapters/orc/adapter.cc b/cpp/src/arrow/adapters/orc/adapter.cc index 68041957304..1ad22790ead 100644 --- a/cpp/src/arrow/adapters/orc/adapter.cc +++ b/cpp/src/arrow/adapters/orc/adapter.cc @@ -39,6 +39,7 @@ #include "arrow/util/bit-util.h" #include "arrow/util/checked_cast.h" #include "arrow/util/decimal.h" +#include "arrow/util/lazy.h" #include "arrow/util/macros.h" #include "arrow/util/visibility.h" @@ -521,18 +522,17 @@ class ORCFileReader::Impl { if (length == 0) { return Status::OK(); } - int64_t start = builder->length(); const uint8_t* valid_bytes = nullptr; if (batch->hasNulls) { valid_bytes = reinterpret_cast(batch->notNull.data()) + offset; } - RETURN_NOT_OK(builder->AppendNulls(valid_bytes, length)); - const source_type* source = batch->data.data() + offset; - target_type* target = reinterpret_cast(builder->data()->mutable_data()); + auto cast_iter = internal::MakeLazyRange( + [&source](int64_t index) { return static_cast(source[index]); }, + length); - std::copy(source, source + length, target + start); + RETURN_NOT_OK(builder->AppendValues(cast_iter.begin(), cast_iter.end(), valid_bytes)); return Status::OK(); } @@ -545,24 +545,18 @@ class ORCFileReader::Impl { if (length == 0) { return Status::OK(); } - int64_t start = builder->length(); const uint8_t* valid_bytes = nullptr; if (batch->hasNulls) { valid_bytes = reinterpret_cast(batch->notNull.data()) + offset; } - RETURN_NOT_OK(builder->AppendNulls(valid_bytes, length)); - const int64_t* source = batch->data.data() + offset; - uint8_t* target = reinterpret_cast(builder->data()->mutable_data()); - for (int64_t i = 0; i < length; i++) { - if (source[i]) { - BitUtil::SetBit(target, start + i); - } else { - BitUtil::ClearBit(target, start + i); - } - } + auto cast_iter = internal::MakeLazyRange( + [&source](int64_t index) { return static_cast(source[index]); }, length); + + RETURN_NOT_OK(builder->AppendValues(cast_iter.begin(), cast_iter.end(), valid_bytes)); + return Status::OK(); } @@ -574,23 +568,23 @@ class ORCFileReader::Impl { if (length == 0) { return Status::OK(); } - int64_t start = builder->length(); const uint8_t* valid_bytes = nullptr; if (batch->hasNulls) { valid_bytes = reinterpret_cast(batch->notNull.data()) + offset; } - RETURN_NOT_OK(builder->AppendNulls(valid_bytes, length)); const int64_t* seconds = batch->data.data() + offset; const int64_t* nanos = batch->nanoseconds.data() + offset; - int64_t* target = reinterpret_cast(builder->data()->mutable_data()); - for (int64_t i = 0; i < length; i++) { - // TODO: boundscheck this, as ORC supports higher resolution timestamps - // than arrow for nanosecond resolution - target[start + i] = seconds[i] * kOneSecondNanos + nanos[i]; - } + auto transform_timestamp = [seconds, nanos](int64_t index) { + return seconds[index] * kOneSecondNanos + nanos[index]; + }; + + auto transform_range = internal::MakeLazyRange(transform_timestamp, length); + + RETURN_NOT_OK(builder->AppendValues(transform_range.begin(), transform_range.end(), + valid_bytes)); return Status::OK(); } diff --git a/cpp/src/arrow/array-test.cc b/cpp/src/arrow/array-test.cc index 589f149fc65..44270d1fc2f 100644 --- a/cpp/src/arrow/array-test.cc +++ b/cpp/src/arrow/array-test.cc @@ -35,6 +35,7 @@ #include "arrow/type_traits.h" #include "arrow/util/checked_cast.h" #include "arrow/util/decimal.h" +#include "arrow/util/lazy.h" namespace arrow { @@ -241,14 +242,17 @@ TEST_F(TestArray, TestIsNullIsValidNoNulls) { } TEST_F(TestArray, BuildLargeInMemoryArray) { +#ifdef NDEBUG const int64_t length = static_cast(std::numeric_limits::max()) + 1; +#else + // use a smaller size since the insert function isn't optimized properly on debug and + // the test takes a long time to complete + const int64_t length = 2 << 24; +#endif BooleanBuilder builder; - ASSERT_OK(builder.Reserve(length)); - - // Advance does not write to data, see docstring - ASSERT_OK(builder.Advance(length)); - memset(builder.data()->mutable_data(), 0, BitUtil::BytesForBits(length)); + std::vector zeros(length); + ASSERT_OK(builder.AppendValues(zeros)); std::shared_ptr result; FinishAndCheckPadding(&builder, &result); @@ -265,10 +269,10 @@ TEST_F(TestBuilder, TestReserve) { UInt8Builder builder(pool_); ASSERT_OK(builder.Init(10)); - ASSERT_EQ(2, builder.null_bitmap()->size()); + ASSERT_EQ(10, builder.capacity()); ASSERT_OK(builder.Reserve(30)); - ASSERT_EQ(4, builder.null_bitmap()->size()); + ASSERT_EQ(BitUtil::NextPower2(30), builder.capacity()); } template @@ -328,7 +332,6 @@ class TestPrimitiveBuilder : public TestBuilder { ASSERT_EQ(0, builder->length()); ASSERT_EQ(0, builder->capacity()); ASSERT_EQ(0, builder->null_count()); - ASSERT_EQ(nullptr, builder->data()); ASSERT_EQ(ex_null_count, result->null_count()); ASSERT_TRUE(result->Equals(*expected)); @@ -468,7 +471,6 @@ void TestPrimitiveBuilder::Check(const std::unique_ptr ASSERT_EQ(0, builder->length()); ASSERT_EQ(0, builder->capacity()); ASSERT_EQ(0, builder->null_count()); - ASSERT_EQ(nullptr, builder->data()); } typedef ::testing::Typesbuilder_->Reserve(n)); ASSERT_EQ(BitUtil::NextPower2(n), this->builder_->capacity()); - ASSERT_EQ(BitUtil::NextPower2(TypeTraits::bytes_required(n)), - this->builder_->data()->size()); // unsure if this should go in all builder classes ASSERT_EQ(0, this->builder_->num_children()); @@ -711,6 +709,109 @@ TYPED_TEST(TestPrimitiveBuilder, TestAppendValues) { this->Check(this->builder_nn_, false); } +TYPED_TEST(TestPrimitiveBuilder, TestAppendValuesIter) { + int64_t size = 10000; + this->RandomData(size); + + ASSERT_OK(this->builder_->AppendValues(this->draws_.begin(), this->draws_.end(), + this->valid_bytes_.begin())); + ASSERT_OK(this->builder_nn_->AppendValues(this->draws_.begin(), this->draws_.end())); + + ASSERT_EQ(size, this->builder_->length()); + ASSERT_EQ(BitUtil::NextPower2(size), this->builder_->capacity()); + + this->Check(this->builder_, true); + this->Check(this->builder_nn_, false); +} + +TYPED_TEST(TestPrimitiveBuilder, TestAppendValuesIterNullValid) { + int64_t size = 10000; + this->RandomData(size); + + ASSERT_OK(this->builder_nn_->AppendValues(this->draws_.begin(), + this->draws_.begin() + size / 2, + static_cast(nullptr))); + + ASSERT_EQ(BitUtil::NextPower2(size / 2), this->builder_nn_->capacity()); + + ASSERT_OK(this->builder_nn_->AppendValues(this->draws_.begin() + size / 2, + this->draws_.end(), + static_cast(nullptr))); + + this->Check(this->builder_nn_, false); +} + +TYPED_TEST(TestPrimitiveBuilder, TestAppendValuesLazyIter) { + DECL_T(); + + int64_t size = 10000; + this->RandomData(size); + + auto& draws = this->draws_; + auto& valid_bytes = this->valid_bytes_; + + auto doubler = [&draws](int64_t index) { return draws[index] * 2; }; + auto lazy_iter = internal::MakeLazyRange(doubler, size); + + ASSERT_OK(this->builder_->AppendValues(lazy_iter.begin(), lazy_iter.end(), + valid_bytes.begin())); + + std::vector doubled; + transform(draws.begin(), draws.end(), back_inserter(doubled), + [](T in) { return in * 2; }); + + std::shared_ptr result; + FinishAndCheckPadding(this->builder_.get(), &result); + + std::shared_ptr expected; + ASSERT_OK( + this->builder_->AppendValues(doubled.data(), doubled.size(), valid_bytes.data())); + FinishAndCheckPadding(this->builder_.get(), &expected); + + ASSERT_TRUE(expected->Equals(result)); +} + +TYPED_TEST(TestPrimitiveBuilder, TestAppendValuesIterConverted) { + DECL_T(); + // find type we can safely convert the tested values to and from + using conversion_type = + typename std::conditional::value, double, + typename std::conditional::value, + uint64_t, int64_t>::type>::type; + + int64_t size = 10000; + this->RandomData(size); + + // append convertible values + vector draws_converted(this->draws_.begin(), this->draws_.end()); + vector valid_bytes_converted(this->valid_bytes_.begin(), + this->valid_bytes_.end()); + + auto cast_values = internal::MakeLazyRange( + [&draws_converted](int64_t index) { + return static_cast(draws_converted[index]); + }, + size); + auto cast_valid = internal::MakeLazyRange( + [&valid_bytes_converted](int64_t index) { + return static_cast(valid_bytes_converted[index]); + }, + size); + + ASSERT_OK(this->builder_->AppendValues(cast_values.begin(), cast_values.end(), + cast_valid.begin())); + ASSERT_OK(this->builder_nn_->AppendValues(cast_values.begin(), cast_values.end())); + + ASSERT_EQ(size, this->builder_->length()); + ASSERT_EQ(BitUtil::NextPower2(size), this->builder_->capacity()); + + ASSERT_EQ(size, this->builder_->length()); + ASSERT_EQ(BitUtil::NextPower2(size), this->builder_->capacity()); + + this->Check(this->builder_, true); + this->Check(this->builder_nn_, false); +} + TYPED_TEST(TestPrimitiveBuilder, TestZeroPadded) { DECL_T(); @@ -789,15 +890,10 @@ TYPED_TEST(TestPrimitiveBuilder, TestAdvance) { } TYPED_TEST(TestPrimitiveBuilder, TestResize) { - DECL_TYPE(); - int64_t cap = kMinBuilderCapacity * 2; ASSERT_OK(this->builder_->Reserve(cap)); ASSERT_EQ(cap, this->builder_->capacity()); - - ASSERT_EQ(TypeTraits::bytes_required(cap), this->builder_->data()->size()); - ASSERT_EQ(BitUtil::BytesForBits(cap), this->builder_->null_bitmap()->size()); } TYPED_TEST(TestPrimitiveBuilder, TestReserve) { diff --git a/cpp/src/arrow/buffer-test.cc b/cpp/src/arrow/buffer-test.cc index cb9399e0aa2..60708201359 100644 --- a/cpp/src/arrow/buffer-test.cc +++ b/cpp/src/arrow/buffer-test.cc @@ -178,6 +178,9 @@ TEST(TestBuffer, Copy) { Buffer expected(data + 5, 4); ASSERT_TRUE(out->Equals(expected)); + // assert the padding is zeroed + std::vector zeros(out->capacity() - out->size()); + ASSERT_EQ(0, memcmp(out->data() + out->size(), zeros.data(), zeros.size())); } TEST(TestBuffer, SliceBuffer) { diff --git a/cpp/src/arrow/buffer.cc b/cpp/src/arrow/buffer.cc index b87aa4072f1..fde20ff824e 100644 --- a/cpp/src/arrow/buffer.cc +++ b/cpp/src/arrow/buffer.cc @@ -32,8 +32,8 @@ Status Buffer::Copy(const int64_t start, const int64_t nbytes, MemoryPool* pool, DCHECK_LT(start, size_); DCHECK_LE(nbytes, size_ - start); - auto new_buffer = std::make_shared(pool); - RETURN_NOT_OK(new_buffer->Resize(nbytes)); + std::shared_ptr new_buffer; + RETURN_NOT_OK(AllocateResizableBuffer(pool, nbytes, &new_buffer)); std::memcpy(new_buffer->mutable_data(), data() + start, static_cast(nbytes)); @@ -123,6 +123,7 @@ Status PoolBuffer::Resize(const int64_t new_size, bool shrink_to_fit) { } } size_ = new_size; + return Status::OK(); } @@ -142,6 +143,7 @@ Status AllocateBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out) { auto buffer = std::make_shared(pool); RETURN_NOT_OK(buffer->Resize(size)); + buffer->ZeroPadding(); *out = buffer; return Status::OK(); } @@ -154,6 +156,7 @@ Status AllocateResizableBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out) { auto buffer = std::make_shared(pool); RETURN_NOT_OK(buffer->Resize(size)); + buffer->ZeroPadding(); *out = buffer; return Status::OK(); } diff --git a/cpp/src/arrow/buffer.h b/cpp/src/arrow/buffer.h index f7fee061c5f..23fbdfacf0f 100644 --- a/cpp/src/arrow/buffer.h +++ b/cpp/src/arrow/buffer.h @@ -101,6 +101,14 @@ class ARROW_EXPORT Buffer { Status Copy(const int64_t start, const int64_t nbytes, std::shared_ptr* out) const; + /// Zero bytes in padding, i.e. bytes between size_ and capacity_. + void ZeroPadding() { +#ifndef NDEBUG + CheckMutable(); +#endif + memset(mutable_data_ + size_, 0, static_cast(capacity_ - size_)); + } + /// \brief Construct a new buffer that owns its memory from a std::string /// /// \param[in] data a std::string object @@ -188,6 +196,7 @@ class ARROW_EXPORT ResizableBuffer : public MutableBuffer { /// Change buffer reported size to indicated size, allocating memory if /// necessary. This will ensure that the capacity of the buffer is a multiple /// of 64 bytes as defined in Layout.md. + /// Consider using ZeroPadding afterwards, in case you return buffer to a reader. /// /// @param shrink_to_fit On deactivating this option, the capacity of the Buffer won't /// decrease. @@ -195,7 +204,7 @@ class ARROW_EXPORT ResizableBuffer : public MutableBuffer { /// Ensure that buffer has enough memory allocated to fit the indicated /// capacity (and meets the 64 byte padding requirement in Layout.md). - /// It does not change buffer's reported size. + /// It does not change buffer's reported size and doesn't zero the padding. virtual Status Reserve(const int64_t new_capacity) = 0; template @@ -365,7 +374,7 @@ class ARROW_EXPORT TypedBufferBuilder : public BufferBuilder { int64_t capacity() const { return capacity_ / sizeof(T); } }; -/// \brief Allocate a fixed-size mutable buffer from a memory pool +/// \brief Allocate a fixed size mutable buffer from a memory pool, zero its padding. /// /// \param[in] pool a memory pool /// \param[in] size size of buffer to allocate @@ -384,7 +393,7 @@ Status AllocateBuffer(MemoryPool* pool, const int64_t size, std::shared_ptr* out); -/// \brief Allocate a resizeable buffer from a memory pool +/// \brief Allocate a resizeable buffer from a memory pool, zero its padding. /// /// \param[in] pool a memory pool /// \param[in] size size of buffer to allocate diff --git a/cpp/src/arrow/builder.cc b/cpp/src/arrow/builder.cc index d8498ceb901..1b779452635 100644 --- a/cpp/src/arrow/builder.cc +++ b/cpp/src/arrow/builder.cc @@ -43,6 +43,24 @@ namespace arrow { using internal::AdaptiveIntBuilderBase; +namespace { + +Status TrimBuffer(const int64_t bytes_filled, ResizableBuffer* buffer) { + if (buffer) { + if (bytes_filled < buffer->size()) { + // Trim buffer + RETURN_NOT_OK(buffer->Resize(bytes_filled)); + } + // zero the padding + buffer->ZeroPadding(); + } else { + DCHECK_EQ(bytes_filled, 0); + } + return Status::OK(); +} + +} // namespace + Status ArrayBuilder::AppendToBitmap(bool is_valid) { if (length_ == capacity_) { // If the capacity was not already a multiple of 2, do so here @@ -66,11 +84,13 @@ Status ArrayBuilder::Init(int64_t capacity) { int64_t to_alloc = BitUtil::BytesForBits(capacity); null_bitmap_ = std::make_shared(pool_); RETURN_NOT_OK(null_bitmap_->Resize(to_alloc)); + // Buffers might allocate more then necessary to satisfy padding requirements const int64_t byte_capacity = null_bitmap_->capacity(); capacity_ = capacity; null_bitmap_data_ = null_bitmap_->mutable_data(); memset(null_bitmap_data_, 0, static_cast(byte_capacity)); + return Status::OK(); } @@ -132,64 +152,11 @@ void ArrayBuilder::UnsafeAppendToBitmap(const uint8_t* valid_bytes, int64_t leng UnsafeSetNotNull(length); return; } - - int64_t byte_offset = length_ / 8; - int64_t bit_offset = length_ % 8; - uint8_t bitset = null_bitmap_data_[byte_offset]; - - for (int64_t i = 0; i < length; ++i) { - if (bit_offset == 8) { - bit_offset = 0; - null_bitmap_data_[byte_offset] = bitset; - byte_offset++; - // TODO: Except for the last byte, this shouldn't be needed - bitset = null_bitmap_data_[byte_offset]; - } - - if (valid_bytes[i]) { - bitset |= BitUtil::kBitmask[bit_offset]; - } else { - bitset &= BitUtil::kFlippedBitmask[bit_offset]; - ++null_count_; - } - - bit_offset++; - } - if (bit_offset != 0) { - null_bitmap_data_[byte_offset] = bitset; - } - length_ += length; + UnsafeAppendToBitmap(valid_bytes, valid_bytes + length); } void ArrayBuilder::UnsafeAppendToBitmap(const std::vector& is_valid) { - int64_t byte_offset = length_ / 8; - int64_t bit_offset = length_ % 8; - uint8_t bitset = null_bitmap_data_[byte_offset]; - - const int64_t length = static_cast(is_valid.size()); - - for (int64_t i = 0; i < length; ++i) { - if (bit_offset == 8) { - bit_offset = 0; - null_bitmap_data_[byte_offset] = bitset; - byte_offset++; - // TODO: Except for the last byte, this shouldn't be needed - bitset = null_bitmap_data_[byte_offset]; - } - - if (is_valid[i]) { - bitset |= BitUtil::kBitmask[bit_offset]; - } else { - bitset &= BitUtil::kFlippedBitmask[bit_offset]; - ++null_count_; - } - - bit_offset++; - } - if (bit_offset != 0) { - null_bitmap_data_[byte_offset] = bitset; - } - length_ += length; + UnsafeAppendToBitmap(is_valid.begin(), is_valid.end()); } void ArrayBuilder::UnsafeSetNotNull(int64_t length) { @@ -242,6 +209,12 @@ Status PrimitiveBuilder::Init(int64_t capacity) { return Status::OK(); } +template +void PrimitiveBuilder::Reset() { + data_.reset(); + raw_data_ = nullptr; +} + template Status PrimitiveBuilder::Resize(int64_t capacity) { // XXX: Set floor size for now @@ -272,7 +245,6 @@ Status PrimitiveBuilder::AppendValues(const value_type* values, int64_t lengt // length_ is update by these ArrayBuilder::UnsafeAppendToBitmap(valid_bytes, length); - return Status::OK(); } @@ -295,7 +267,6 @@ Status PrimitiveBuilder::AppendValues(const value_type* values, int64_t lengt // length_ is update by these ArrayBuilder::UnsafeAppendToBitmap(is_valid); - return Status::OK(); } @@ -327,24 +298,6 @@ Status PrimitiveBuilder::Append(const std::vector& values) { return AppendValues(values); } -namespace { - -Status TrimBuffer(const int64_t bytes_filled, ResizableBuffer* buffer) { - if (buffer) { - if (bytes_filled < buffer->size()) { - // Trim buffer - RETURN_NOT_OK(buffer->Resize(bytes_filled)); - } - // zero the padding - memset(buffer->mutable_data() + bytes_filled, 0, buffer->capacity() - bytes_filled); - } else { - DCHECK_EQ(bytes_filled, 0); - } - return Status::OK(); -} - -} // namespace - template Status PrimitiveBuilder::FinishInternal(std::shared_ptr* out) { RETURN_NOT_OK(TrimBuffer(BitUtil::BytesForBits(length_), null_bitmap_.get())); @@ -354,6 +307,7 @@ Status PrimitiveBuilder::FinishInternal(std::shared_ptr* out) { data_ = null_bitmap_ = nullptr; capacity_ = length_ = null_count_ = 0; + return Status::OK(); } @@ -388,6 +342,12 @@ Status AdaptiveIntBuilderBase::Init(int64_t capacity) { return Status::OK(); } +void AdaptiveIntBuilderBase::Reset() { + ArrayBuilder::Reset(); + data_.reset(); + raw_data_ = nullptr; +} + Status AdaptiveIntBuilderBase::Resize(int64_t capacity) { // XXX: Set floor size for now if (capacity < kMinBuilderCapacity) { @@ -490,7 +450,6 @@ Status AdaptiveIntBuilder::AppendValues(const int64_t* values, int64_t length, // length_ is update by these ArrayBuilder::UnsafeAppendToBitmap(valid_bytes, length); - return Status::OK(); } @@ -649,7 +608,6 @@ Status AdaptiveUIntBuilder::AppendValues(const uint64_t* values, int64_t length, // length_ is update by these ArrayBuilder::UnsafeAppendToBitmap(valid_bytes, length); - return Status::OK(); } @@ -750,6 +708,12 @@ Status BooleanBuilder::Init(int64_t capacity) { return Status::OK(); } +void BooleanBuilder::Reset() { + ArrayBuilder::Reset(); + data_.reset(); + raw_data_ = nullptr; +} + Status BooleanBuilder::Resize(int64_t capacity) { // XXX: Set floor size for now if (capacity < kMinBuilderCapacity) { @@ -904,8 +868,7 @@ struct DictionaryHashHelper> { // Get the dictionary value at the given builder index static Scalar GetDictionaryValue(const Builder& builder, int64_t index) { - const Scalar* data = reinterpret_cast(builder.data()->data()); - return data[index]; + return builder.GetValue(index); } // Compute the hash of a scalar value @@ -924,10 +887,10 @@ struct DictionaryHashHelper> { } // Append another builder's contents to the builder - static Status AppendBuilder(Builder& builder, const Builder& source_builder) { - return builder.AppendValues( - reinterpret_cast(source_builder.data()->data()), - source_builder.length(), nullptr); + static Status AppendArray(Builder& builder, const Array& in_array) { + const auto& array = checked_cast(in_array); + return builder.AppendValues(reinterpret_cast(array.values()->data()), + array.length(), nullptr); } }; @@ -958,10 +921,11 @@ struct DictionaryHashHelper> { return builder.Append(value.ptr_, value.length_); } - static Status AppendBuilder(Builder& builder, const Builder& source_builder) { - for (uint64_t index = 0, limit = source_builder.length(); index < limit; ++index) { + static Status AppendArray(Builder& builder, const Array& in_array) { + const auto& array = checked_cast(in_array); + for (uint64_t index = 0, limit = array.length(); index < limit; ++index) { int32_t length; - const uint8_t* ptr = source_builder.GetValue(index, &length); + const uint8_t* ptr = array.GetValue(index, &length); RETURN_NOT_OK(builder.Append(ptr, length)); } return Status::OK(); @@ -992,9 +956,10 @@ struct DictionaryHashHelper> { return builder.Append(value); } - static Status AppendBuilder(Builder& builder, const Builder& source_builder) { - for (uint64_t index = 0, limit = source_builder.length(); index < limit; ++index) { - const Scalar value = GetDictionaryValue(source_builder, index); + static Status AppendArray(Builder& builder, const Array& in_array) { + const auto& array = checked_cast(in_array); + for (uint64_t index = 0, limit = array.length(); index < limit; ++index) { + const Scalar value = array.GetValue(index); RETURN_NOT_OK(builder.Append(value)); } return Status::OK(); @@ -1025,8 +990,6 @@ DictionaryBuilder::DictionaryBuilder(const std::shared_ptr& } } -DictionaryBuilder::~DictionaryBuilder() {} - template <> DictionaryBuilder::DictionaryBuilder( const std::shared_ptr& type, MemoryPool* pool) @@ -1062,6 +1025,13 @@ Status DictionaryBuilder::Init(int64_t elements) { return values_builder_.Init(elements); } +template +void DictionaryBuilder::Reset() { + dict_builder_.Reset(); + overflow_dict_builder_.Reset(); + values_builder_.Reset(); +} + template Status DictionaryBuilder::Resize(int64_t capacity) { if (capacity < kMinBuilderCapacity) { @@ -1208,20 +1178,21 @@ Status DictionaryBuilder::DoubleTableSize() { template Status DictionaryBuilder::FinishInternal(std::shared_ptr* out) { + std::shared_ptr dictionary; entry_id_offset_ += dict_builder_.length(); + RETURN_NOT_OK(dict_builder_.Finish(&dictionary)); + // Store current dict entries for further uses of this DictionaryBuilder RETURN_NOT_OK( - DictionaryHashHelper::AppendBuilder(overflow_dict_builder_, dict_builder_)); + DictionaryHashHelper::AppendArray(overflow_dict_builder_, *dictionary)); DCHECK_EQ(entry_id_offset_, overflow_dict_builder_.length()); - std::shared_ptr dictionary; - RETURN_NOT_OK(dict_builder_.Finish(&dictionary)); - RETURN_NOT_OK(values_builder_.FinishInternal(out)); (*out)->type = std::make_shared((*out)->type, dictionary); - RETURN_NOT_OK(dict_builder_.Init(capacity_)); - RETURN_NOT_OK(values_builder_.Init(capacity_)); + dict_builder_.Reset(); + values_builder_.Reset(); + return Status::OK(); } @@ -1230,6 +1201,7 @@ Status DictionaryBuilder::FinishInternal(std::shared_ptr* o RETURN_NOT_OK(values_builder_.FinishInternal(out)); (*out)->type = std::make_shared((*out)->type, dictionary); + return Status::OK(); } @@ -1293,6 +1265,7 @@ Status Decimal128Builder::FinishInternal(std::shared_ptr* out) { RETURN_NOT_OK(byte_builder_.Finish(&data)); *out = ArrayData::Make(type_, length_, {null_bitmap_, data}, null_count_); + return Status::OK(); } @@ -1374,7 +1347,9 @@ Status ListBuilder::FinishInternal(std::shared_ptr* out) { void ListBuilder::Reset() { ArrayBuilder::Reset(); - values_ = nullptr; + values_.reset(); + offsets_builder_.Reset(); + value_builder_->Reset(); } ArrayBuilder* ListBuilder::value_builder() const { @@ -1430,6 +1405,7 @@ Status BinaryBuilder::Append(const uint8_t* value, int32_t length) { RETURN_NOT_OK(Reserve(1)); RETURN_NOT_OK(AppendNextOffset()); RETURN_NOT_OK(value_data_builder_.Append(value, length)); + UnsafeAppendToBitmap(true); return Status::OK(); } @@ -1437,6 +1413,7 @@ Status BinaryBuilder::Append(const uint8_t* value, int32_t length) { Status BinaryBuilder::AppendNull() { RETURN_NOT_OK(AppendNextOffset()); RETURN_NOT_OK(Reserve(1)); + UnsafeAppendToBitmap(false); return Status::OK(); } @@ -1499,6 +1476,7 @@ Status StringBuilder::AppendValues(const std::vector& values, reinterpret_cast(values[i].data()), values[i].size())); } } + UnsafeAppendToBitmap(valid_bytes, values.size()); return Status::OK(); } @@ -1607,6 +1585,11 @@ Status FixedSizeBinaryBuilder::Init(int64_t elements) { return byte_builder_.Resize(elements * byte_width_); } +void FixedSizeBinaryBuilder::Reset() { + ArrayBuilder::Reset(); + byte_builder_.Reset(); +} + Status FixedSizeBinaryBuilder::Resize(int64_t capacity) { RETURN_NOT_OK(byte_builder_.Resize(capacity * byte_width_)); return ArrayBuilder::Resize(capacity); @@ -1637,6 +1620,12 @@ StructBuilder::StructBuilder(const std::shared_ptr& type, MemoryPool* field_builders_ = std::move(field_builders); } +void StructBuilder::Reset() { + ArrayBuilder::Reset(); + for (const auto& field_builder : field_builders_) { + field_builder->Reset(); + } +} Status StructBuilder::FinishInternal(std::shared_ptr* out) { RETURN_NOT_OK(TrimBuffer(BitUtil::BytesForBits(length_), null_bitmap_.get())); *out = ArrayData::Make(type_, length_, {null_bitmap_}, null_count_); diff --git a/cpp/src/arrow/builder.h b/cpp/src/arrow/builder.h index 7a4704d8e06..eeff12bb2a3 100644 --- a/cpp/src/arrow/builder.h +++ b/cpp/src/arrow/builder.h @@ -18,6 +18,7 @@ #ifndef ARROW_BUILDER_H #define ARROW_BUILDER_H +#include #include #include #include @@ -100,6 +101,9 @@ class ARROW_EXPORT ArrayBuilder { /// method as well. virtual Status Resize(int64_t new_bits); + /// Reset the builder. + virtual void Reset(); + /// Ensures there is enough space for adding the number of elements by checking /// capacity and calling Resize if necessary. Status Reserve(int64_t elements); @@ -109,6 +113,7 @@ class ARROW_EXPORT ArrayBuilder { /// this function responsibly. Status Advance(int64_t elements); + ARROW_DEPRECATED("Use Finish instead") std::shared_ptr null_bitmap() const { return null_bitmap_; } /// \brief Return result of builder as an internal generic ArrayData @@ -129,7 +134,7 @@ class ARROW_EXPORT ArrayBuilder { // Unsafe operations (don't check capacity/don't resize) - // Append to null bitmap. + // Append to null bitmap, update the length void UnsafeAppendToBitmap(bool is_valid) { if (is_valid) { BitUtil::SetBit(null_bitmap_data_, length_); @@ -139,6 +144,38 @@ class ARROW_EXPORT ArrayBuilder { ++length_; } + template + void UnsafeAppendToBitmap(const IterType& begin, const IterType& end) { + int64_t byte_offset = length_ / 8; + int64_t bit_offset = length_ % 8; + uint8_t bitset = null_bitmap_data_[byte_offset]; + + for (auto iter = begin; iter != end; ++iter) { + if (bit_offset == 8) { + bit_offset = 0; + null_bitmap_data_[byte_offset] = bitset; + byte_offset++; + // TODO: Except for the last byte, this shouldn't be needed + bitset = null_bitmap_data_[byte_offset]; + } + + if (*iter) { + bitset |= BitUtil::kBitmask[bit_offset]; + } else { + bitset &= BitUtil::kFlippedBitmask[bit_offset]; + ++null_count_; + } + + bit_offset++; + } + + if (bit_offset != 0) { + null_bitmap_data_[byte_offset] = bitset; + } + + length_ += std::distance(begin, end); + } + protected: ArrayBuilder() {} @@ -157,8 +194,6 @@ class ARROW_EXPORT ArrayBuilder { // Child value array builders. These are owned by this class std::vector> children_; - void Reset(); - // Vector append. Treat each zero byte as a nullzero. If valid_bytes is null // assume all of length bits are valid. void UnsafeAppendToBitmap(const uint8_t* valid_bytes, int64_t length); @@ -214,8 +249,13 @@ class ARROW_EXPORT PrimitiveBuilder : public ArrayBuilder { return Status::OK(); } + ARROW_DEPRECATED("Use Finish instead") std::shared_ptr data() const { return data_; } + const value_type GetValue(int64_t index) const { + return reinterpret_cast(data_->data())[index]; + } + /// \brief Append a sequence of elements in one shot /// \param[in] values a contiguous C array of values /// \param[in] length the number of values to append @@ -257,12 +297,77 @@ class ARROW_EXPORT PrimitiveBuilder : public ArrayBuilder { /// \param[in] values a std::vector of values /// \return Status Status AppendValues(const std::vector& values); + + /// \brief Append a sequence of elements in one shot + /// \param[in] values_begin InputIterator to the beginning of the values + /// \param[in] values_end InputIterator pointing to the end of the values + /// \return Status + + template + Status AppendValues(ValuesIter values_begin, ValuesIter values_end) { + int64_t length = static_cast(std::distance(values_begin, values_end)); + RETURN_NOT_OK(Reserve(length)); + + std::copy(values_begin, values_end, raw_data_ + length_); + + // this updates the length_ + UnsafeSetNotNull(length); + return Status::OK(); + } + + /// \brief Append a sequence of elements in one shot, with a specified nullmap + /// \param[in] values_begin InputIterator to the beginning of the values + /// \param[in] values_end InputIterator pointing to the end of the values + /// \param[in] valid_begin InputIterator with elements indication valid(1) + /// or null(0) values. + /// \return Status + template + typename std::enable_if::value, Status>::type AppendValues( + ValuesIter values_begin, ValuesIter values_end, ValidIter valid_begin) { + static_assert(!is_null_pointer::value, + "Don't pass a NULLPTR directly as valid_begin, use the 2-argument " + "version instead"); + int64_t length = static_cast(std::distance(values_begin, values_end)); + RETURN_NOT_OK(Reserve(length)); + + std::copy(values_begin, values_end, raw_data_ + length_); + + // this updates the length_ + UnsafeAppendToBitmap(valid_begin, std::next(valid_begin, length)); + return Status::OK(); + } + + /// \brief Append a sequence of elements in one shot, with a specified nullmap + /// \param[in] values_begin InputIterator to the beginning of the values + /// \param[in] values_end InputIterator pointing to the end of the values + /// \param[in] valid_begin uint8_t* indication valid(1) or null(0) values. + /// nullptr indicates all values are valid. + /// \return Status + template + typename std::enable_if::value, Status>::type AppendValues( + ValuesIter values_begin, ValuesIter values_end, ValidIter valid_begin) { + int64_t length = static_cast(std::distance(values_begin, values_end)); + RETURN_NOT_OK(Reserve(length)); + + std::copy(values_begin, values_end, raw_data_ + length_); + + // this updates the length_ + if (valid_begin == NULLPTR) { + UnsafeSetNotNull(length); + } else { + UnsafeAppendToBitmap(valid_begin, std::next(valid_begin, length)); + } + + return Status::OK(); + } + /// \deprecated Use AppendValues instead. ARROW_DEPRECATED("Use AppendValues instead") Status Append(const std::vector& values); Status FinishInternal(std::shared_ptr* out) override; Status Init(int64_t capacity) override; + void Reset() override; /// Increase the capacity of the builder to accommodate at least the indicated /// number of elements @@ -357,9 +462,11 @@ class ARROW_EXPORT AdaptiveIntBuilderBase : public ArrayBuilder { return Status::OK(); } + ARROW_DEPRECATED("Use Finish instead") std::shared_ptr data() const { return data_; } Status Init(int64_t capacity) override; + void Reset() override; /// Increase the capacity of the builder to accommodate at least the indicated /// number of elements @@ -420,6 +527,7 @@ class ARROW_EXPORT AdaptiveUIntBuilder : public internal::AdaptiveIntBuilderBase explicit AdaptiveUIntBuilder(MemoryPool* pool ARROW_MEMORY_POOL_DEFAULT); using ArrayBuilder::Advance; + using internal::AdaptiveIntBuilderBase::Reset; /// Scalar append Status Append(const uint64_t val) { @@ -486,6 +594,7 @@ class ARROW_EXPORT AdaptiveIntBuilder : public internal::AdaptiveIntBuilderBase explicit AdaptiveIntBuilder(MemoryPool* pool ARROW_MEMORY_POOL_DEFAULT); using ArrayBuilder::Advance; + using internal::AdaptiveIntBuilderBase::Reset; /// Scalar append Status Append(const int64_t val) { @@ -566,9 +675,11 @@ class ARROW_EXPORT BooleanBuilder : public ArrayBuilder { Status AppendNull() { RETURN_NOT_OK(Reserve(1)); UnsafeAppendToBitmap(false); + return Status::OK(); } + ARROW_DEPRECATED("Use Finish instead") std::shared_ptr data() const { return data_; } /// Scalar append @@ -648,8 +759,77 @@ class ARROW_EXPORT BooleanBuilder : public ArrayBuilder { ARROW_DEPRECATED("Use AppendValues instead") Status Append(const std::vector& values); + /// \brief Append a sequence of elements in one shot + /// \param[in] values_begin InputIterator to the beginning of the values + /// \param[in] values_end InputIterator pointing to the end of the values + /// or null(0) values + /// \return Status + template + Status AppendValues(ValuesIter values_begin, ValuesIter values_end) { + int64_t length = static_cast(std::distance(values_begin, values_end)); + RETURN_NOT_OK(Reserve(length)); + auto iter = values_begin; + internal::GenerateBitsUnrolled(raw_data_, length_, length, + [&iter]() -> bool { return *(iter++); }); + + // this updates length_ + UnsafeSetNotNull(length); + return Status::OK(); + } + + /// \brief Append a sequence of elements in one shot, with a specified nullmap + /// \param[in] values_begin InputIterator to the beginning of the values + /// \param[in] values_end InputIterator pointing to the end of the values + /// \param[in] valid_begin InputIterator with elements indication valid(1) + /// or null(0) values + /// \return Status + template + typename std::enable_if::value, Status>::type AppendValues( + ValuesIter values_begin, ValuesIter values_end, ValidIter valid_begin) { + static_assert(!is_null_pointer::value, + "Don't pass a NULLPTR directly as valid_begin, use the 2-argument " + "version instead"); + int64_t length = static_cast(std::distance(values_begin, values_end)); + RETURN_NOT_OK(Reserve(length)); + + auto iter = values_begin; + internal::GenerateBitsUnrolled(raw_data_, length_, length, + [&iter]() -> bool { return *(iter++); }); + + // this updates length_ + ArrayBuilder::UnsafeAppendToBitmap(valid_begin, std::next(valid_begin, length)); + return Status::OK(); + } + + /// \brief Append a sequence of elements in one shot, with a specified nullmap + /// \param[in] values_begin InputIterator to the beginning of the values + /// \param[in] values_end InputIterator pointing to the end of the values + /// \param[in] valid_begin uint8_t* indication valid(1) or null(0) values. + /// nullptr indicates all values are valid. + /// \return Status + template + typename std::enable_if::value, Status>::type AppendValues( + ValuesIter values_begin, ValuesIter values_end, ValidIter valid_begin) { + int64_t length = static_cast(std::distance(values_begin, values_end)); + RETURN_NOT_OK(Reserve(length)); + + auto iter = values_begin; + internal::GenerateBitsUnrolled(raw_data_, length_, length, + [&iter]() -> bool { return *(iter++); }); + + // this updates the length_ + if (valid_begin == NULLPTR) { + UnsafeSetNotNull(length); + } else { + UnsafeAppendToBitmap(valid_begin, std::next(valid_begin, length)); + } + + return Status::OK(); + } + Status FinishInternal(std::shared_ptr* out) override; Status Init(int64_t capacity) override; + void Reset() override; /// Increase the capacity of the builder to accommodate at least the indicated /// number of elements @@ -685,6 +865,7 @@ class ARROW_EXPORT ListBuilder : public ArrayBuilder { Status Init(int64_t elements) override; Status Resize(int64_t capacity) override; + void Reset() override; Status FinishInternal(std::shared_ptr* out) override; /// \brief Vector append @@ -714,8 +895,6 @@ class ARROW_EXPORT ListBuilder : public ArrayBuilder { std::shared_ptr values_; Status AppendNextOffset(); - - void Reset(); }; // ---------------------------------------------------------------------- @@ -742,6 +921,7 @@ class ARROW_EXPORT BinaryBuilder : public ArrayBuilder { Status AppendNull(); Status Init(int64_t elements) override; + void Reset() override; Status Resize(int64_t capacity) override; /// \brief Ensures there is enough allocated capacity to append the indicated /// number of bytes to the value data buffer without additional allocations @@ -763,7 +943,6 @@ class ARROW_EXPORT BinaryBuilder : public ArrayBuilder { TypedBufferBuilder value_data_builder_; Status AppendNextOffset(); - void Reset(); }; /// \class StringBuilder @@ -774,6 +953,7 @@ class ARROW_EXPORT StringBuilder : public BinaryBuilder { explicit StringBuilder(MemoryPool* pool ARROW_MEMORY_POOL_DEFAULT); using BinaryBuilder::Append; + using BinaryBuilder::Reset; /// \brief Append a sequence of strings in one shot. /// @@ -839,6 +1019,7 @@ class ARROW_EXPORT FixedSizeBinaryBuilder : public ArrayBuilder { Status AppendNull(); Status Init(int64_t elements) override; + void Reset() override; Status Resize(int64_t capacity) override; Status FinishInternal(std::shared_ptr* out) override; @@ -864,6 +1045,7 @@ class ARROW_EXPORT Decimal128Builder : public FixedSizeBinaryBuilder { using FixedSizeBinaryBuilder::Append; using FixedSizeBinaryBuilder::AppendValues; + using FixedSizeBinaryBuilder::Reset; Status Append(const Decimal128& val); @@ -912,6 +1094,8 @@ class ARROW_EXPORT StructBuilder : public ArrayBuilder { Status AppendNull() { return Append(false); } + void Reset() override; + ArrayBuilder* field_builder(int i) const { return field_builders_[i].get(); } int num_fields() const { return static_cast(field_builders_.size()); } @@ -968,8 +1152,6 @@ class ARROW_EXPORT DictionaryBuilder : public ArrayBuilder { public: using Scalar = typename internal::DictionaryScalar::type; - ~DictionaryBuilder() override {} - DictionaryBuilder(const std::shared_ptr& type, MemoryPool* pool); template @@ -987,6 +1169,7 @@ class ARROW_EXPORT DictionaryBuilder : public ArrayBuilder { Status AppendArray(const Array& array); Status Init(int64_t elements) override; + void Reset() override; Status Resize(int64_t capacity) override; Status FinishInternal(std::shared_ptr* out) override; @@ -1035,8 +1218,6 @@ class ARROW_EXPORT DictionaryBuilder : public ArrayBuilder { template <> class ARROW_EXPORT DictionaryBuilder : public ArrayBuilder { public: - ~DictionaryBuilder() override; - DictionaryBuilder(const std::shared_ptr& type, MemoryPool* pool); explicit DictionaryBuilder(MemoryPool* pool); diff --git a/cpp/src/arrow/column-benchmark.cc b/cpp/src/arrow/column-benchmark.cc index af2c368c329..c37c0889031 100644 --- a/cpp/src/arrow/column-benchmark.cc +++ b/cpp/src/arrow/column-benchmark.cc @@ -31,6 +31,8 @@ Status MakePrimitive(int64_t length, int64_t null_count, std::shared_ptr* auto null_bitmap = std::make_shared(pool); RETURN_NOT_OK(data->Resize(length * sizeof(typename ArrayType::value_type))); RETURN_NOT_OK(null_bitmap->Resize(BitUtil::BytesForBits(length))); + data->ZeroPadding(); + null_bitmap->ZeroPadding(); *out = std::make_shared(length, data, null_bitmap, null_count); return Status::OK(); } diff --git a/cpp/src/arrow/compute/kernels/hash.cc b/cpp/src/arrow/compute/kernels/hash.cc index 5ef8ee59fd1..2341e6829fb 100644 --- a/cpp/src/arrow/compute/kernels/hash.cc +++ b/cpp/src/arrow/compute/kernels/hash.cc @@ -289,6 +289,7 @@ class HashTableKernel< // TODO(wesm): handle null being in the dictionary auto dict_data = dict_.buffer; RETURN_NOT_OK(dict_data->Resize(dict_.size * sizeof(T), false)); + dict_data->ZeroPadding(); *out = ArrayData::Make(type_, dict_.size, {nullptr, dict_data}, 0); return Status::OK(); diff --git a/cpp/src/arrow/io/file.cc b/cpp/src/arrow/io/file.cc index 19f4a35384b..12c287bdb4e 100644 --- a/cpp/src/arrow/io/file.cc +++ b/cpp/src/arrow/io/file.cc @@ -199,6 +199,7 @@ class ReadableFile::ReadableFileImpl : public OSFile { RETURN_NOT_OK(Read(nbytes, &bytes_read, buffer->mutable_data())); if (bytes_read < nbytes) { RETURN_NOT_OK(buffer->Resize(bytes_read)); + buffer->ZeroPadding(); } *out = buffer; return Status::OK(); @@ -212,6 +213,7 @@ class ReadableFile::ReadableFileImpl : public OSFile { RETURN_NOT_OK(ReadAt(position, nbytes, &bytes_read, buffer->mutable_data())); if (bytes_read < nbytes) { RETURN_NOT_OK(buffer->Resize(bytes_read)); + buffer->ZeroPadding(); } *out = buffer; return Status::OK(); diff --git a/cpp/src/arrow/io/hdfs.cc b/cpp/src/arrow/io/hdfs.cc index ba89b48c253..73201325023 100644 --- a/cpp/src/arrow/io/hdfs.cc +++ b/cpp/src/arrow/io/hdfs.cc @@ -143,6 +143,7 @@ class HdfsReadableFile::HdfsReadableFileImpl : public HdfsAnyFileImpl { if (bytes_read < nbytes) { RETURN_NOT_OK(buffer->Resize(bytes_read)); + buffer->ZeroPadding(); } *out = buffer; diff --git a/cpp/src/arrow/io/memory.cc b/cpp/src/arrow/io/memory.cc index 54cf8e4595a..345a10804e3 100644 --- a/cpp/src/arrow/io/memory.cc +++ b/cpp/src/arrow/io/memory.cc @@ -68,6 +68,7 @@ Status BufferOutputStream::Close() { Status BufferOutputStream::Finish(std::shared_ptr* result) { RETURN_NOT_OK(Close()); + buffer_->ZeroPadding(); *result = buffer_; buffer_ = nullptr; is_open_ = false; diff --git a/cpp/src/arrow/ipc/json-internal.cc b/cpp/src/arrow/ipc/json-internal.cc index 829c895c7ba..df1dcbc16b5 100644 --- a/cpp/src/arrow/ipc/json-internal.cc +++ b/cpp/src/arrow/ipc/json-internal.cc @@ -1017,7 +1017,6 @@ class ArrayReader { DCHECK_EQ(static_cast(json_data_arr.Size()), length_); - auto byte_buffer = std::make_shared(pool_); for (int i = 0; i < length_; ++i) { if (!is_valid_[i]) { RETURN_NOT_OK(builder.AppendNull()); @@ -1034,9 +1033,8 @@ class ArrayReader { DCHECK(hex_string.size() % 2 == 0) << "Expected base16 hex string"; int32_t length = static_cast(hex_string.size()) / 2; - if (byte_buffer->size() < length) { - RETURN_NOT_OK(byte_buffer->Resize(length)); - } + std::shared_ptr byte_buffer; + RETURN_NOT_OK(AllocateBuffer(pool_, length, &byte_buffer)); const char* hex_data = hex_string.c_str(); uint8_t* byte_buffer_data = byte_buffer->mutable_data(); diff --git a/cpp/src/arrow/ipc/metadata-internal.cc b/cpp/src/arrow/ipc/metadata-internal.cc index d73deb43c41..530262d14dc 100644 --- a/cpp/src/arrow/ipc/metadata-internal.cc +++ b/cpp/src/arrow/ipc/metadata-internal.cc @@ -685,8 +685,8 @@ static Status SchemaToFlatbuffer(FBB& fbb, const Schema& schema, static Status WriteFlatbufferBuilder(FBB& fbb, std::shared_ptr* out) { int32_t size = fbb.GetSize(); - auto result = std::make_shared(); - RETURN_NOT_OK(result->Resize(size)); + std::shared_ptr result; + RETURN_NOT_OK(AllocateBuffer(default_memory_pool(), size, &result)); uint8_t* dst = result->mutable_data(); memcpy(dst, fbb.GetBufferPointer(), size); diff --git a/cpp/src/arrow/python/numpy_to_arrow.cc b/cpp/src/arrow/python/numpy_to_arrow.cc index 09926ba6153..c0a2649af03 100644 --- a/cpp/src/arrow/python/numpy_to_arrow.cc +++ b/cpp/src/arrow/python/numpy_to_arrow.cc @@ -543,8 +543,8 @@ Status CastBuffer(const std::shared_ptr& in_type, template Status StaticCastBuffer(const Buffer& input, const int64_t length, MemoryPool* pool, std::shared_ptr* out) { - auto result = std::make_shared(pool); - RETURN_NOT_OK(result->Resize(sizeof(ToType) * length)); + std::shared_ptr result; + RETURN_NOT_OK(AllocateBuffer(pool, sizeof(ToType) * length, &result)); auto in_values = reinterpret_cast(input.data()); auto out_values = reinterpret_cast(result->mutable_data()); @@ -582,8 +582,8 @@ Status CopyStridedArray(PyArrayObject* arr, const int64_t length, MemoryPool* po using T = typename traits::T; // Strided, must copy into new contiguous memory - auto new_buffer = std::make_shared(pool); - RETURN_NOT_OK(new_buffer->Resize(sizeof(T) * length)); + std::shared_ptr new_buffer; + RETURN_NOT_OK(AllocateBuffer(pool, sizeof(T) * length, &new_buffer)); const int64_t stride = PyArray_STRIDES(arr)[0]; if (stride % sizeof(T) == 0) { @@ -623,8 +623,8 @@ inline Status NumPyConverter::ConvertData(std::shared_ptr* data) { template <> inline Status NumPyConverter::ConvertData(std::shared_ptr* data) { int64_t nbytes = BitUtil::BytesForBits(length_); - auto buffer = std::make_shared(pool_); - RETURN_NOT_OK(buffer->Resize(nbytes)); + std::shared_ptr buffer; + RETURN_NOT_OK(AllocateBuffer(pool_, nbytes, &buffer)); Ndarray1DIndexer values(arr_); @@ -698,8 +698,8 @@ inline Status NumPyConverter::ConvertData(std::shared_ptr* d // separately here from int64_t to int32_t, because this data is not // supported in compute::Cast if (date_dtype->meta.base == NPY_FR_D) { - auto result = std::make_shared(pool_); - RETURN_NOT_OK(result->Resize(sizeof(int64_t) * length_)); + std::shared_ptr result; + RETURN_NOT_OK(AllocateBuffer(pool_, sizeof(int64_t) * length_, &result)); auto in_values = reinterpret_cast((*data)->data()); auto out_values = reinterpret_cast(result->mutable_data()); @@ -1068,8 +1068,8 @@ Status NumPyConverter::ConvertBooleans() { } int64_t nbytes = BitUtil::BytesForBits(length_); - auto data = std::make_shared(pool_); - RETURN_NOT_OK(data->Resize(nbytes)); + std::shared_ptr data; + RETURN_NOT_OK(AllocateBuffer(pool_, nbytes, &data)); uint8_t* bitmap = data->mutable_data(); memset(bitmap, 0, nbytes); diff --git a/cpp/src/arrow/python/python_to_arrow.cc b/cpp/src/arrow/python/python_to_arrow.cc index ae308f0b4e9..15cd2a694f8 100644 --- a/cpp/src/arrow/python/python_to_arrow.cc +++ b/cpp/src/arrow/python/python_to_arrow.cc @@ -80,7 +80,7 @@ class SequenceBuilder { Status AppendNone() { RETURN_NOT_OK(offsets_.Append(0)); RETURN_NOT_OK(types_.Append(0)); - return nones_.AppendToBitmap(false); + return nones_.AppendNull(); } Status Update(int64_t offset, int8_t* tag) { @@ -91,7 +91,7 @@ class SequenceBuilder { RETURN_NOT_OK(internal::CastSize(offset, &offset32)); RETURN_NOT_OK(offsets_.Append(offset32)); RETURN_NOT_OK(types_.Append(*tag)); - return nones_.AppendToBitmap(true); + return nones_.Append(true); } template @@ -219,7 +219,7 @@ class SequenceBuilder { if (tag != -1) { fields_[tag] = ::arrow::field(name, out->type()); RETURN_NOT_OK(out->Finish(&children_[tag])); - RETURN_NOT_OK(nones_.AppendToBitmap(true)); + RETURN_NOT_OK(nones_.Append(true)); type_ids_.push_back(tag); } return Status::OK(); @@ -240,7 +240,7 @@ class SequenceBuilder { fields_[tag] = ::arrow::field("", type); children_[tag] = std::shared_ptr( new StructArray(type, list_array->length(), {list_array})); - RETURN_NOT_OK(nones_.AppendToBitmap(true)); + RETURN_NOT_OK(nones_.Append(true)); type_ids_.push_back(tag); } else { DCHECK_EQ(offsets.size(), 1); @@ -280,10 +280,13 @@ class SequenceBuilder { RETURN_NOT_OK(offsets_.Finish(&offsets_array)); const auto& offsets = checked_cast(*offsets_array); + std::shared_ptr nones_array; + RETURN_NOT_OK(nones_.Finish(&nones_array)); + const auto& nones = checked_cast(*nones_array); + auto type = ::arrow::union_(fields_, type_ids_, UnionMode::DENSE); out->reset(new UnionArray(type, types.length(), children_, types.values(), - offsets.values(), nones_.null_bitmap(), - nones_.null_count())); + offsets.values(), nones.null_bitmap(), nones.null_count())); return Status::OK(); } @@ -293,7 +296,7 @@ class SequenceBuilder { Int8Builder types_; Int32Builder offsets_; - NullBuilder nones_; + BooleanBuilder nones_; BooleanBuilder bools_; Int64Builder ints_; Int64Builder py2_ints_; diff --git a/cpp/src/arrow/test-common.h b/cpp/src/arrow/test-common.h index 4f7a268485d..f09561e2ac7 100644 --- a/cpp/src/arrow/test-common.h +++ b/cpp/src/arrow/test-common.h @@ -62,9 +62,9 @@ class TestBase : public ::testing::Test { template std::shared_ptr TestBase::MakeRandomArray(int64_t length, int64_t null_count) { - auto data = std::make_shared(pool_); const int64_t data_nbytes = length * sizeof(typename ArrayType::value_type); - EXPECT_OK(data->Resize(data_nbytes)); + std::shared_ptr data; + EXPECT_OK(AllocateBuffer(pool_, data_nbytes, &data)); // Fill with random data test::random_bytes(data_nbytes, random_seed_++, data->mutable_data()); @@ -84,9 +84,9 @@ std::shared_ptr TestBase::MakeRandomArray( int64_t length, int64_t null_count) { const int byte_width = 10; std::shared_ptr null_bitmap = MakeRandomNullBitmap(length, null_count); - std::shared_ptr data = std::make_shared(pool_); + std::shared_ptr data; + EXPECT_OK(AllocateBuffer(pool_, byte_width * length, &data)); - EXPECT_OK(data->Resize(byte_width * length)); ::arrow::test::random_bytes(data->size(), 0, data->mutable_data()); return std::make_shared(fixed_size_binary(byte_width), length, data, null_bitmap, null_count); diff --git a/cpp/src/arrow/test-util.h b/cpp/src/arrow/test-util.h index cb5c8b7d84f..7109e64f891 100644 --- a/cpp/src/arrow/test-util.h +++ b/cpp/src/arrow/test-util.h @@ -124,8 +124,8 @@ inline Status CopyBufferFromVector(const std::vector& values, MemoryPool* poo std::shared_ptr* result) { int64_t nbytes = static_cast(values.size()) * sizeof(T); - auto buffer = std::make_shared(pool); - RETURN_NOT_OK(buffer->Resize(nbytes)); + std::shared_ptr buffer; + RETURN_NOT_OK(AllocateBuffer(pool, nbytes, &buffer)); auto immutable_data = reinterpret_cast(values.data()); std::copy(immutable_data, immutable_data + nbytes, buffer->mutable_data()); memset(buffer->mutable_data() + nbytes, 0, @@ -289,7 +289,8 @@ Status MakeRandomInt32PoolBuffer(int64_t length, MemoryPool* pool, uint32_t seed = 0) { DCHECK(pool); auto data = std::make_shared(pool); - RETURN_NOT_OK(data->Resize(length * sizeof(int32_t))); + RETURN_NOT_OK(data->Resize(sizeof(int32_t) * length)); + data->ZeroPadding(); test::rand_uniform_int(length, seed, 0, std::numeric_limits::max(), reinterpret_cast(data->mutable_data())); *pool_buffer = data; @@ -301,6 +302,7 @@ Status MakeRandomBytePoolBuffer(int64_t length, MemoryPool* pool, uint32_t seed = 0) { auto bytes = std::make_shared(pool); RETURN_NOT_OK(bytes->Resize(length)); + bytes->ZeroPadding(); test::random_bytes(length, seed, bytes->mutable_data()); *pool_buffer = bytes; return Status::OK(); diff --git a/cpp/src/arrow/util/CMakeLists.txt b/cpp/src/arrow/util/CMakeLists.txt index d309b2b98f5..16fd44236cb 100644 --- a/cpp/src/arrow/util/CMakeLists.txt +++ b/cpp/src/arrow/util/CMakeLists.txt @@ -60,8 +60,10 @@ ADD_ARROW_TEST(key-value-metadata-test) ADD_ARROW_TEST(rle-encoding-test) ADD_ARROW_TEST(stl-util-test) ADD_ARROW_TEST(thread-pool-test) +ADD_ARROW_TEST(lazy-test) ADD_ARROW_BENCHMARK(bit-util-benchmark) ADD_ARROW_BENCHMARK(decimal-benchmark) +ADD_ARROW_BENCHMARK(lazy-benchmark) add_subdirectory(variant) diff --git a/cpp/src/arrow/util/hash.cc b/cpp/src/arrow/util/hash.cc index 4f2f1a22bc9..bd28b2fd190 100644 --- a/cpp/src/arrow/util/hash.cc +++ b/cpp/src/arrow/util/hash.cc @@ -24,9 +24,9 @@ namespace arrow { namespace internal { Status NewHashTable(int64_t size, MemoryPool* pool, std::shared_ptr* out) { - auto hash_table = std::make_shared(pool); + std::shared_ptr hash_table; + RETURN_NOT_OK(AllocateBuffer(pool, sizeof(hash_slot_t) * size, &hash_table)); - RETURN_NOT_OK(hash_table->Resize(sizeof(hash_slot_t) * size)); auto slots = reinterpret_cast(hash_table->mutable_data()); std::fill(slots, slots + size, kHashSlotEmpty); diff --git a/cpp/src/arrow/util/io-util.h b/cpp/src/arrow/util/io-util.h index 28197e0dce3..ac85b9b4f30 100644 --- a/cpp/src/arrow/util/io-util.h +++ b/cpp/src/arrow/util/io-util.h @@ -109,6 +109,7 @@ class StdinStream : public InputStream { int64_t bytes_read; RETURN_NOT_OK(Read(nbytes, &bytes_read, buffer->mutable_data())); RETURN_NOT_OK(buffer->Resize(bytes_read, false)); + buffer->ZeroPadding(); *out = buffer; return Status::OK(); } diff --git a/cpp/src/arrow/util/lazy-benchmark.cc b/cpp/src/arrow/util/lazy-benchmark.cc new file mode 100644 index 00000000000..9b04c655273 --- /dev/null +++ b/cpp/src/arrow/util/lazy-benchmark.cc @@ -0,0 +1,123 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include +#include +#include + +#include + +#include "arrow/test-util.h" +#include "arrow/util/lazy.h" + +namespace arrow { + +static constexpr int64_t kSize = 100000000; + +template +std::vector generate_junk(int64_t size) { + std::vector v(size); + test::randint(size, 0, 100000, &v); + return v; +} + +// baseline +void BM_for_loop(benchmark::State& state) { + auto source = generate_junk(kSize); + std::vector target(kSize); + + for (auto _ : state) { + for (int64_t index = 0; index < kSize; ++index) target[index] = source[index] + 1; + } +} + +BENCHMARK(BM_for_loop)->Repetitions(3)->Unit(benchmark::kMillisecond); + +// for comparison: pure copy without any changes +void BM_std_copy(benchmark::State& state) { + auto source = generate_junk(kSize); + std::vector target(kSize); + + for (auto _ : state) { + std::copy(source.begin(), source.end(), target.begin()); + } +} + +BENCHMARK(BM_std_copy)->Repetitions(3)->Unit(benchmark::kMillisecond); + +// for comparison: pure copy without any changes +void BM_std_copy_converting(benchmark::State& state) { + auto source = generate_junk(kSize); + // bigger type to avoid warnings + std::vector target(kSize); + + for (auto _ : state) { + std::copy(source.begin(), source.end(), target.begin()); + } +} + +BENCHMARK(BM_std_copy_converting)->Repetitions(3)->Unit(benchmark::kMillisecond); + +// std::copy with a lazy iterator +void BM_lazy_copy(benchmark::State& state) { + auto source = generate_junk(kSize); + std::vector target(kSize); + auto lazy_range = internal::MakeLazyRange( + [&source](int64_t index) { return source[index]; }, source.size()); + + for (auto _ : state) { + std::copy(lazy_range.begin(), lazy_range.end(), target.begin()); + } +} + +BENCHMARK(BM_lazy_copy)->Repetitions(3)->Unit(benchmark::kMillisecond); + +// std::copy with a lazy iterator which does static cast +// should be the same performance as std::copy with differtly typed iterators +void BM_lazy_copy_converting(benchmark::State& state) { + auto source = generate_junk(kSize); + std::vector target(kSize); + auto lazy_range = internal::MakeLazyRange( + [&source](int64_t index) { return static_cast(source[index]); }, + source.size()); + + for (auto _ : state) { + std::copy(lazy_range.begin(), lazy_range.end(), target.begin()); + } +} + +BENCHMARK(BM_lazy_copy_converting)->Repetitions(3)->Unit(benchmark::kMillisecond); + +// for loop with a post-increment of a lazy operator +void BM_lazy_postinc(benchmark::State& state) { + auto source = generate_junk(kSize); + std::vector target(kSize); + auto lazy_range = internal::MakeLazyRange( + [&source](int64_t index) { return source[index]; }, source.size()); + + for (auto _ : state) { + auto lazy_iter = lazy_range.begin(); + auto lazy_end = lazy_range.end(); + auto target_iter = target.begin(); + + while (lazy_iter != lazy_end) *(target_iter++) = *(lazy_iter++); + } +} + +BENCHMARK(BM_lazy_postinc)->Repetitions(3)->Unit(benchmark::kMillisecond); + +} // namespace arrow diff --git a/cpp/src/arrow/util/lazy-test.cc b/cpp/src/arrow/util/lazy-test.cc new file mode 100644 index 00000000000..83cc6a0d436 --- /dev/null +++ b/cpp/src/arrow/util/lazy-test.cc @@ -0,0 +1,65 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include + +#include + +#include "arrow/test-util.h" +#include "arrow/util/lazy.h" + +namespace arrow { + +class TestLazyIter : public ::testing::Test { + public: + int64_t kSize = 1000; + void SetUp() { + test::randint(kSize, 0, 1000000, &source_); + target_.resize(kSize); + } + + protected: + std::vector source_; + std::vector target_; +}; + +TEST_F(TestLazyIter, TestIncrementCopy) { + auto add_one = [this](int64_t index) { return source_[index] + 1; }; + auto lazy_range = internal::MakeLazyRange(add_one, kSize); + std::copy(lazy_range.begin(), lazy_range.end(), target_.begin()); + + for (int64_t index = 0; index < kSize; ++index) { + ASSERT_EQ(source_[index] + 1, target_[index]); + } +} + +TEST_F(TestLazyIter, TestPostIncrementCopy) { + auto add_one = [this](int64_t index) { return source_[index] + 1; }; + auto lazy_range = internal::MakeLazyRange(add_one, kSize); + auto iter = lazy_range.begin(); + auto end = lazy_range.end(); + auto target_iter = target_.begin(); + + while (iter != end) { + *(target_iter++) = *(iter++); + } + + for (size_t index = 0, limit = source_.size(); index != limit; ++index) { + ASSERT_EQ(source_[index] + 1, target_[index]); + } +} +} // namespace arrow diff --git a/cpp/src/arrow/util/lazy.h b/cpp/src/arrow/util/lazy.h new file mode 100644 index 00000000000..de32b5f22af --- /dev/null +++ b/cpp/src/arrow/util/lazy.h @@ -0,0 +1,128 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#ifndef ARROW_UTIL_LAZY_H +#define ARROW_UTIL_LAZY_H + +#include +#include + +namespace arrow { +namespace internal { + +/// Create a range from a callable which takes a single index parameter +/// and returns the value of iterator on each call and a length. +/// Only iterators obtained from the same range should be compared, the +/// behaviour generally similar to other STL containers. +template +class LazyRange { + private: + // callable which generates the values + // has to be defined at the beginning of the class for type deduction + const Generator gen_; + // the length of the range + int64_t length_; +#ifdef _MSC_VER + // workaround to VS2010 not supporting decltype properly + // see https://stackoverflow.com/questions/21782846/decltype-for-class-member-function + static Generator gen_static_; +#endif + + public: +#ifdef _MSC_VER + using return_type = decltype(gen_static_(0)); +#else + using return_type = decltype(gen_(0)); +#endif + + /// Construct a new range from a callable and length + LazyRange(Generator gen, int64_t length) : gen_(gen), length_(length) {} + + // Class of the dependent iterator, created implicitly by begin and end + class RangeIter { + public: + using difference_type = int64_t; + using value_type = return_type; + using reference = const value_type&; + using pointer = const value_type*; + using iterator_category = std::forward_iterator_tag; + +#ifdef _MSC_VER + // msvc complains about unchecked iterators, + // see https://stackoverflow.com/questions/21655496/error-c4996-checked-iterators + using _Unchecked_type = typename LazyRange::RangeIter; +#endif + + RangeIter(const LazyRange& range, int64_t index) + : range_(range), index_(index) {} + + const return_type operator*() { return range_.gen_(index_); } + + RangeIter operator+(difference_type length) { + return RangeIter(range_, index_ + length); + } + + // pre-increment + RangeIter& operator++() { + ++index_; + return *this; + } + + // post-increment + RangeIter operator++(int) { + auto copy = RangeIter(*this); + ++index_; + return copy; + } + + bool operator==(const typename LazyRange::RangeIter& other) const { + return this->index_ == other.index_ && &this->range_ == &other.range_; + } + + bool operator!=(const typename LazyRange::RangeIter& other) const { + return this->index_ != other.index_ || &this->range_ != &other.range_; + } + + int64_t operator-(const typename LazyRange::RangeIter& other) { + return this->index_ - other.index_; + } + + private: + // parent range reference + const LazyRange& range_; + // current index + int64_t index_; + }; + + friend class RangeIter; + + // Create a new begin const iterator + RangeIter begin() { return RangeIter(*this, 0); } + + // Create a new end const iterator + RangeIter end() { return RangeIter(*this, length_); } +}; + +/// Helper function to create a lazy range from a callable (e.g. lambda) and length +template +LazyRange MakeLazyRange(Generator&& gen, int64_t length) { + return LazyRange(std::forward(gen), length); +} + +} // namespace internal +} // namespace arrow +#endif diff --git a/cpp/src/arrow/util/type_traits.h b/cpp/src/arrow/util/type_traits.h index c05309af826..289748ad7af 100644 --- a/cpp/src/arrow/util/type_traits.h +++ b/cpp/src/arrow/util/type_traits.h @@ -36,6 +36,11 @@ struct IsOneOf { template using EnableIfIsOneOf = typename std::enable_if::value, T>::type; +/// \brief is_null_pointer from C++17 +template +struct is_null_pointer : std::is_same::type> { +}; + } // namespace arrow #endif // ARROW_UTIL_TYPE_TRAITS_H