Bound the parsed type AST cache

clickhouse/columns/factory.cpp

@@ -46,6 +46,16 @@ const auto& GetASTChildElement(const TypeAst & ast, int position) {
     return ast.elements[static_cast<size_t>(position)];
 }
 
+size_t GetFixedStringSize(const TypeAst& ast) {
+    // Same bounds as the server's DataTypeFixedString (MAX_FIXEDSTRING_SIZE).
+    constexpr int64_t kMaxFixedStringSize = 0xFFFFFF;
+    const auto size                       = GetASTChildElement(ast, 0).value;
+    if (size <= 0 || size > kMaxFixedStringSize)
+        throw ValidationError("FixedString size must be between 1 and " + std::to_string(kMaxFixedStringSize) + ", got " +
+                              std::to_string(size));
+    return static_cast<size_t>(size);
+}
+
 static ColumnRef CreateTerminalColumn(const TypeAst& ast) {
     switch (ast.code) {
     case Type::Void:
@@ -92,7 +102,7 @@ static ColumnRef CreateTerminalColumn(const TypeAst& ast) {
     case Type::String:
         return std::make_shared<ColumnString>();
     case Type::FixedString:
-        return std::make_shared<ColumnFixedString>(GetASTChildElement(ast, 0).value);
+        return std::make_shared<ColumnFixedString>(GetFixedStringSize(ast));
 
     case Type::DateTime:
         if (ast.elements.empty()) {
@@ -225,7 +235,7 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti
                     case Type::String:
                         return std::make_shared<LowCardinalitySerializationAdaptor<ColumnString>>();
                     case Type::FixedString:
-                        return std::make_shared<LowCardinalitySerializationAdaptor<ColumnFixedString>>(GetASTChildElement(nested, 0).value);
+                        return std::make_shared<LowCardinalitySerializationAdaptor<ColumnFixedString>>(GetFixedStringSize(nested));
                     case Type::Nullable:
                         throw UnimplementedError("LowCardinality(" + nested.name + ") is not supported with LowCardinalityAsWrappedColumn on");
                     default:

clickhouse/columns/string.cpp

@@ -29,6 +29,7 @@ ColumnFixedString::ColumnFixedString(size_t n)
     : Column(Type::CreateString(n))
     , string_size_(n)
 {
+    if (n == 0) throw ValidationError("FixedString size must be positive");
 }
 
 void ColumnFixedString::Reserve(size_t new_cap) {

clickhouse/types/type_parser.cpp

@@ -16,6 +16,7 @@
 #include <strings.h>
 #endif
 
+#include <charconv>
 
 namespace clickhouse {
 
@@ -200,10 +201,13 @@ bool TypeParser::Parse(TypeAst* type) {
                 type_->name = token.value.to_string();
                 type_->code = GetTypeCode(type_->name);
                 break;
-            case Token::Number:
+            case Token::Number: {
                 type_->meta = TypeAst::Number;
-                type_->value = std::stol(token.value.to_string());
+                const char* last     = token.value.data() + token.value.size();
+                const auto [ptr, ec] = std::from_chars(token.value.data(), last, type_->value);
+                if (ec != std::errc() || ptr != last) return false;
                 break;
+            }
             case Token::String:
                 type_->meta = TypeAst::String;
                 type_->value_string = std::string(token.value);
@@ -354,26 +358,32 @@ TypeParser::Token TypeParser::NextToken() {
     return Token{Token::EOS, StringView()};
 }
 
-
-const TypeAst* ParseTypeName(const std::string& type_name) {
-    // Cache for type_name.
-    // Usually we won't have too many type names in the cache, so do not try to
-    // limit cache size.
-    static std::map<std::string, TypeAst> ast_cache;
+std::shared_ptr<const TypeAst> ParseTypeName(const std::string& type_name) {
+    static std::map<std::string, std::shared_ptr<const TypeAst>> ast_cache;
+    static size_t ast_cache_bytes = 0;
     static std::mutex lock;
 
+    // Declared before the guard so a cleared cache is destroyed after the mutex is released.
+    std::map<std::string, std::shared_ptr<const TypeAst>> evicted;
     std::lock_guard<std::mutex> guard(lock);
     auto it = ast_cache.find(type_name);
     if (it != ast_cache.end()) {
-        return &it->second;
+        return it->second;
     }
 
-    auto& ast = ast_cache[type_name];
-    if (TypeParser(type_name).Parse(&ast)) {
-        return &ast;
+    auto ast = std::make_shared<TypeAst>();
+    if (!TypeParser(type_name).Parse(ast.get())) {
+        return nullptr;
     }
-    ast_cache.erase(type_name);
-    return nullptr;
-}
 
+    if (type_name.size() <= kTypeAstCacheMaxBytes) {
+        if (ast_cache.size() >= kTypeAstCacheMaxEntries || ast_cache_bytes + type_name.size() > kTypeAstCacheMaxBytes) {
+            evicted.swap(ast_cache);
+            ast_cache_bytes = 0;
+        }
+        ast_cache.emplace(type_name, ast);
+        ast_cache_bytes += type_name.size();
+    }
+    return ast;
+}
 }

clickhouse/types/type_parser.h

@@ -94,7 +94,12 @@ class TypeParser {
     std::string scratch_;
 };
 
-
-const TypeAst* ParseTypeName(const std::string& type_name);
-
+/// ParseTypeName() keeps parsed ASTs in a process-wide cache. When adding an
+/// entry would exceed either limit, the cache is cleared first. A type name
+/// longer than kTypeAstCacheMaxBytes is parsed on every call and never cached.
+inline constexpr size_t kTypeAstCacheMaxEntries = 4096;
+inline constexpr size_t kTypeAstCacheMaxBytes   = 256 * 1024;
+
+/// Returns nullptr if type_name is not a valid type.
+std::shared_ptr<const TypeAst> ParseTypeName(const std::string& type_name);
 }

ut/CreateColumnByType_ut.cpp

@@ -64,6 +64,53 @@ TEST(CreateColumnByType, UnmatchedBrackets) {
     ASSERT_EQ(nullptr, CreateColumnByType("Array(LowCardinality(Nullable(FixedString(10000)))"));
 }
 
+TEST(CreateColumnByType, FixedStringSize) {
+    const auto col = CreateColumnByType("FixedString(16)");
+    ASSERT_NE(nullptr, col);
+    EXPECT_EQ(16u, col->As<ColumnFixedString>()->FixedSize());
+
+    EXPECT_THROW(CreateColumnByType("FixedString(0)"), ValidationError);
+    EXPECT_THROW(CreateColumnByType("FixedString(-1)"), ValidationError);
+    EXPECT_THROW(CreateColumnByType("Nullable(FixedString(0))"), ValidationError);
+    EXPECT_THROW(CreateColumnByType("LowCardinality(FixedString(0))"), ValidationError);
+}
+
+TEST(CreateColumnByType, FixedStringSizeLimit) {
+    const auto col = CreateColumnByType("FixedString(16777215)");
+    ASSERT_NE(nullptr, col);
+    EXPECT_EQ(16777215u, col->As<ColumnFixedString>()->FixedSize());
+
+    EXPECT_THROW(CreateColumnByType("FixedString(16777216)"), ValidationError);
+}
+
+TEST(CreateColumnByType, LowCardinalityFixedStringSizeLimit) {
+    CreateColumnByTypeSettings wrapped;
+    wrapped.low_cardinality_as_wrapped_column = true;
+
+    const std::pair<CreateColumnByTypeSettings, std::string> cases[] = {
+        {CreateColumnByTypeSettings{}, "LowCardinality(FixedString(16777215))"},
+        {wrapped, "FixedString(16777215)"},
+    };
+    for (const auto& [settings, expected_type] : cases) {
+        SCOPED_TRACE(expected_type);
+        const auto col = CreateColumnByType("LowCardinality(FixedString(16777215))", settings);
+        ASSERT_NE(nullptr, col);
+        EXPECT_EQ(expected_type, col->GetType().GetName());
+
+        EXPECT_THROW(CreateColumnByType("LowCardinality(FixedString(16777216))", settings), ValidationError);
+    }
+    EXPECT_THROW(CreateColumnByType("LowCardinality(Nullable(FixedString(16777216)))"), ValidationError);
+}
+
+TEST(CreateColumnByType, MalformedFixedStringSize) {
+    // Repeated on purpose: a failed parse must not leave a cached AST behind.
+    for (int i = 0; i < 2; ++i) {
+        SCOPED_TRACE(i);
+        EXPECT_EQ(nullptr, CreateColumnByType("FixedString(-)"));
+        EXPECT_EQ(nullptr, CreateColumnByType("FixedString(99999999999999999999)"));
+    }
+}
+
 TEST(CreateColumnByType, LowCardinalityAsWrappedColumn) {
     CreateColumnByTypeSettings create_column_settings;
     create_column_settings.low_cardinality_as_wrapped_column = true;
@@ -263,14 +310,10 @@ INSTANTIATE_TEST_SUITE_P(Basic, CreateColumnByTypeWithName, ::testing::Values(
 INSTANTIATE_TEST_SUITE_P(BasicBool, CreateColumnByTypeWithName, ::testing::Values("Bool"));
 #endif
 
-INSTANTIATE_TEST_SUITE_P(Parametrized, CreateColumnByTypeWithName, ::testing::Values(
-    "FixedString(0)", "FixedString(10000)",
-    "DateTime('UTC')", "DateTime64(3, 'UTC')",
-    "Decimal(9,3)", "Decimal(18,3)",
-    "Enum8('ONE' = 1, 'TWO' = 2)",
-    "Enum16('ONE' = 1, 'TWO' = 2, 'THREE' = 3, 'FOUR' = 4)"
-));
-
+INSTANTIATE_TEST_SUITE_P(Parametrized, CreateColumnByTypeWithName,
+                         ::testing::Values("FixedString(1)", "FixedString(10000)", "DateTime('UTC')", "DateTime64(3, 'UTC')",
+                                           "Decimal(9,3)", "Decimal(18,3)", "Enum8('ONE' = 1, 'TWO' = 2)",
+                                           "Enum16('ONE' = 1, 'TWO' = 2, 'THREE' = 3, 'FOUR' = 4)"));
 
 INSTANTIATE_TEST_SUITE_P(Nested, CreateColumnByTypeWithName, ::testing::Values(
     "Nullable(FixedString(10000))",

ut/columns_ut.cpp

@@ -303,8 +303,12 @@ TEST(ColumnsCase, FixedString_Append_LargeString) {
     EXPECT_ANY_THROW(col->Append("this is a long string"));
 }
 
-TEST(ColumnsCase, FixedString_Type_Size_Eq0) {
-    const auto col = std::make_shared<ColumnFixedString>(0);
+TEST(ColumnsCase, FixedString_Size_Eq0_Throws) {
+    EXPECT_THROW(ColumnFixedString(0), ValidationError);
+}
+
+TEST(ColumnsCase, FixedString_Type_Size_Eq1) {
+    const auto col = std::make_shared<ColumnFixedString>(1);
     ASSERT_EQ(col->FixedSize(), col->Type()->As<FixedStringType>()->GetSize());
 }
 

ut/type_parser_ut.cpp

@@ -541,6 +541,84 @@ TEST(ParseTypeName, EmptyName) {
     EXPECT_EQ(nullptr, ParseTypeName(std::string(5, '\0')));
 }
 
+TEST(TypeParser, InvalidNumber) {
+    TypeAst ast;
+    EXPECT_FALSE(TypeParser("FixedString(-)").Parse(&ast));
+
+    TypeAst overflow;
+    EXPECT_FALSE(TypeParser("FixedString(99999999999999999999)").Parse(&overflow));
+
+    TypeAst negative;
+    ASSERT_TRUE(TypeParser("Enum8('a' = -128)").Parse(&negative));
+    ASSERT_EQ(negative.elements.size(), 2u);
+    EXPECT_EQ(negative.elements[1].value, -128);
+}
+
+TEST(ParseTypeName, MalformedNumberReturnsNull) {
+    EXPECT_EQ(nullptr, ParseTypeName("FixedString(-)"));
+    EXPECT_EQ(nullptr, ParseTypeName("FixedString(-)"));
+
+    const auto ast = ParseTypeName("FixedString(16)");
+    ASSERT_NE(nullptr, ast);
+    EXPECT_EQ(ast, ParseTypeName("FixedString(16)"));
+    ASSERT_EQ(ast->elements.size(), 1u);
+    EXPECT_EQ(ast->elements[0].value, 16);
+}
+
+namespace {
+
+size_t CountAlive(const std::vector<std::weak_ptr<const TypeAst>>& asts) {
+    size_t alive = 0;
+    for (const auto& ast : asts) alive += !ast.expired();
+    return alive;
+}
+
+}  // namespace
+
+TEST(ParseTypeName, CacheReusesAst) {
+    const auto ast = ParseTypeName("Array(Nullable(String))");
+    ASSERT_NE(nullptr, ast);
+    EXPECT_EQ(ast, ParseTypeName("Array(Nullable(String))"));
+}
+
+TEST(ParseTypeName, CacheEntryCountIsBounded) {
+    std::vector<std::weak_ptr<const TypeAst>> asts;
+    for (size_t i = 0; i < 3 * kTypeAstCacheMaxEntries; ++i) {
+        asts.push_back(ParseTypeName("FixedString(" + std::to_string(i + 1) + ")"));
+        ASSERT_FALSE(asts.back().expired());
+    }
+    EXPECT_LE(CountAlive(asts), kTypeAstCacheMaxEntries);
+    EXPECT_GT(CountAlive(asts), 0u);
+}
+
+TEST(ParseTypeName, CacheByteBudgetIsBounded) {
+    const std::string label(1024, 'x');
+    const std::string first_name = "Enum8('" + label + "' = 0)";
+    const auto first             = ParseTypeName(first_name);
+    ASSERT_NE(nullptr, first);
+
+    std::vector<std::weak_ptr<const TypeAst>> asts;
+    for (size_t i = 1; i <= 3 * kTypeAstCacheMaxBytes / label.size(); ++i) {
+        asts.push_back(ParseTypeName("Enum8('" + label + std::to_string(i) + "' = 0)"));
+        ASSERT_FALSE(asts.back().expired());
+    }
+    EXPECT_LE(CountAlive(asts), kTypeAstCacheMaxBytes / label.size());
+
+    // An AST handed out before the cache was cleared stays valid.
+    EXPECT_EQ(first->elements[0].value_string, label);
+    EXPECT_NE(first, ParseTypeName(first_name));
+}
+
+TEST(ParseTypeName, OversizedNameIsNotCached) {
+    const std::string name            = "Enum8('" + std::string(kTypeAstCacheMaxBytes, 'x') + "' = 0)";
+    std::weak_ptr<const TypeAst> weak = ParseTypeName(name);
+    EXPECT_TRUE(weak.expired());
+
+    const auto ast = ParseTypeName(name);
+    ASSERT_NE(nullptr, ast);
+    EXPECT_EQ(ast->elements[0].value_string.size(), kTypeAstCacheMaxBytes);
+}
+
 TEST(TypeParser, AggregateFunction) {
     {
         TypeAst ast;

ut/types_ut.cpp

@@ -184,9 +184,9 @@ TEST(TypesCase, IsEqual) {
 #endif
         "UInt8",
         "Int8",
-//        "UInt128",
+        //        "UInt128",
         "String",
-        "FixedString(0)",
+        "FixedString(1)",
         "FixedString(10000)",
         "DateTime('UTC')",
         "DateTime64(3, 'UTC')",
@@ -208,7 +208,8 @@ TEST(TypesCase, IsEqual) {
         "Array(Array(Nullable(Tuple(String, Int8, Date, DateTime))))",
         "Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime)))))",
         "Array(Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime('UTC')))))))"
-        "Array(Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime('UTC'), Tuple(LowCardinality(String), Enum8('READ'=1, 'WRITE'=0))))))))",
+        "Array(Array(Array(Array(Nullable(Tuple(String, Int8, Date, DateTime('UTC'), Tuple(LowCardinality(String), Enum8('READ'=1, "
+        "'WRITE'=0))))))))",
         "Map(String, Int8)",
         "Map(String, Tuple(String, Int8, Date, DateTime))",
         "Map(UUID, Array(Tuple(String, Int8, Date, DateTime)))",