Fixed ColumnLowCardinality::Clear() and handling of null-item at other places

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{}};
     }
 }
 
@@ -125,18 +127,19 @@ ColumnLowCardinality::ColumnLowCardinality(ColumnRef dictionary_column)
 {
     if (dictionary_column_->Size() != 0) {
         // When dictionary column was constructed with values, re-add values by copying to update index and unique_items_map.
+        // TODO: eliminate data copying by coming with better solution than doing AppendUnsafe() N times.
 
         // Steal values into temporary column.
         auto values = dictionary_column_->Slice(0, 0);
         values->Swap(*dictionary_column_);
 
-        AppendNullItemToDictionary(dictionary_column_);
+        AppendNullItemToEmptyColumn();
 
         // Re-add values, updating index and unique_items_map.
         for (size_t i = 0; i < values->Size(); ++i)
             AppendUnsafe(values->GetItem(i));
     } else {
-        AppendNullItemToDictionary(dictionary_column_);
+        AppendNullItemToEmptyColumn();
     }
 }
 
@@ -288,6 +291,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 {
@@ -353,6 +359,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

@@ -82,6 +82,9 @@ class ColumnLowCardinality : public Column {
     ColumnRef GetDictionary();
     void AppendUnsafe(const ItemView &);
 
+private:
+    void AppendNullItemToEmptyColumn();
+
 public:
     static details::LowCardinalityHashKey computeHashKey(const ItemView &);
 };
@@ -90,7 +93,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 +105,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 +125,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>