Merge pull request #13283 from ClickHouse/aku/dictionary-layout

Do not fuzz ASTDictionaryLayout

programs/client/QueryFuzzer.cpp

@@ -212,6 +212,8 @@ void QueryFuzzer::fuzzColumnLikeExpressionList(ASTPtr ast)
     }
 
     auto * impl = assert_cast<ASTExpressionList *>(ast.get());
+
+    // Remove element
     if (fuzz_rand() % 50 == 0 && impl->children.size() > 1)
     {
         // Don't remove last element -- this leads to questionable
@@ -219,6 +221,8 @@ void QueryFuzzer::fuzzColumnLikeExpressionList(ASTPtr ast)
         impl->children.erase(impl->children.begin()
                              + fuzz_rand() % impl->children.size());
     }
+
+    // Add element
     if (fuzz_rand() % 50 == 0)
     {
         auto pos = impl->children.empty()
@@ -234,6 +238,9 @@ void QueryFuzzer::fuzzColumnLikeExpressionList(ASTPtr ast)
             fprintf(stderr, "no random col!\n");
         }
     }
+
+    // We don't have to recurse here to fuzz the children, this is handled by
+    // the generic recursion into IAST.children.
 }
 
 void QueryFuzzer::fuzz(ASTs & asts)
@@ -289,21 +296,19 @@ void QueryFuzzer::fuzz(ASTPtr & ast)
     }
     else if (auto * literal = typeid_cast<ASTLiteral *>(ast.get()))
     {
-        // Only change the queries sometimes.
-        int r = fuzz_rand() % 10;
-        if (r == 0)
+        // There is a caveat with fuzzing the children: many ASTs also keep the
+        // links to particular children in own fields. This means that replacing
+        // the child with another object might lead to error. Many of these fields
+        // are ASTPtr -- this is redundant ownership, but hides the error if the
+        // child field is replaced. Others can be ASTLiteral * or the like, which
+        // leads to segfault if the pointed-to AST is replaced.
+        // Replacing children is safe in case of ASTExpressionList. In a more
+        // general case, we can change the value of ASTLiteral, which is what we
+        // do here.
+        if (fuzz_rand() % 11 == 0)
         {
             literal->value = fuzzField(literal->value);
         }
-        else if (r == 1)
-        {
-            /* replace with a random function? */
-        }
-        else if (r == 2)
-        {
-            /* replace with something column-like */
-            replaceWithColumnLike(ast);
-        }
     }
     else
     {

src/Core/Field.cpp

@@ -522,4 +522,21 @@ template <> bool decimalEqual(Decimal128 x, Decimal128 y, UInt32 x_scale, UInt32
 template <> bool decimalLess(Decimal128 x, Decimal128 y, UInt32 x_scale, UInt32 y_scale) { return decLess(x, y, x_scale, y_scale); }
 template <> bool decimalLessOrEqual(Decimal128 x, Decimal128 y, UInt32 x_scale, UInt32 y_scale) { return decLessOrEqual(x, y, x_scale, y_scale); }
 
+inline void writeText(const Null &, WriteBuffer & buf)
+{
+    writeText(std::string("Null"), buf);
+}
+
+String toString(const Field & x)
+{
+    return Field::dispatch(
+        [] (const auto & value)
+        {
+            // Use explicit type to prevent implicit construction of Field and
+            // infinite recursion into toString<Field>.
+            return toString<decltype(value)>(value);
+        },
+        x);
+}
+
 }

src/Core/Field.h

@@ -859,10 +859,27 @@ void writeBinary(const Tuple & x, WriteBuffer & buf);
 
 void writeText(const Tuple & x, WriteBuffer & buf);
 
+
+__attribute__ ((noreturn)) inline void writeText(const AggregateFunctionStateData &, WriteBuffer &)
+{
+    // This probably doesn't make any sense, but we have to have it for
+    // completeness, so that we can use toString(field_value) in field visitors.
+    throw Exception(ErrorCodes::LOGICAL_ERROR, "Cannot convert a Field of type AggregateFunctionStateData to human-readable text");
+}
+
+template <typename T>
+inline void writeText(const DecimalField<T> & value, WriteBuffer & buf)
+{
+    writeText(value.getValue(), value.getScale(), buf);
+}
+
 template <typename T>
 void readQuoted(DecimalField<T> & x, ReadBuffer & buf);
 
 void writeFieldText(const Field & x, WriteBuffer & buf);
 
 [[noreturn]] inline void writeQuoted(const Tuple &, WriteBuffer &) { throw Exception("Cannot write Tuple quoted.", ErrorCodes::NOT_IMPLEMENTED); }
+
+String toString(const Field & x);
+
 }

