fix: FlatVector::resizeValues can lose existing values for opaque types (facebookincubator#15487)

Summary:
Pull Request resolved: facebookincubator#15487

There is a bug in FlatVector::resizeValues where for non-POD types (I think this is just opaque
types) if the Buffer is immutable, there's a typo where it copies values from the new Buffer
into itself, losing the existing values.

Fixing this bug exposes another bug that FlatVector::resizeValues uses BaseVector::length_
as the previous size, however, by this point BaseVector::resize has already been invoked, so it
should be equal to new size (leading to reading off the end of the Buffer after the fix for the
first issue).

To address this I pass the previous size to FlatVector::resizeValues as relying on the order of
invoking this function and BaseVector::resize seems error prone.

Another option would be to rely on the Buffer sizes, but then we may be copying more values
than necessary. Along these lines I updated the logic for POD types (other types) to use the
Vector size to determine how many values to copy over (any other values would have been set
to initialValue already if one is needed).

I noticed a similar bug in the bool template specialization of FlatVector::resizeValues where it
was using BaseVector::length_ for deciding how many values to set to initialValue if we are
just updating the size of an existing Buffer with sufficient capacity. I opted to just delete this
code since it is in a private function and there is no invocation that passes an initialValue for a
bool, and it's inconsistent with the base implementation of FlatVector::resizeValues which only
sets values to initialValue if the Buffer is reallocated.

Reviewed By: bikramSingh91

Differential Revision: D86920858

fbshipit-source-id: 83bd8bdc121cc7998f94b412191dfba1e0c74513

Author: kevinwilfong

velox/vector/FlatVector-inl.h

@@ -456,7 +456,7 @@ void FlatVector<T>::resize(vector_size_t newSize, bool setNotNull) {
   }
 
   if constexpr (std::is_same_v<T, StringView>) {
-    resizeValues(newSize, StringView());
+    resizeValues(newSize, previousSize, StringView());
     if (newSize < previousSize) {
       // If we downsize, just invalidate ascii, because we might have become
       // 'all ascii' from 'not all ascii'.
@@ -476,7 +476,7 @@ void FlatVector<T>::resize(vector_size_t newSize, bool setNotNull) {
       keepAtMostOneStringBuffer();
     }
   } else {
-    resizeValues(newSize, std::nullopt);
+    resizeValues(newSize, previousSize, std::nullopt);
   }
 }
 
@@ -537,6 +537,7 @@ void FlatVector<T>::prepareForReuse() {
 template <typename T>
 void FlatVector<T>::resizeValues(
     vector_size_t newSize,
+    vector_size_t previousSize,
     const std::optional<T>& initialValue) {
   // TODO: change this to isMutable(). See
   // https://github.com/facebookincubator/velox/issues/6562.
@@ -554,16 +555,16 @@ void FlatVector<T>::resizeValues(
       AlignedBuffer::allocate<T>(newSize, BaseVector::pool_, initialValue);
 
   if (values_) {
+    auto len = std::min(newSize, previousSize);
+
     if constexpr (Buffer::is_pod_like_v<T>) {
       auto dst = newValues->asMutable<T>();
       auto src = values_->as<T>();
-      auto len = std::min(values_->size(), newValues->size());
-      memcpy(dst, src, len);
+      auto byteLen = BaseVector::byteSize<T>(len);
+      memcpy(dst, src, byteLen);
     } else {
-      const vector_size_t previousSize = BaseVector::length_;
-      auto* rawOldValues = newValues->asMutable<T>();
+      const auto* rawOldValues = values_->as<T>();
       auto* rawNewValues = newValues->asMutable<T>();
-      const auto len = std::min<vector_size_t>(newSize, previousSize);
       for (vector_size_t row = 0; row < len; ++row) {
         rawNewValues[row] = rawOldValues[row];
       }
@@ -576,6 +577,7 @@ void FlatVector<T>::resizeValues(
 template <>
 inline void FlatVector<bool>::resizeValues(
     vector_size_t newSize,
+    vector_size_t previousSize,
     const std::optional<bool>& initialValue) {
   // TODO: change this to isMutable(). See
   // https://github.com/facebookincubator/velox/issues/6562.
@@ -586,13 +588,6 @@ inline void FlatVector<bool>::resizeValues(
     } else {
       values_->setSize(newByteSize);
     }
-    // ensure that the newly added positions have the right initial value for
-    // the case where changes in size don't result in change in the size of
-    // the underlying buffer.
-    if (initialValue.has_value() && length_ < newSize) {
-      auto rawData = values_->asMutable<uint64_t>();
-      bits::fillBits(rawData, length_, newSize, initialValue.value());
-    }
     rawValues_ = values_->asMutable<bool>();
     return;
   }
@@ -602,7 +597,7 @@ inline void FlatVector<bool>::resizeValues(
   if (values_) {
     auto dst = newValues->asMutable<char>();
     auto src = values_->as<char>();
-    auto len = std::min(values_->size(), newValues->size());
+    auto len = BaseVector::byteSize<bool>(std::min(newSize, previousSize));
     memcpy(dst, src, len);
   }
   values_ = std::move(newValues);

velox/vector/FlatVector.h

@@ -574,10 +574,11 @@ class FlatVector final : public SimpleVector<T> {
       const vector_size_t* toSourceRow);
 
   // Ensures that the values buffer has space for 'newSize' elements and is
-  // mutable. Sets elements between the old and new sizes to 'initialValue' if
-  // the new size > old size.
+  // mutable. Sets elements between the previous and new sizes to 'initialValue'
+  // if the new size > previous size.
   void resizeValues(
       vector_size_t newSize,
+      vector_size_t previousSize,
       const std::optional<T>& initialValue);
 
   // Check string buffers. Keep at most one singly-referenced buffer if it is

velox/vector/tests/VectorTest.cpp

@@ -749,14 +749,17 @@ class VectorTest : public testing::Test, public velox::test::VectorTestBase {
       // immutable. This is true for a slice since it creates buffer views over
       // the buffers of the original vector that it sliced.
       auto newSize = slice->size() * 2;
+      auto sliceCopy = BaseVector::copy(*slice);
       slice->resize(newSize);
       EXPECT_EQ(slice->size(), newSize);
+      slice->resize(sliceCopy->size());
+      test::assertEqualVectors(slice, sliceCopy);
     }
   }
 
   static void testSlices(const VectorPtr& slice, int level = 2) {
     for (vector_size_t offset : {0, 16, 17}) {
-      for (vector_size_t length : {0, 1, 83}) {
+      for (vector_size_t length : {0, 1, 83, 100}) {
         if (offset + length <= slice->size()) {
           testSlice(slice, level, offset, length);
         }