From 2da9583aaa4f3ef66604eb8465187e01b01e0295 Mon Sep 17 00:00:00 2001 From: wangwei <1261385937@qq.com> Date: Wed, 15 Nov 2023 20:54:47 +0800 Subject: [PATCH 1/5] ColumnArrayT Append will move container data if it is right value --- clickhouse/columns/array.h | 19 ++++++-- clickhouse/columns/string.cpp | 1 - ut/column_array_ut.cpp | 86 +++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 6 deletions(-) diff --git a/clickhouse/columns/array.h b/clickhouse/columns/array.h index ea51c778..309d8b35 100644 --- a/clickhouse/columns/array.h +++ b/clickhouse/columns/array.h @@ -95,7 +95,7 @@ class ColumnArray : public Column { }; template -class ColumnArrayT : public ColumnArray { +class ColumnArrayT final : public ColumnArray { public: class ArrayValueView; using ValueType = ArrayValueView; @@ -260,8 +260,17 @@ class ColumnArrayT : public ColumnArray { using ColumnArray::Append; template - inline void Append(const Container& container) { - Append(std::begin(container), std::end(container)); + inline void Append(Container&& container) { + using container_type = decltype(container); + if constexpr (std::is_lvalue_reference_v || + std::is_const_v>) { + Append(std::begin(container), std::end(container)); + } + else { + Append(std::make_move_iterator(std::begin(container)), + std::make_move_iterator(std::end(container))); + container.clear(); + } } template @@ -270,12 +279,12 @@ class ColumnArrayT : public ColumnArray { } template - inline void Append(Begin begin, const End & end) { + inline void Append(Begin begin, End end) { auto & nested_data = *typed_nested_data_; size_t counter = 0; while (begin != end) { - nested_data.Append(std::move(*begin)); + nested_data.Append(*begin); ++begin; ++counter; } diff --git a/clickhouse/columns/string.cpp b/clickhouse/columns/string.cpp index 791c2c6c..dff45bac 100644 --- a/clickhouse/columns/string.cpp +++ b/clickhouse/columns/string.cpp @@ -232,7 +232,6 @@ void ColumnString::Clear() { items_.clear(); blocks_.clear(); append_data_.clear(); - append_data_.shrink_to_fit(); } std::string_view ColumnString::At(size_t n) const { diff --git a/ut/column_array_ut.cpp b/ut/column_array_ut.cpp index 001cc62e..3b3a82ee 100644 --- a/ut/column_array_ut.cpp +++ b/ut/column_array_ut.cpp @@ -313,3 +313,89 @@ TEST(ColumnArrayT, Wrap_UInt64_2D) { EXPECT_TRUE(CompareRecursive(values, array)); } + +TEST(ColumnArrayT, left_value_no_move) { + std::string value0 = "000000000000000000"; + std::string value1 = "111111111111111111"; + std::string value2 = "222222222222222222"; + std::vector> all_values{ + { value0, value1, value2}, + { value0, value1, value2}, + { value0, value1, value2} + }; + size_t origin_size = 3; + auto array = std::make_shared>>(); + array->Append(all_values); + EXPECT_EQ(3u, (*array)[0][0].size()); + EXPECT_EQ(3u, (*array)[0][1].size()); + EXPECT_EQ(3u, (*array)[0][2].size()); + + EXPECT_EQ(value0, (*array)[0][0][0]); + EXPECT_EQ(value1, (*array)[0][1][1]); + EXPECT_EQ(value2, (*array)[0][2][2]); + + for (const auto& values : all_values) { + EXPECT_EQ(origin_size, values.size()); + } + + EXPECT_EQ(origin_size, all_values.size()); + for (const auto& values : all_values) { + EXPECT_EQ(values[0], value0); + EXPECT_EQ(values[1], value1); + EXPECT_EQ(values[2], value2); + } +} + +TEST(ColumnArrayT, right_value_move) { + std::string value0 = "000000000000000000"; + std::string value1 = "111111111111111111"; + std::string value2 = "222222222222222222"; + std::vector> all_values{ + { value0, value1, value2}, + { value0, value1, value2}, + { value0, value1, value2} + }; + auto array = std::make_shared>>(); + array->Append(std::move(all_values)); + EXPECT_EQ(3u, (*array)[0][0].size()); + EXPECT_EQ(3u, (*array)[0][1].size()); + EXPECT_EQ(3u, (*array)[0][2].size()); + + EXPECT_EQ(value0, (*array)[0][0][0]); + EXPECT_EQ(value1, (*array)[0][1][1]); + EXPECT_EQ(value2, (*array)[0][2][2]); + + EXPECT_EQ(0u, all_values.size()); +} + +TEST(ColumnArrayT, const_right_value_no_move) { + std::string value0 = "000000000000000000"; + std::string value1 = "111111111111111111"; + std::string value2 = "222222222222222222"; + const std::vector> all_values{ + { value0, value1, value2}, + { value0, value1, value2}, + { value0, value1, value2} + }; + size_t origin_size = 3; + auto array = std::make_shared>>(); + array->Append(std::move(all_values)); + EXPECT_EQ(3u, (*array)[0][0].size()); + EXPECT_EQ(3u, (*array)[0][1].size()); + EXPECT_EQ(3u, (*array)[0][2].size()); + + EXPECT_EQ(value0, (*array)[0][0][0]); + EXPECT_EQ(value1, (*array)[0][1][1]); + EXPECT_EQ(value2, (*array)[0][2][2]); + + for (const auto& values : all_values) { + EXPECT_EQ(origin_size, values.size()); + } + + EXPECT_EQ(origin_size, all_values.size()); + for (const auto& values : all_values) { + EXPECT_EQ(values[0], value0); + EXPECT_EQ(values[1], value1); + EXPECT_EQ(values[2], value2); + } +} \ No newline at end of file From fd3e526621d5efda01a7e62495fe83550531fe2b Mon Sep 17 00:00:00 2001 From: wangwei <1261385937@qq.com> Date: Wed, 15 Nov 2023 22:03:41 +0800 Subject: [PATCH 2/5] remove undesired final --- clickhouse/columns/array.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clickhouse/columns/array.h b/clickhouse/columns/array.h index 309d8b35..e8dc6124 100644 --- a/clickhouse/columns/array.h +++ b/clickhouse/columns/array.h @@ -95,7 +95,7 @@ class ColumnArray : public Column { }; template -class ColumnArrayT final : public ColumnArray { +class ColumnArrayT : public ColumnArray { public: class ArrayValueView; using ValueType = ArrayValueView; From 1ed32e47952b1e04c98ec04794cf1cc4bda05454 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Thu, 16 Nov 2023 00:24:10 +0100 Subject: [PATCH 3/5] Update array.h --- clickhouse/columns/array.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clickhouse/columns/array.h b/clickhouse/columns/array.h index e8dc6124..f47853ba 100644 --- a/clickhouse/columns/array.h +++ b/clickhouse/columns/array.h @@ -249,7 +249,7 @@ class ColumnArrayT : public ColumnArray { if (index >= Size()) throw ValidationError("ColumnArray row index out of bounds: " + std::to_string(index) + ", max is " + std::to_string(Size())); - +It is actually not, b return ArrayValueView{typed_nested_data_, GetOffset(index), GetSize(index)}; } @@ -269,7 +269,6 @@ class ColumnArrayT : public ColumnArray { else { Append(std::make_move_iterator(std::begin(container)), std::make_move_iterator(std::end(container))); - container.clear(); } } From 27c38b2d45782fe4aa5aa35eb05bf7258747a1ab Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Thu, 16 Nov 2023 00:28:03 +0100 Subject: [PATCH 4/5] Update array.h --- clickhouse/columns/array.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clickhouse/columns/array.h b/clickhouse/columns/array.h index f47853ba..3ad9c94d 100644 --- a/clickhouse/columns/array.h +++ b/clickhouse/columns/array.h @@ -249,7 +249,7 @@ class ColumnArrayT : public ColumnArray { if (index >= Size()) throw ValidationError("ColumnArray row index out of bounds: " + std::to_string(index) + ", max is " + std::to_string(Size())); -It is actually not, b + return ArrayValueView{typed_nested_data_, GetOffset(index), GetSize(index)}; } From af908f20fa05af35a2de5b7b5fc4fbd12b6bc83c Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Thu, 16 Nov 2023 01:49:14 +0100 Subject: [PATCH 5/5] update test --- ut/column_array_ut.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ut/column_array_ut.cpp b/ut/column_array_ut.cpp index 3b3a82ee..6fe0bd19 100644 --- a/ut/column_array_ut.cpp +++ b/ut/column_array_ut.cpp @@ -365,7 +365,8 @@ TEST(ColumnArrayT, right_value_move) { EXPECT_EQ(value1, (*array)[0][1][1]); EXPECT_EQ(value2, (*array)[0][2][2]); - EXPECT_EQ(0u, all_values.size()); + // Here we don't care about the size of the container from which all values were moved-out. + //EXPECT_EQ(0u, all_values.size()); } TEST(ColumnArrayT, const_right_value_no_move) { @@ -398,4 +399,4 @@ TEST(ColumnArrayT, const_right_value_no_move) { EXPECT_EQ(values[1], value1); EXPECT_EQ(values[2], value2); } -} \ No newline at end of file +}