Merge pull request #535 from ClickHouse/fix-nullable-append-exception-safety

Fix NullableT append exception guarantee

Author: slabko

clickhouse/columns/json.h

@@ -72,10 +72,15 @@ class ColumnJSON : public Column {
 template <>
 inline void ColumnNullableT<ColumnJSON>::Append(std::optional<std::string_view> value) {
     ColumnNullable::Append(!value.has_value());
-    if (value.has_value()) {
-        typed_nested_data_->Append(*value);
-    } else {
-        typed_nested_data_->Append(std::string_view("{}"));
+    try {
+        if (value.has_value()) {
+            typed_nested_data_->Append(*value);
+        } else {
+            typed_nested_data_->Append(std::string_view("{}"));
+        }
+    } catch (...) {
+        Nulls()->AsStrict<ColumnUInt8>()->Erase(Size() - 1);
+        throw;
     }
 }
 

clickhouse/columns/nullable.h

@@ -96,10 +96,15 @@ class ColumnNullableT : public ColumnNullable {
 
     inline void Append(ValueType value) {
         ColumnNullable::Append(!value.has_value());
-        if (value.has_value()) {
-            typed_nested_data_->Append(std::move(*value));
-        } else {
-            typed_nested_data_->Append(typename ValueType::value_type{});
+        try {
+            if (value.has_value()) {
+                typed_nested_data_->Append(std::move(*value));
+            } else {
+                typed_nested_data_->Append(typename ValueType::value_type{});
+            }
+        } catch (...) {
+            Nulls()->template As<ColumnUInt8>()->Erase(Size() - 1);
+            throw;
         }
     }
 

ut/Column_ut.cpp

@@ -41,6 +41,17 @@ std::ostream& operator<<(std::ostream& ostr, const Type::Code& type_code) {
 
 }
 
+TEST(ColumnNullableT, AppendRollsBackNullFlagWhenNestedAppendThrows) {
+    auto nested = std::make_shared<ColumnFixedString>(1);
+    auto column = std::make_shared<ColumnNullableT<ColumnFixedString>>(nested);
+
+    EXPECT_THROW(column->Append(std::string_view("too long")), ValidationError);
+
+    EXPECT_EQ(0u, column->Size());
+    EXPECT_EQ(0u, column->Nulls()->Size());
+    EXPECT_EQ(0u, column->Nested()->Size());
+}
+
 
 // Generic tests for a Column subclass against basic API:
 // 1. Constructor: Create, ensure that it is empty
@@ -515,4 +526,3 @@ TYPED_TEST(GenericColumnTest, ArrayT_RoundTrip) {
 
     this->TestColumnRoundtrip(column, LocalHostEndpoint, AllCompressionMethods);
 }
-