Merge pull request ClickHouse#49 from Enmk/LowCardinality-Clear-fix

Fixed `ColumnLowCardinality::Clear()`

Author: Enmk

clickhouse/columns/column.h

@@ -57,7 +57,7 @@ class Column : public std::enable_shared_from_this<Column> {
     virtual void Swap(Column&) = 0;
 
     /// Get a view on raw item data if it is supported by column, will throw an exception if index is out of range.
-    /// Please note that view is invalidated once column is items are added or deleted, column is loaded from strean or destroyed.
+    /// Please note that view is invalidated once column items are added or deleted, column is loaded from strean or destroyed.
     virtual ItemView GetItem(size_t) const {
         throw std::runtime_error("GetItem() is not supported for column of " + type_->GetName());
     }

clickhouse/columns/lowcardinality.cpp

@@ -10,6 +10,8 @@
 #include <string_view>
 #include <type_traits>
 
+#include <cassert>
+
 namespace {
 using namespace clickhouse;
 
@@ -105,13 +107,13 @@ inline void AppendToDictionary(Column& dictionary, const ItemView & item) {
     }
 }
 
-// Add special NULL-item, which is expected at pos(0) in dictionary,
+// A special NULL-item, which is expected at pos(0) in dictionary,
 // note that we distinguish empty string from NULL-value.
-inline void AppendNullItemToDictionary(ColumnRef dictionary) {
+inline auto GetNullItemForDictionary(const ColumnRef dictionary) {
     if (auto n = dictionary->As<ColumnNullable>()) {
-        AppendToDictionary(*dictionary, ItemView{});
+        return ItemView{};
     } else {
-        AppendToDictionary(*dictionary, ItemView{dictionary->Type()->GetCode(), std::string_view{}});
+        return ItemView{dictionary->Type()->GetCode(), std::string_view{}};
     }
 }
 
@@ -120,23 +122,21 @@ inline void AppendNullItemToDictionary(ColumnRef dictionary) {
 namespace clickhouse {
 ColumnLowCardinality::ColumnLowCardinality(ColumnRef dictionary_column)
     : Column(Type::CreateLowCardinality(dictionary_column->Type())),
-      dictionary_column_(dictionary_column),
+      dictionary_column_(dictionary_column->Slice(0, 0)), // safe way to get an column of the same type.
       index_column_(std::make_shared<ColumnUInt32>())
 {
-    if (dictionary_column_->Size() != 0) {
-        // When dictionary column was constructed with values, re-add values by copying to update index and unique_items_map.
-
-        // Steal values into temporary column.
-        auto values = dictionary_column_->Slice(0, 0);
-        values->Swap(*dictionary_column_);
-
-        AppendNullItemToDictionary(dictionary_column_);
-
-        // Re-add values, updating index and unique_items_map.
-        for (size_t i = 0; i < values->Size(); ++i)
-            AppendUnsafe(values->GetItem(i));
+    if (dictionary_column->Size() != 0) {
+        AppendNullItemToEmptyColumn();
+
+        // Add values, updating index_column_ and unique_items_map_.
+        for (size_t i = 0; i < dictionary_column->Size(); ++i) {
+            // TODO: it would be possible to eliminate copying
+            // by adding InsertUnsafe(pos, ItemView) method to a Column,
+            // but that is too much work for now.
+            AppendUnsafe(dictionary_column->GetItem(i));
+        }
     } else {
-        AppendNullItemToDictionary(dictionary_column_);
+        AppendNullItemToEmptyColumn();
     }
 }
 
@@ -288,6 +288,9 @@ void ColumnLowCardinality::Save(CodedOutputStream* output) {
 void ColumnLowCardinality::Clear() {
     index_column_->Clear();
     dictionary_column_->Clear();
+    unique_items_map_.clear();
+
+    AppendNullItemToEmptyColumn();
 }
 
 size_t ColumnLowCardinality::Size() const {
@@ -298,8 +301,7 @@ ColumnRef ColumnLowCardinality::Slice(size_t begin, size_t len) {
     begin = std::min(begin, Size());
     len = std::min(len, Size() - begin);
 
-    ColumnRef new_dictionary = dictionary_column_->Slice(0, 0);
-    auto result = std::make_shared<ColumnLowCardinality>(new_dictionary);
+    auto result = std::make_shared<ColumnLowCardinality>(dictionary_column_->Slice(0, 0));
 
     for (size_t i = begin; i < begin + len; ++i)
         result->AppendUnsafe(this->GetItem(i));
@@ -353,6 +355,19 @@ void ColumnLowCardinality::AppendUnsafe(const ItemView & value) {
     }
 }
 
+void ColumnLowCardinality::AppendNullItemToEmptyColumn()
+{
+    // INVARIANT: Empty LC column has an (invisible) null-item at pos 0, which MUST be present in
+    // unique_items_map_ in order to reuse dictionary posistion on subsequent Append()-s.
+
+    // Should be only performed on empty LC column.
+    assert(dictionary_column_->Size() == 0 && unique_items_map_.empty());
+
+    const auto null_item = GetNullItemForDictionary(dictionary_column_);
+    AppendToDictionary(*dictionary_column_, null_item);
+    unique_items_map_.emplace(computeHashKey(null_item), dictionary_column_->Size());
+}
+
 size_t ColumnLowCardinality::GetDictionarySize() const {
     return dictionary_column_->Size();
 }

clickhouse/columns/lowcardinality.h

@@ -40,13 +40,14 @@ class ColumnLowCardinality : public Column {
     friend class ColumnLowCardinalityT;
 
 private:
-    // Please note that ColumnLowCardinalityT takes reference to underlying dictionary column object,
+    // IMPLEMENTATION NOTE: ColumnLowCardinalityT takes reference to underlying dictionary column object,
     // so make sure to NOT change address of the dictionary object (with reset(), swap()) or with anything else.
     ColumnRef dictionary_column_;
     ColumnRef index_column_;
     UniqueItems unique_items_map_;
 
 public:
+    // c-tor makes a deep copy of the dictionary_column.
     explicit ColumnLowCardinality(ColumnRef dictionary_column);
     ~ColumnLowCardinality();
 
@@ -82,6 +83,9 @@ class ColumnLowCardinality : public Column {
     ColumnRef GetDictionary();
     void AppendUnsafe(const ItemView &);
 
+private:
+    void AppendNullItemToEmptyColumn();
+
 public:
     static details::LowCardinalityHashKey computeHashKey(const ItemView &);
 };
@@ -90,7 +94,9 @@ class ColumnLowCardinality : public Column {
  */
 template <typename DictionaryColumnType>
 class ColumnLowCardinalityT : public ColumnLowCardinality {
+
     DictionaryColumnType& typed_dictionary_;
+    const Type::Code type_;
 
 public:
     using WrappedColumnType = DictionaryColumnType;
@@ -100,7 +106,8 @@ class ColumnLowCardinalityT : public ColumnLowCardinality {
     template <typename ...Args>
     explicit ColumnLowCardinalityT(Args &&... args)
         : ColumnLowCardinality(std::make_shared<DictionaryColumnType>(std::forward<Args>(args)...)),
-          typed_dictionary_(dynamic_cast<DictionaryColumnType &>(*GetDictionary()))
+          typed_dictionary_(dynamic_cast<DictionaryColumnType &>(*GetDictionary())),
+          type_(typed_dictionary_.Type()->GetCode())
     {}
 
     /// Extended interface to simplify reading/adding individual items.
@@ -119,7 +126,7 @@ class ColumnLowCardinalityT : public ColumnLowCardinality {
     using ColumnLowCardinality::Append;
 
     inline void Append(const ValueType & value) {
-        AppendUnsafe(ItemView{typed_dictionary_.Type()->GetCode(), value});
+        AppendUnsafe(ItemView{type_, value});
     }
 
     template <typename T>

ut/client_ut.cpp

@@ -30,7 +30,29 @@ class ClientCase : public testing::TestWithParam<ClientOptions> {
             client_->Execute("DROP DATABASE test_clickhouse_cpp");
     }
 
+    template <typename T>
+    std::shared_ptr<T> createTableWithOneColumn(Block & block)
+    {
+        auto col = std::make_shared<T>();
+        const auto type_name = col->GetType().GetName();
+
+        client_->Execute("DROP TABLE IF EXISTS " + table_name + ";");
+        client_->Execute("CREATE TABLE IF NOT EXISTS " + table_name + "( " + column_name + " " + type_name + " )"
+                "ENGINE = Memory");
+
+        block.AppendColumn("test_column", col);
+
+        return col;
+    }
+
+    std::string getOneColumnSelectQuery() const
+    {
+        return "SELECT " + column_name + " FROM " + table_name;
+    }
+
     std::unique_ptr<Client> client_;
+    const std::string table_name = "test_clickhouse_cpp.test_ut_table";
+    const std::string column_name = "test_column";
 };
 
 TEST_P(ClientCase, Array) {
@@ -117,22 +139,57 @@ TEST_P(ClientCase, Date) {
 
 TEST_P(ClientCase, LowCardinality) {
     Block block;
-    client_->Execute("DROP TABLE IF EXISTS test_clickhouse_cpp.low_cardinality;");
+    auto lc = createTableWithOneColumn<ColumnLowCardinalityT<ColumnString>>(block);
 
-    client_->Execute("CREATE TABLE IF NOT EXISTS "
-            "test_clickhouse_cpp.low_cardinality (lc LowCardinality(String)) "
-            "ENGINE = Memory");
+    const std::vector<std::string> data{{"FooBar", "1", "2", "Foo", "4", "Bar", "Foo", "7", "8", "Foo"}};
+    lc->AppendMany(data);
+
+    block.RefreshRowCount();
+    client_->Insert(table_name, block);
+
+    size_t total_rows = 0;
+    client_->Select(getOneColumnSelectQuery(),
+        [&total_rows, &data](const Block& block) {
+            total_rows += block.GetRowCount();
+            if (block.GetRowCount() == 0) {
+                return;
+            }
+
+            ASSERT_EQ(1U, block.GetColumnCount());
+            if (auto col = block[0]->As<ColumnLowCardinalityT<ColumnString>>()) {
+                ASSERT_EQ(data.size(), col->Size());
+                for (size_t i = 0; i < col->Size(); ++i) {
+                    EXPECT_EQ(data[i], (*col)[i]) << " at index: " << i;
+                }
+            }
+        }
+    );
+
+    ASSERT_EQ(total_rows, data.size());
+}
+
+TEST_P(ClientCase, LowCardinality_InsertAfterClear) {
+    // User can successfully insert values after invoking Clear() on LC column.
+    Block block;
+    auto lc = createTableWithOneColumn<ColumnLowCardinalityT<ColumnString>>(block);
+
+    // Add some data, but don't care about it much.
+    lc->AppendMany(std::vector<std::string_view>{"abc", "def", "123", "abc", "123", "def", "ghi"});
+    EXPECT_GT(lc->Size(), 0u);
+    EXPECT_GT(lc->GetDictionarySize(), 0u);
 
-    auto lc = std::make_shared<ColumnLowCardinalityT<ColumnString>>();
+    lc->Clear();
 
+    // Now ensure that all data appended after Clear() is inserted properly
     const std::vector<std::string> data{{"FooBar", "1", "2", "Foo", "4", "Bar", "Foo", "7", "8", "Foo"}};
     lc->AppendMany(data);
 
-    block.AppendColumn("lc", lc);
-    client_->Insert("test_clickhouse_cpp.low_cardinality", block);
+    block.RefreshRowCount();
+    client_->Insert(table_name, block);
 
+    // Now validate that data was properly inserted
     size_t total_rows = 0;
-    client_->Select("SELECT lc FROM test_clickhouse_cpp.low_cardinality",
+    client_->Select(getOneColumnSelectQuery(),
         [&total_rows, &data](const Block& block) {
             total_rows += block.GetRowCount();
             if (block.GetRowCount() == 0) {

ut/columns_ut.cpp

@@ -11,7 +11,14 @@
 #include <contrib/gtest/gtest.h>
 #include "utils.h"
 
+#include <string_view>
+
+
+namespace
+{
+
 using namespace clickhouse;
+using namespace std::literals::string_view_literals;
 
 static std::vector<uint32_t> MakeNumbers() {
     return std::vector<uint32_t>
@@ -40,6 +47,17 @@ static std::vector<uint64_t> MakeUUIDs() {
          0x3507213c178649f9llu, 0x9faf035d662f60aellu};
 }
 
+#define BINARY_STRING(x) std::string_view(x, sizeof(x) - 1)
+
+static const auto LOWCARDINALITY_STRING_FOOBAR_10_ITEMS_BINARY =
+        "\x01\x00\x00\x00\x00\x00\x00\x00\x00\x06\x00\x00\x00\x00\x00\x00"
+        "\x09\x00\x00\x00\x00\x00\x00\x00\x00\x06\x46\x6f\x6f\x42\x61\x72"
+        "\x01\x31\x01\x32\x03\x46\x6f\x6f\x01\x34\x03\x42\x61\x72\x01\x37"
+        "\x01\x38\x0a\x00\x00\x00\x00\x00\x00\x00\x01\x02\x03\x04\x05\x06"
+        "\x04\x07\x08\x04"sv;
+
+}
+
 // TODO: add tests for ColumnDecimal.
 
 TEST(ColumnsCase, NumericInit) {
@@ -217,14 +235,26 @@ TEST(ColumnsCase, LowCardinalityWrapperString_Append_and_Read) {
     }
 }
 
-#define BINARY_STRING(x) std::string_view(x, sizeof(x) - 1)
+TEST(ColumnsCase, ColumnLowCardinalityT_String_Clear_and_Append) {
+    const size_t items_count = 11;
+    ColumnLowCardinalityT<ColumnString> col;
+    for (const auto & item : build_vector(&foobar, items_count))
+    {
+        col.Append(item);
+    }
 
-static const auto LOWCARDINALITY_STRING_FOOBAR_10_ITEMS_BINARY =
-        BINARY_STRING("\x01\x00\x00\x00\x00\x00\x00\x00\x00\x06\x00\x00\x00\x00\x00\x00"
-                      "\x09\x00\x00\x00\x00\x00\x00\x00\x00\x06\x46\x6f\x6f\x42\x61\x72"
-                      "\x01\x31\x01\x32\x03\x46\x6f\x6f\x01\x34\x03\x42\x61\x72\x01\x37"
-                      "\x01\x38\x0a\x00\x00\x00\x00\x00\x00\x00\x01\x02\x03\x04\x05\x06"
-                      "\x04\x07\x08\x04");
+    col.Clear();
+    ASSERT_EQ(col.Size(), 0u);
+    ASSERT_EQ(col.GetDictionarySize(), 1u); // null-item
+
+    for (const auto & item : build_vector(&foobar, items_count))
+    {
+        col.Append(item);
+    }
+
+    ASSERT_EQ(col.Size(), items_count);
+    ASSERT_EQ(col.GetDictionarySize(), 8u + 1); // 8 unique items from sequence + 1 null-item
+}
 
 TEST(ColumnsCase, LowCardinalityString_Load) {
     const size_t items_count = 10;