ARROW-1712: [C++] Remove unneeded data member in BinaryBuilder and mo…

…dify test case
apache · Jan 20, 2018 · bc5db7d · bc5db7d
1 parent 5a5b70e
commit bc5db7d
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 51 deletions.
diff --git a/cpp/src/arrow/array-test.cc b/cpp/src/arrow/array-test.cc
@@ -1156,51 +1156,32 @@ TEST_F(TestBinaryBuilder, TestScalarAppend) {
 }
 
 TEST_F(TestBinaryBuilder, TestCapacityReserve) {
-  vector<string> strings = {"a", "bb", "cc", "ddddd", "eeeee"};
-  int64_t N = static_cast<int>(strings.size());
+  vector<string> strings = {"aaaaa", "bbbbbbbbbb", "ccccccccccccccc", "dddddddddddddddddddd", "eeeeeeeeee"};
+  int N = static_cast<int>(strings.size());
+  int reps = 10;
   int64_t length = 0;
-  int64_t data_length = 0;
-  int64_t capacity = N;
-
-  ASSERT_OK(builder_->Reserve(capacity));
+  int64_t capacity = 1000;
+
+
   ASSERT_OK(builder_->ReserveData(capacity));
 
-  ASSERT_EQ(builder_->length(), length);
-  ASSERT_EQ(builder_->capacity(), BitUtil::NextPower2(capacity));
-  ASSERT_EQ(builder_->value_data_length(), data_length);
-  ASSERT_EQ(builder_->value_data_capacity(), capacity);
+  ASSERT_EQ(length, builder_->value_data_length());
+  ASSERT_EQ(BitUtil::RoundUpToMultipleOf64(capacity), builder_->value_data_capacity());
 
-  for(const string& str : strings) {
-    ASSERT_OK(builder_->Append(str));
-    length++;
-    data_length += static_cast<int>(str.size());
+  for (int j = 0; j < reps; ++j) {
+    for (int i = 0; i < N; ++i) {
+      ASSERT_OK(builder_->Append(strings[i]));
+      length += static_cast<int>(strings[i].size());
 
-    ASSERT_EQ(builder_->length(), length);
-    ASSERT_EQ(builder_->capacity(), BitUtil::NextPower2(capacity));
-    ASSERT_EQ(builder_->value_data_length(), data_length);
-    if (data_length <= capacity) {
-      ASSERT_EQ(builder_->value_data_capacity(), capacity);
-    } else {
-      ASSERT_EQ(builder_->value_data_capacity(), data_length);
+      ASSERT_EQ(length, builder_->value_data_length());
+      ASSERT_EQ(BitUtil::RoundUpToMultipleOf64(capacity), builder_->value_data_capacity());
     }
   }
-
-  int extra_capacity = 20;
-
-  ASSERT_OK(builder_->Reserve(extra_capacity));
-  ASSERT_OK(builder_->ReserveData(extra_capacity));
-
-  ASSERT_EQ(builder_->length(), length);
-  ASSERT_EQ(builder_->capacity(), BitUtil::NextPower2(length + extra_capacity));
-  ASSERT_EQ(builder_->value_data_length(), data_length);
-  ASSERT_EQ(builder_->value_data_capacity(), data_length + extra_capacity);
-
   Done();
-
-  ASSERT_EQ(result_->length(), N);
-  ASSERT_EQ(result_->null_count(), 0);
-  ASSERT_EQ(result_->value_data()->size(), data_length);
-  ASSERT_EQ(result_->value_data()->capacity(), BitUtil::RoundUpToMultipleOf64(data_length + extra_capacity));
+  ASSERT_EQ(reps * N, result_->length());
+  ASSERT_EQ(0, result_->null_count());
+  ASSERT_EQ(reps * 60, result_->value_data()->size());
+  ASSERT_EQ(BitUtil::RoundUpToMultipleOf64(capacity), result_->value_data()->capacity());
 }
 
 TEST_F(TestBinaryBuilder, TestZeroLength) {

diff --git a/cpp/src/arrow/builder.cc b/cpp/src/arrow/builder.cc
@@ -1208,7 +1208,7 @@ ArrayBuilder* ListBuilder::value_builder() const {
 // String and binary
 
 BinaryBuilder::BinaryBuilder(const std::shared_ptr<DataType>& type, MemoryPool* pool)
-    : ArrayBuilder(type, pool), offsets_builder_(pool), value_data_builder_(pool), data_capacity_(0) {}
+    : ArrayBuilder(type, pool), offsets_builder_(pool), value_data_builder_(pool) {}
 
 BinaryBuilder::BinaryBuilder(MemoryPool* pool) : BinaryBuilder(binary(), pool) {}
 
@@ -1226,14 +1226,12 @@ Status BinaryBuilder::Resize(int64_t capacity) {
   return ArrayBuilder::Resize(capacity);
 }
 
-Status BinaryBuilder::ReserveData(int64_t capacity) {
-  if (value_data_length() + capacity > data_capacity_) {
-    if (value_data_length() + capacity > std::numeric_limits<int32_t>::max()) {
+Status BinaryBuilder::ReserveData(int64_t elements) {
+  if (value_data_length() + elements > value_data_capacity()) {
+    if (value_data_length() + elements > std::numeric_limits<int32_t>::max()) {
       return Status::Invalid("Cannot reserve capacity larger than 2^31 - 1 in length for binary data");
-    }    
-
-    RETURN_NOT_OK(value_data_builder_.Resize(value_data_length() + capacity));
-    data_capacity_ = value_data_length() + capacity;
+    }
+    RETURN_NOT_OK(value_data_builder_.Reserve(elements));
   }  
   return Status::OK();
 }
@@ -1253,9 +1251,6 @@ 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));
-  if (data_capacity_ < value_data_length()) {
-    data_capacity_ = value_data_length();
-  }
   UnsafeAppendToBitmap(true);
   return Status::OK();
 }

diff --git a/cpp/src/arrow/builder.h b/cpp/src/arrow/builder.h
@@ -682,13 +682,15 @@ class ARROW_EXPORT BinaryBuilder : public ArrayBuilder {
 
   Status Init(int64_t elements) override;
   Status Resize(int64_t capacity) override;
-  Status ReserveData(int64_t capacity);
+  /// Ensures there is enough space for adding the number of value elements 
+  /// by checking value buffer capacity and resizing if necessary.
+  Status ReserveData(int64_t elements);
   Status FinishInternal(std::shared_ptr<ArrayData>* out) override;
 
   /// \return size of values buffer so far
   int64_t value_data_length() const { return value_data_builder_.length(); }
   /// \return capacity of values buffer
-  int64_t value_data_capacity() const { return data_capacity_; }
+  int64_t value_data_capacity() const { return value_data_builder_.capacity(); }
 
   /// Temporary access to a value.
   ///
@@ -699,7 +701,6 @@ class ARROW_EXPORT BinaryBuilder : public ArrayBuilder {
   TypedBufferBuilder<int32_t> offsets_builder_;
   TypedBufferBuilder<uint8_t> value_data_builder_;
 
-  int64_t data_capacity_;
   static constexpr int64_t kMaximumCapacity = std::numeric_limits<int32_t>::max() - 1;
 
   Status AppendNextOffset();