Merge pull request ClickHouse#11446 from ClickHouse/aku/better-error-messages

akuzm · web-flow · commit 7db8edf40213 · 2020-06-05T13:11:02.000+03:00
Improve error messages
diff --git a/src/AggregateFunctions/parseAggregateFunctionParameters.cpp b/src/AggregateFunctions/parseAggregateFunctionParameters.cpp
@@ -27,8 +27,12 @@ Array getAggregateFunctionParametersArray(const ASTPtr & expression_list, const
         const auto * literal = parameters[i]->as<ASTLiteral>();
         if (!literal)
         {
-            throw Exception("Parameters to aggregate functions must be literals" + (error_context.empty() ? "" : " (in " + error_context +")"),
-                        ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS);
+            throw Exception(
+                ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS,
+                "Parameters to aggregate functions must be literals. "
+                "Got parameter '{}'{}",
+                parameters[i]->formatForErrorMessage(),
+                (error_context.empty() ? "" : " (in " + error_context +")"));
         }
 
         params_row[i] = literal->value;
diff --git a/src/Common/Exception.h b/src/Common/Exception.h
@@ -8,6 +8,8 @@
 
 #include <Common/StackTrace.h>
 
+#include <fmt/format.h>
+
 namespace Poco { class Logger; }
 
 
@@ -20,8 +22,14 @@ class Exception : public Poco::Exception
     Exception() = default;
     Exception(const std::string & msg, int code);
 
-    enum CreateFromPocoTag { CreateFromPoco };
-    enum CreateFromSTDTag { CreateFromSTD };
+    // Format message with fmt::format, like the logging functions.
+    template <typename ...Fmt>
+    Exception(int code, Fmt&&... fmt)
+        : Exception(fmt::format(std::forward<Fmt>(fmt)...), code)
+    {}
+
+    struct CreateFromPocoTag {};
+    struct CreateFromSTDTag {};
 
     Exception(CreateFromPocoTag, const Poco::Exception & exc);
     Exception(CreateFromSTDTag, const std::exception & exc);
diff --git a/src/DataTypes/DataTypeAggregateFunction.cpp b/src/DataTypes/DataTypeAggregateFunction.cpp
@@ -362,8 +362,11 @@ static DataTypePtr create(const ASTPtr & arguments)
         {
             const auto * literal = parameters[i]->as<ASTLiteral>();
             if (!literal)
-                throw Exception("Parameters to aggregate functions must be literals",
-                    ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS);
+                throw Exception(
+                    ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS,
+                    "Parameters to aggregate functions must be literals. "
+                    "Got parameter '{}' for function '{}'",
+                    parameters[i]->formatForErrorMessage(), function_name);
 
             params_row[i] = literal->value;
         }
diff --git a/src/DataTypes/DataTypeCustomSimpleAggregateFunction.cpp b/src/DataTypes/DataTypeCustomSimpleAggregateFunction.cpp
@@ -82,8 +82,11 @@ static std::pair<DataTypePtr, DataTypeCustomDescPtr> create(const ASTPtr & argum
         {
             const ASTLiteral * lit = parameters[i]->as<ASTLiteral>();
             if (!lit)
-                throw Exception("Parameters to aggregate functions must be literals",
-                                ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS);
+                throw Exception(
+                    ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS,
+                    "Parameters to aggregate functions must be literals. "
+                    "Got parameter '{}' for function '{}'",
+                    parameters[i]->formatForErrorMessage(), function_name);
 
             params_row[i] = lit->value;
         }
diff --git a/src/Databases/DatabaseOnDisk.cpp b/src/Databases/DatabaseOnDisk.cpp
@@ -294,7 +294,7 @@ void DatabaseOnDisk::renameTable(
     {
         attachTable(table_name, table, table_data_relative_path);
         /// Better diagnostics.
-        throw Exception{Exception::CreateFromPoco, e};
+        throw Exception{Exception::CreateFromPocoTag{}, e};
     }
 
     /// Now table data are moved to new database, so we must add metadata and attach table to new database
diff --git a/src/Processors/Formats/Impl/AvroRowInputFormat.cpp b/src/Processors/Formats/Impl/AvroRowInputFormat.cpp
@@ -638,7 +638,7 @@ class AvroConfluentRowInputFormat::SchemaRegistry
             }
             catch (const Poco::Exception & e)
             {
-                throw Exception(Exception::CreateFromPoco, e);
+                throw Exception(Exception::CreateFromPocoTag{}, e);
             }
             catch (const avro::Exception & e)
             {
diff --git a/src/Server/TCPHandler.cpp b/src/Server/TCPHandler.cpp
@@ -304,17 +304,17 @@ void TCPHandler::runImpl()
              *  We will try to send exception to the client in any case - see below.
              */
             state.io.onException();
-            exception.emplace(Exception::CreateFromPoco, e);
+            exception.emplace(Exception::CreateFromPocoTag{}, e);
         }
         catch (const Poco::Exception & e)
         {
             state.io.onException();
-            exception.emplace(Exception::CreateFromPoco, e);
+            exception.emplace(Exception::CreateFromPocoTag{}, e);
         }
         catch (const std::exception & e)
         {
             state.io.onException();
-            exception.emplace(Exception::CreateFromSTD, e);
+            exception.emplace(Exception::CreateFromSTDTag{}, e);
         }
         catch (...)
         {

Original file line number	Diff line number	Diff line change
`@@ -27,8 +27,12 @@ Array getAggregateFunctionParametersArray(const ASTPtr & expression_list, const`
`27`	`27`	`const auto * literal = parameters[i]->as<ASTLiteral>();`
`28`	`28`	`if (!literal)`
`29`	`29`	`{`
`30`		`- throw Exception("Parameters to aggregate functions must be literals" + (error_context.empty() ? "" : " (in " + error_context +")"),`
`31`		`- ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS);`
	`30`	`+ throw Exception(`
	`31`	`+ ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS,`
	`32`	`+ "Parameters to aggregate functions must be literals. "`
	`33`	`+ "Got parameter '{}'{}",`
	`34`	`+ parameters[i]->formatForErrorMessage(),`
	`35`	`+ (error_context.empty() ? "" : " (in " + error_context +")"));`
`32`	`36`	`}`
`33`	`37`
`34`	`38`	`params_row[i] = literal->value;`
Original file line number	Diff line number	Diff line change
`@@ -362,8 +362,11 @@ static DataTypePtr create(const ASTPtr & arguments)`
`362`	`362`	`{`
`363`	`363`	`const auto * literal = parameters[i]->as<ASTLiteral>();`
`364`	`364`	`if (!literal)`
`365`		`- throw Exception("Parameters to aggregate functions must be literals",`
`366`		`- ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS);`
	`365`	`+ throw Exception(`
	`366`	`+ ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS,`
	`367`	`+ "Parameters to aggregate functions must be literals. "`
	`368`	`+ "Got parameter '{}' for function '{}'",`
	`369`	`+ parameters[i]->formatForErrorMessage(), function_name);`
`367`	`370`
`368`	`371`	`params_row[i] = literal->value;`
`369`	`372`	`}`
Original file line number	Diff line number	Diff line change
`@@ -82,8 +82,11 @@ static std::pair<DataTypePtr, DataTypeCustomDescPtr> create(const ASTPtr & argum`
`82`	`82`	`{`
`83`	`83`	`const ASTLiteral * lit = parameters[i]->as<ASTLiteral>();`
`84`	`84`	`if (!lit)`
`85`		`- throw Exception("Parameters to aggregate functions must be literals",`
`86`		`- ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS);`
	`85`	`+ throw Exception(`
	`86`	`+ ErrorCodes::PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS,`
	`87`	`+ "Parameters to aggregate functions must be literals. "`
	`88`	`+ "Got parameter '{}' for function '{}'",`
	`89`	`+ parameters[i]->formatForErrorMessage(), function_name);`
`87`	`90`
`88`	`91`	`params_row[i] = lit->value;`
`89`	`92`	`}`
Original file line number	Diff line number	Diff line change
`@@ -294,7 +294,7 @@ void DatabaseOnDisk::renameTable(`
`294`	`294`	`{`
`295`	`295`	`attachTable(table_name, table, table_data_relative_path);`
`296`	`296`	`/// Better diagnostics.`
`297`		`- throw Exception{Exception::CreateFromPoco, e};`
	`297`	`+ throw Exception{Exception::CreateFromPocoTag{}, e};`
`298`	`298`	`}`
`299`	`299`
`300`	`300`	`/// Now table data are moved to new database, so we must add metadata and attach table to new database`
Original file line number	Diff line number	Diff line change
`@@ -638,7 +638,7 @@ class AvroConfluentRowInputFormat::SchemaRegistry`
`638`	`638`	`}`
`639`	`639`	`catch (const Poco::Exception & e)`
`640`	`640`	`{`
`641`		`- throw Exception(Exception::CreateFromPoco, e);`
	`641`	`+ throw Exception(Exception::CreateFromPocoTag{}, e);`
`642`	`642`	`}`
`643`	`643`	`catch (const avro::Exception & e)`
`644`	`644`	`{`
Original file line number	Diff line number	Diff line change
`@@ -304,17 +304,17 @@ void TCPHandler::runImpl()`
`304`	`304`	`* We will try to send exception to the client in any case - see below.`
`305`	`305`	`*/`
`306`	`306`	`state.io.onException();`
`307`		`- exception.emplace(Exception::CreateFromPoco, e);`
	`307`	`+ exception.emplace(Exception::CreateFromPocoTag{}, e);`
`308`	`308`	`}`
`309`	`309`	`catch (const Poco::Exception & e)`
`310`	`310`	`{`
`311`	`311`	`state.io.onException();`
`312`		`- exception.emplace(Exception::CreateFromPoco, e);`
	`312`	`+ exception.emplace(Exception::CreateFromPocoTag{}, e);`
`313`	`313`	`}`
`314`	`314`	`catch (const std::exception & e)`
`315`	`315`	`{`
`316`	`316`	`state.io.onException();`
`317`		`- exception.emplace(Exception::CreateFromSTD, e);`
	`317`	`+ exception.emplace(Exception::CreateFromSTDTag{}, e);`
`318`	`318`	`}`
`319`	`319`	`catch (...)`
`320`	`320`	`{`