diff --git a/cpp/src/arrow/util/small_vector.h b/cpp/src/arrow/util/small_vector.h index f371e647152f..b6bb45389bc0 100644 --- a/cpp/src/arrow/util/small_vector.h +++ b/cpp/src/arrow/util/small_vector.h @@ -64,6 +64,8 @@ struct StaticVectorStorage : public StaticVectorStorageBase { using Base::size_; using Base::static_data_; + static constexpr bool can_overflow = false; + StaticVectorStorage() noexcept = default; constexpr storage_type* storage_ptr() { return static_data_; } @@ -117,6 +119,8 @@ struct SmallVectorStorage { storage_type* data_ = static_data_; size_t dynamic_capacity_ = 0; + static constexpr bool can_overflow = true; + SmallVectorStorage() noexcept = default; ~SmallVectorStorage() { destroy(); } @@ -163,9 +167,13 @@ struct SmallVectorStorage { other.data_ = other.static_data_; other.dynamic_capacity_ = 0; other.size_ = 0; - } else if (size_ != 0) { - // Use a compile-time memcpy size (N) for trivial types - storage_type::move_construct_several(other.static_data_, static_data_, size_, N); + } else { + // Reset to static storage (in case we had dynamic storage before destroy()) + data_ = static_data_; + if (size_ != 0) { + // Use a compile-time memcpy size (N) for trivial types + storage_type::move_construct_several(other.static_data_, static_data_, size_, N); + } } } @@ -238,9 +246,23 @@ class StaticVectorImpl { StaticVectorImpl& operator=(StaticVectorImpl&& other) noexcept { if (ARROW_PREDICT_TRUE(&other != this)) { - // TODO move_assign? - storage_.destroy(); - storage_.move_construct(std::move(other.storage_)); + // Determine whether to use fast path (destroy + move_construct): + // - For trivial types: always use fast path (memcpy is fast) + // - For dynamic storage: always use fast path (pointer swap is O(1)) + // - For static storage + non-trivial types: use assign_by_moving (may reuse + // objects) + bool use_fast_path = std::is_trivial::value; + if constexpr (Storage::can_overflow) { + use_fast_path = use_fast_path || storage_.dynamic_capacity_ || + other.storage_.dynamic_capacity_; + } + + if (use_fast_path) { + storage_.destroy(); + storage_.move_construct(std::move(other.storage_)); + } else { + assign_by_moving(other.storage_.size_, other.data()); + } } return *this; } diff --git a/cpp/src/arrow/util/small_vector_test.cc b/cpp/src/arrow/util/small_vector_test.cc index f9ec5fedfeaf..31e6d313bfb8 100644 --- a/cpp/src/arrow/util/small_vector_test.cc +++ b/cpp/src/arrow/util/small_vector_test.cc @@ -782,5 +782,51 @@ TEST(SmallVector, Traits) { ASSERT_FALSE((std::is_trivially_destructible>::value)); } +TEST(SmallVector, MoveAssignDynamicToStatic) { + SmallVector src; + SmallVector dst; + + // src: 3 elements (static storage) + src.push_back("a"); + src.push_back("b"); + src.push_back("c"); + + // dst: 12 elements (dynamic storage, uses heap) + for (int i = 0; i < 12; ++i) { + dst.push_back("item" + std::to_string(i)); + } + + // Move from dynamic to static storage + dst = std::move(src); + + ASSERT_EQ(dst.size(), 3); + EXPECT_EQ(dst[0], "a"); + EXPECT_EQ(dst[1], "b"); + EXPECT_EQ(dst[2], "c"); +} + +TEST(SmallVector, MoveAssignStaticToDynamic) { + SmallVector src; + SmallVector dst; + + // src: 12 elements (dynamic storage, uses heap) + for (int i = 0; i < 12; ++i) { + src.push_back("item" + std::to_string(i)); + } + + // dst: 3 elements (static storage) + dst.push_back("x"); + dst.push_back("y"); + dst.push_back("z"); + + // Move from static to dynamic storage + dst = std::move(src); + + ASSERT_EQ(dst.size(), 12); + for (int i = 0; i < 12; ++i) { + EXPECT_EQ(dst[i], "item" + std::to_string(i)); + } +} + } // namespace internal } // namespace arrow