Skip to content

Commit

Permalink
Merge pull request ClickHouse#60088 from ClickHouse/remove-formatting…
Browse files Browse the repository at this point in the history
…-consistency-check-from-the-fuzzer

Remove the check for formatting consistency from the Fuzzer
  • Loading branch information
alexey-milovidov authored Feb 17, 2024
2 parents d4c87de + 3128cf1 commit 538b4ba
Showing 1 changed file with 1 addition and 77 deletions.
78 changes: 1 addition & 77 deletions programs/client/Client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -845,83 +845,7 @@ bool Client::processWithFuzzing(const String & full_query)
have_error = true;
}

// Check that after the query is formatted, we can parse it back,
// format again and get the same result. Unfortunately, we can't
// compare the ASTs, which would be more sensitive to errors. This
// double formatting check doesn't catch all errors, e.g. we can
// format query incorrectly, but to a valid SQL that we can then
// parse and format into the same SQL.
// There are some complicated cases where we can generate the SQL
// which we can't parse:
// * first argument of lambda() replaced by fuzzer with
// something else, leading to constructs such as
// arrayMap((min(x) + 3) -> x + 1, ....)
// * internals of Enum replaced, leading to:
// Enum(equals(someFunction(y), 3)).
// And there are even the cases when we can parse the query, but
// it's logically incorrect and its formatting is a mess, such as
// when `lambda()` function gets substituted into a wrong place.
// To avoid dealing with these cases, run the check only for the
// queries we were able to successfully execute.
// Another caveat is that sometimes WITH queries are not executed,
// if they are not referenced by the main SELECT, so they can still
// have the aforementioned problems. Disable this check for such
// queries, for lack of a better solution.
// There is also a problem that fuzzer substitutes positive Int64
// literals or Decimal literals, which are then parsed back as
// UInt64, and suddenly duplicate alias substitution starts or stops
// working (ASTWithAlias::formatImpl) or something like that.
// So we compare not even the first and second formatting of the
// query, but second and third.
// If you have to add any more workarounds to this check, just remove
// it altogether, it's not so useful.
if (ast_to_process && !have_error && !queryHasWithClause(*ast_to_process))
{
ASTPtr ast_2;
try
{
const auto * tmp_pos = query_to_execute.c_str();
ast_2 = parseQuery(tmp_pos, tmp_pos + query_to_execute.size(), false /* allow_multi_statements */);
}
catch (Exception & e)
{
if (e.code() != ErrorCodes::SYNTAX_ERROR &&
e.code() != ErrorCodes::TOO_DEEP_RECURSION)
throw;
}

if (ast_2)
{
const auto text_2 = ast_2->formatForErrorMessage();
const auto * tmp_pos = text_2.c_str();
const auto ast_3 = parseQuery(tmp_pos, tmp_pos + text_2.size(),
false /* allow_multi_statements */);
const auto text_3 = ast_3 ? ast_3->formatForErrorMessage() : "";

if (text_3 != text_2)
{
fmt::print(stderr, "Found error: The query formatting is broken.\n");

printChangedSettings();

fmt::print(stderr,
"Got the following (different) text after formatting the fuzzed query and parsing it back:\n'{}'\n, expected:\n'{}'\n",
text_3, text_2);
fmt::print(stderr, "In more detail:\n");
fmt::print(stderr, "AST-1 (generated by fuzzer):\n'{}'\n", ast_to_process->dumpTree());
fmt::print(stderr, "Text-1 (AST-1 formatted):\n'{}'\n", query_to_execute);
fmt::print(stderr, "AST-2 (Text-1 parsed):\n'{}'\n", ast_2->dumpTree());
fmt::print(stderr, "Text-2 (AST-2 formatted):\n'{}'\n", text_2);
fmt::print(stderr, "AST-3 (Text-2 parsed):\n'{}'\n", ast_3 ? ast_3->dumpTree() : "");
fmt::print(stderr, "Text-3 (AST-3 formatted):\n'{}'\n", text_3);
fmt::print(stderr, "Text-3 must be equal to Text-2, but it is not.\n");

_exit(1);
}
}
}

// The server is still alive so we're going to continue fuzzing.
// The server is still alive, so we're going to continue fuzzing.
// Determine what we're going to use as the starting AST.
if (have_error)
{
Expand Down

0 comments on commit 538b4ba

Please sign in to comment.