diff --git a/clickhouse/columns/factory.cpp b/clickhouse/columns/factory.cpp index 2fb0d39f..90ab1826 100644 --- a/clickhouse/columns/factory.cpp +++ b/clickhouse/columns/factory.cpp @@ -46,6 +46,16 @@ const auto& GetASTChildElement(const TypeAst & ast, int position) { return ast.elements[static_cast(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); +} + 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(); case Type::FixedString: - return std::make_shared(GetASTChildElement(ast, 0).value); + return std::make_shared(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>(); case Type::FixedString: - return std::make_shared>(GetASTChildElement(nested, 0).value); + return std::make_shared>(GetFixedStringSize(nested)); case Type::Nullable: throw UnimplementedError("LowCardinality(" + nested.name + ") is not supported with LowCardinalityAsWrappedColumn on"); default: diff --git a/clickhouse/columns/string.cpp b/clickhouse/columns/string.cpp index e62a9b2b..9c7d739a 100644 --- a/clickhouse/columns/string.cpp +++ b/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) { diff --git a/clickhouse/types/type_parser.cpp b/clickhouse/types/type_parser.cpp index d305a398..c103ea05 100644 --- a/clickhouse/types/type_parser.cpp +++ b/clickhouse/types/type_parser.cpp @@ -16,6 +16,7 @@ #include #endif +#include 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); @@ -368,12 +372,11 @@ const TypeAst* ParseTypeName(const std::string& type_name) { return &it->second; } - auto& ast = ast_cache[type_name]; - if (TypeParser(type_name).Parse(&ast)) { - return * + TypeAst ast; + if (!TypeParser(type_name).Parse(&ast)) { + return nullptr; } - ast_cache.erase(type_name); - return nullptr; + return &ast_cache.emplace(type_name, std::move(ast)).first->second; } } diff --git a/ut/CreateColumnByType_ut.cpp b/ut/CreateColumnByType_ut.cpp index 0f1fbc4f..7c0b6614 100644 --- a/ut/CreateColumnByType_ut.cpp +++ b/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()->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()->FixedSize()); + + EXPECT_THROW(CreateColumnByType("FixedString(16777216)"), ValidationError); +} + +TEST(CreateColumnByType, LowCardinalityFixedStringSizeLimit) { + CreateColumnByTypeSettings wrapped; + wrapped.low_cardinality_as_wrapped_column = true; + + const std::pair 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))", diff --git a/ut/columns_ut.cpp b/ut/columns_ut.cpp index 57650ea9..c4b1e09d 100644 --- a/ut/columns_ut.cpp +++ b/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(0); +TEST(ColumnsCase, FixedString_Size_Eq0_Throws) { + EXPECT_THROW(ColumnFixedString(0), ValidationError); +} + +TEST(ColumnsCase, FixedString_Type_Size_Eq1) { + const auto col = std::make_shared(1); ASSERT_EQ(col->FixedSize(), col->Type()->As()->GetSize()); } diff --git a/ut/type_parser_ut.cpp b/ut/type_parser_ut.cpp index 76bedd7a..fa55a456 100644 --- a/ut/type_parser_ut.cpp +++ b/ut/type_parser_ut.cpp @@ -541,6 +541,30 @@ 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 TypeAst* 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); +} + TEST(TypeParser, AggregateFunction) { { TypeAst ast; diff --git a/ut/types_ut.cpp b/ut/types_ut.cpp index 51ee3aff..6dbe6408 100644 --- a/ut/types_ut.cpp +++ b/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)))",