src/Dictionaries/getDictionaryConfigurationFromAST.cpp

@@ -98,22 +98,36 @@ void buildLayoutConfiguration(
     root->appendChild(layout_element);
     AutoPtr<Element> layout_type_element(doc->createElement(layout->layout_type));
     layout_element->appendChild(layout_type_element);
-    for (const auto & param : layout->parameters)
+    for (const auto & param : layout->parameters->children)
     {
-        AutoPtr<Element> layout_type_parameter_element(doc->createElement(param.first));
-        const ASTLiteral & literal = param.second->as<const ASTLiteral &>();
-        Field::dispatch([&](auto & value)
+        const ASTPair * pair = param->as<ASTPair>();
+        if (!pair)
         {
-            if constexpr (std::is_same_v<std::decay_t<decltype(value)>, UInt64> || std::is_same_v<std::decay_t<decltype(value)>, String>)
-            {
-                AutoPtr<Text> value_to_append(doc->createTextNode(toString(value)));
-                layout_type_parameter_element->appendChild(value_to_append);
-            }
-            else
-            {
-                throw DB::Exception{"Wrong type of layout argument.", ErrorCodes::BAD_ARGUMENTS};
-            }
-        }, literal.value);
+            throw DB::Exception(ErrorCodes::BAD_ARGUMENTS, "Dictionary layout parameters must be key/value pairs, got '{}' instead",
+                param->formatForErrorMessage());
+        }
+
+        const ASTLiteral * value_literal = pair->second->as<ASTLiteral>();
+        if (!value_literal)
+        {
+            throw DB::Exception(ErrorCodes::BAD_ARGUMENTS,
+                "Dictionary layout parameter value must be a literal, got '{}' instead",
+                pair->second->formatForErrorMessage());
+        }
+
+        const auto value_field = value_literal->value;
+
+        if (value_field.getType() != Field::Types::UInt64
+            && value_field.getType() != Field::Types::String)
+        {
+            throw DB::Exception(ErrorCodes::BAD_ARGUMENTS,
+                "Dictionary layout parameter value must be an UInt64 or String, got '{}' instead",
+                value_field.getTypeName());
+        }
+
+        AutoPtr<Element> layout_type_parameter_element(doc->createElement(pair->first));
+        AutoPtr<Text> value_to_append(doc->createTextNode(toString(value_field)));
+        layout_type_parameter_element->appendChild(value_to_append);
         layout_type_element->appendChild(layout_type_parameter_element);
     }
 }

src/Parsers/ASTCreateQuery.cpp

@@ -186,8 +186,13 @@ ASTPtr ASTCreateQuery::clone() const
         res->set(res->select, select->clone());
     if (tables)
         res->set(res->tables, tables->clone());
+
     if (dictionary)
+    {
+        assert(is_dictionary);
+        res->set(res->dictionary_attributes_list, dictionary_attributes_list->clone());
         res->set(res->dictionary, dictionary->clone());
+    }
 
     cloneOutputOptions(*res);
 

src/Parsers/ASTCreateQuery.h

@@ -60,10 +60,8 @@ public:
     bool is_materialized_view{false};
     bool is_live_view{false};
     bool is_populate{false};
-    bool is_dictionary{false}; /// CREATE DICTIONARY
     bool replace_view{false}; /// CREATE OR REPLACE VIEW
     ASTColumns * columns_list = nullptr;
-    ASTExpressionList * dictionary_attributes_list = nullptr; /// attributes of dictionary
     ASTExpressionList * tables = nullptr;
     //FIXME
     StorageID to_table_id = StorageID::createEmpty();   /// For CREATE MATERIALIZED VIEW mv TO table.
