Merge pull request #68 from Enmk/fixed_string_checks

FixedString::Append checks

Author: Enmk

clickhouse/columns/string.cpp

@@ -31,14 +31,23 @@ ColumnFixedString::ColumnFixedString(size_t n)
 }
 
 void ColumnFixedString::Append(std::string_view str) {
-    if (data_.capacity() < str.size())
+    if (str.size() > string_size_) {
+        throw std::runtime_error("Expected string of length not greater than "
+                                 + std::to_string(string_size_) + " bytes, received "
+                                 + std::to_string(str.size()) + " bytes.");
+    }
+
+    if (data_.capacity() - data_.size() < str.size())
     {
         // round up to the next block size
         const auto new_size = (((data_.size() + string_size_) / DEFAULT_BLOCK_SIZE) + 1) * DEFAULT_BLOCK_SIZE;
         data_.reserve(new_size);
     }
 
     data_.insert(data_.size(), str);
+    // Pad up to string_size_ with zeroes.
+    const auto padding_size = string_size_ - str.size();
+    data_.resize(data_.size() + padding_size, char(0));
 }
 
 void ColumnFixedString::Clear() {

clickhouse/columns/string.h

@@ -18,6 +18,14 @@ class ColumnFixedString : public Column {
 
     explicit ColumnFixedString(size_t n);
 
+    template <typename Values>
+    ColumnFixedString(size_t n, const Values & values)
+        : ColumnFixedString(n)
+    {
+        for (const auto & v : values)
+            Append(v);
+    }
+
     /// Appends one element to the column.
     void Append(std::string_view str);
 

ut/columns_ut.cpp

@@ -144,14 +144,48 @@ TEST(ColumnsCase, NumericSlice) {
 
 
 TEST(ColumnsCase, FixedStringInit) {
-    auto col = std::make_shared<ColumnFixedString>(3);
-    for (const auto& s : MakeFixedStrings()) {
+    const auto column_data = MakeFixedStrings();
+    auto col = std::make_shared<ColumnFixedString>(3, column_data);
+
+    ASSERT_EQ(col->Size(), column_data.size());
+
+    size_t i = 0;
+    for (const auto& s : column_data) {
+        EXPECT_EQ(s, col->At(i));
+        ++i;
+    }
+}
+
+TEST(ColumnsCase, FixedString_Append_SmallStrings) {
+    // Ensure that strings smaller than FixedString's size
+    // are padded with zeroes on insertion.
+
+    const size_t string_size = 7;
+    const auto column_data = MakeFixedStrings();
+
+    auto col = std::make_shared<ColumnFixedString>(string_size);
+    size_t i = 0;
+    for (const auto& s : column_data) {
         col->Append(s);
+
+        EXPECT_EQ(string_size, col->At(i).size());
+
+        std::string expected = column_data[i];
+        expected.resize(string_size, char(0));
+        EXPECT_EQ(expected, col->At(i));
+
+        ++i;
     }
 
-    ASSERT_EQ(col->Size(), 4u);
-    ASSERT_EQ(col->At(1), "bbb");
-    ASSERT_EQ(col->At(3), "ddd");
+    ASSERT_EQ(col->Size(), i);
+}
+
+TEST(ColumnsCase, FixedString_Append_LargeString) {
+    // Ensure that inserting strings larger than FixedString size thorws exception.
+
+    const auto col = std::make_shared<ColumnFixedString>(1);
+    EXPECT_ANY_THROW(col->Append("2c"));
+    EXPECT_ANY_THROW(col->Append("this is a long string"));
 }
 
 TEST(ColumnsCase, StringInit) {