@@ -72,7 +70,11 @@ public:
     String as_table;
     ASTPtr as_table_function;
     ASTSelectWithUnionQuery * select = nullptr;
+
+    bool is_dictionary{false}; /// CREATE DICTIONARY
+    ASTExpressionList * dictionary_attributes_list = nullptr; /// attributes of
     ASTDictionary * dictionary = nullptr; /// dictionary definition (layout, primary key, etc.)
+
     std::optional<UInt64> live_view_timeout;    /// For CREATE LIVE VIEW ... WITH TIMEOUT ...
     bool attach_short_syntax{false};
 

src/Parsers/ASTDictionary.cpp

@@ -6,8 +6,7 @@ namespace DB
 
 ASTPtr ASTDictionaryRange::clone() const
 {
-    auto res = std::make_shared<ASTDictionaryRange>(*this);
-    res->children.clear();
+    auto res = std::make_shared<ASTDictionaryRange>();
     res->min_attr_name = min_attr_name;
     res->max_attr_name = max_attr_name;
     return res;
@@ -35,8 +34,7 @@ void ASTDictionaryRange::formatImpl(const FormatSettings & settings,
 
 ASTPtr ASTDictionaryLifetime::clone() const
 {
-    auto res = std::make_shared<ASTDictionaryLifetime>(*this);
-    res->children.clear();
+    auto res = std::make_shared<ASTDictionaryLifetime>();
     res->min_sec = min_sec;
     res->max_sec = max_sec;
     return res;
@@ -64,16 +62,10 @@ void ASTDictionaryLifetime::formatImpl(const FormatSettings & settings,
 
 ASTPtr ASTDictionaryLayout::clone() const
 {
-    auto res = std::make_shared<ASTDictionaryLayout>(*this);
-    res->children.clear();
+    auto res = std::make_shared<ASTDictionaryLayout>();
     res->layout_type = layout_type;
-    res->parameters.clear();
+    res->set(res->parameters, parameters->clone());
     res->has_brackets = has_brackets;
-    for (const auto & parameter : parameters)
-    {
-        res->parameters.emplace_back(parameter.first, nullptr);
-        res->set(res->parameters.back().second, parameter.second->clone());
-    }
     return res;
 }
 
@@ -93,18 +85,7 @@ void ASTDictionaryLayout::formatImpl(const FormatSettings & settings,
     if (has_brackets)
         settings.ostr << "(";
 
-    bool first = true;
-    for (const auto & parameter : parameters)
-    {
-        settings.ostr << (first ? "" : " ")
-                      << (settings.hilite ? hilite_keyword : "")
-                      << Poco::toUpper(parameter.first)
-                      << (settings.hilite ? hilite_none : "")
-                      << " ";
-
-        parameter.second->formatImpl(settings, state, frame);
-        first = false;
-    }
+    parameters->formatImpl(settings, state, frame);
 
     if (has_brackets)
         settings.ostr << ")";
@@ -114,8 +95,7 @@ void ASTDictionaryLayout::formatImpl(const FormatSettings & settings,
 
 ASTPtr ASTDictionarySettings::clone() const
 {
-    auto res = std::make_shared<ASTDictionarySettings>(*this);
-    res->children.clear();
+    auto res = std::make_shared<ASTDictionarySettings>();
     res->changes = changes;
 
     return res;
@@ -143,15 +123,14 @@ void ASTDictionarySettings::formatImpl(const FormatSettings & settings,
 
 ASTPtr ASTDictionary::clone() const
 {
-    auto res = std::make_shared<ASTDictionary>(*this);
-    res->children.clear();
-
-    if (source)
-        res->set(res->source, source->clone());
+    auto res = std::make_shared<ASTDictionary>();
 
     if (primary_key)
         res->set(res->primary_key, primary_key->clone());
 
+    if (source)
+        res->set(res->source, source->clone());
+
     if (lifetime)
         res->set(res->lifetime, lifetime->clone());
 

src/Parsers/ASTDictionary.h

@@ -36,7 +36,8 @@ public:
     /// flat, cache, hashed, etc.
     String layout_type;
     /// parameters (size_in_cells, ...)
-    std::vector<KeyValue> parameters;
+    /// ASTExpressionList -> ASTPair -> (ASTLiteral key, ASTLiteral value).
+    ASTExpressionList * parameters;
     /// has brackets after layout type
     bool has_brackets = true;
 

src/Parsers/ASTFunctionWithKeyValueArguments.cpp

@@ -15,13 +15,7 @@ ASTPtr ASTPair::clone() const
 {
     auto res = std::make_shared<ASTPair>(*this);
     res->children.clear();
-
-    if (second)
-    {
-        res->second = second;
-        res->children.push_back(second);
-    }
-
+    res->set(res->second, second->clone());
     return res;
 }
 
@@ -54,7 +48,7 @@ ASTPtr ASTFunctionWithKeyValueArguments::clone() const
 
     if (elements)
     {
-        res->elements->clone();
+        res->elements = elements->clone();
         res->children.push_back(res->elements);
     }
 

src/Parsers/ASTFunctionWithKeyValueArguments.h

@@ -15,7 +15,7 @@ public:
     /// Name or key of pair
     String first;
     /// Value of pair, which can be also list of pairs
-    ASTPtr second;
+    IAST * second = nullptr;
     /// Value is closed in brackets (HOST '127.0.0.1')
     bool second_with_brackets;
 

src/Parsers/ExpressionListParsers.cpp

@@ -717,7 +717,7 @@ bool ParserKeyValuePair::parseImpl(Pos & pos, ASTPtr & node, Expected & expected
 
     auto pair = std::make_shared<ASTPair>(with_brackets);
     pair->first = Poco::toLower(typeid_cast<ASTIdentifier &>(*identifier.get()).name);
-    pair->second = value;
+    pair->set(pair->second, value);
     node = pair;
     return true;
 }

src/Parsers/ParserDictionary.cpp

@@ -49,7 +49,7 @@ bool ParserDictionaryLifetime::parseImpl(Pos & pos, ASTPtr & node, Expected & ex
     for (const auto & elem : expr_list.children)
     {
         const ASTPair & pair = elem->as<const ASTPair &>();
-        const ASTLiteral * literal = dynamic_cast<const ASTLiteral *>(pair.second.get());
+        const ASTLiteral * literal = pair.second->as<ASTLiteral>();
         if (literal == nullptr)
             return false;
 
@@ -90,7 +90,7 @@ bool ParserDictionaryRange::parseImpl(Pos & pos, ASTPtr & node, Expected & expec
     for (const auto & elem : expr_list.children)
     {
         const ASTPair & pair = elem->as<const ASTPair &>();
-        const ASTIdentifier * identifier = dynamic_cast<const ASTIdentifier *>(pair.second.get());
+        const ASTIdentifier * identifier = pair.second->as<ASTIdentifier>();
         if (identifier == nullptr)
             return false;
 
@@ -130,18 +130,7 @@ bool ParserDictionaryLayout::parseImpl(Pos & pos, ASTPtr & node, Expected & expe
     if (!type_expr_list.children.empty() && !res->has_brackets)
         return false;
 
-    for (const auto & child : type_expr_list.children)
-    {
-        const ASTPair * pair = dynamic_cast<const ASTPair *>(child.get());
-        if (pair == nullptr)
-            return false;
-
-        const ASTLiteral * literal = dynamic_cast<const ASTLiteral *>(pair->second.get());
-        if (literal == nullptr || (literal->value.getType() != Field::Types::UInt64 && literal->value.getType() != Field::Types::String))
-            return false;
-        res->parameters.emplace_back(pair->first, nullptr);
-        res->set(res->parameters.back().second, literal->clone());
-    }
+    res->set(res->parameters, func.elements);
 
     node = res;
     return true;

src/Parsers/tests/gtest_dictionary_parser.cpp

@@ -74,7 +74,7 @@ TEST(ParserDictionaryDDL, SimpleDictionary)
     /// layout test
     auto * layout = create->dictionary->layout;
     EXPECT_EQ(layout->layout_type, "flat");
-    EXPECT_EQ(layout->children.size(), 0);
+    EXPECT_EQ(layout->parameters->children.size(), 0);
 
     /// lifetime test
     auto * lifetime = create->dictionary->lifetime;