From d048e0961311802887656a53e27678f55663ba9e Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 5 Sep 2023 11:59:11 +0000 Subject: [PATCH] Fix unexpected errors in system.errors after join --- src/Interpreters/TreeRewriter.cpp | 21 +++++++++++-------- src/Interpreters/TreeRewriter.h | 10 +++++++-- .../evaluateConstantExpression.cpp | 20 +++++++++++++++--- src/Interpreters/evaluateConstantExpression.h | 5 ++++- .../02871_join_on_system_errors.reference | 0 .../02871_join_on_system_errors.sql | 13 ++++++++++++ 6 files changed, 54 insertions(+), 15 deletions(-) create mode 100644 tests/queries/0_stateless/02871_join_on_system_errors.reference create mode 100644 tests/queries/0_stateless/02871_join_on_system_errors.sql diff --git a/src/Interpreters/TreeRewriter.cpp b/src/Interpreters/TreeRewriter.cpp index b71086f21885..d87ac1ed4359 100644 --- a/src/Interpreters/TreeRewriter.cpp +++ b/src/Interpreters/TreeRewriter.cpp @@ -604,15 +604,13 @@ std::optional tryEvaluateConstCondition(ASTPtr expr, ContextPtr context) Field eval_res; DataTypePtr eval_res_type; - try { - std::tie(eval_res, eval_res_type) = evaluateConstantExpression(expr, context); - } - catch (DB::Exception &) - { - /// not a constant expression - return {}; + auto constant_expression_result = tryEvaluateConstantExpression(expr, context); + if (!constant_expression_result) + return {}; + std::tie(eval_res, eval_res_type) = std::move(constant_expression_result.value()); } + /// UInt8, maybe Nullable, maybe LowCardinality, and NULL are allowed eval_res_type = removeNullable(removeLowCardinality(eval_res_type)); if (auto which = WhichDataType(eval_res_type); !which.isUInt8() && !which.isNothing()) @@ -959,7 +957,7 @@ void TreeRewriterResult::collectSourceColumns(bool add_special) /// Calculate which columns are required to execute the expression. /// Then, delete all other columns from the list of available columns. /// After execution, columns will only contain the list of columns needed to read from the table. -void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select, bool visit_index_hint) +bool TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select, bool visit_index_hint, bool no_throw) { /// We calculate required_source_columns with source_columns modifications and swap them on exit required_source_columns = source_columns; @@ -1178,6 +1176,8 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select ss << " '" << name << "'"; } + if (no_throw) + return false; throw Exception(PreformattedMessage{ss.str(), format_string}, ErrorCodes::UNKNOWN_IDENTIFIER); } @@ -1186,6 +1186,7 @@ void TreeRewriterResult::collectUsedColumns(const ASTPtr & query, bool is_select { source_column_names.insert(column.name); } + return true; } NameSet TreeRewriterResult::getArrayJoinSourceNameSet() const @@ -1395,7 +1396,9 @@ TreeRewriterResultPtr TreeRewriter::analyze( else assertNoAggregates(query, "in wrong place"); - result.collectUsedColumns(query, false, settings.query_plan_optimize_primary_key); + bool is_ok = result.collectUsedColumns(query, false, settings.query_plan_optimize_primary_key, no_throw); + if (!is_ok) + return {}; return std::make_shared(result); } diff --git a/src/Interpreters/TreeRewriter.h b/src/Interpreters/TreeRewriter.h index 89cc13da294b..60832f49b355 100644 --- a/src/Interpreters/TreeRewriter.h +++ b/src/Interpreters/TreeRewriter.h @@ -87,7 +87,7 @@ struct TreeRewriterResult bool add_special = true); void collectSourceColumns(bool add_special); - void collectUsedColumns(const ASTPtr & query, bool is_select, bool visit_index_hint); + bool collectUsedColumns(const ASTPtr & query, bool is_select, bool visit_index_hint, bool no_throw = false); Names requiredSourceColumns() const { return required_source_columns.getNames(); } const Names & requiredSourceColumnsForAccessCheck() const { return required_source_columns_before_expanding_alias_columns; } NameSet getArrayJoinSourceNameSet() const; @@ -108,7 +108,10 @@ using TreeRewriterResultPtr = std::shared_ptr; class TreeRewriter : WithContext { public: - explicit TreeRewriter(ContextPtr context_) : WithContext(context_) {} + explicit TreeRewriter(ContextPtr context_, bool no_throw_ = false) + : WithContext(context_) + , no_throw(no_throw_) + {} /// Analyze and rewrite not select query TreeRewriterResultPtr analyze( @@ -132,6 +135,9 @@ class TreeRewriter : WithContext private: static void normalize(ASTPtr & query, Aliases & aliases, const NameSet & source_columns_set, bool ignore_alias, const Settings & settings, bool allow_self_aliases, ContextPtr context_, bool is_create_parameterized_view = false); + + /// Do not throw exception from analyze on unknown identifiers, but only return nullptr. + bool no_throw = false; }; } diff --git a/src/Interpreters/evaluateConstantExpression.cpp b/src/Interpreters/evaluateConstantExpression.cpp index 921cd5ff553e..6d5a0c4bdfa1 100644 --- a/src/Interpreters/evaluateConstantExpression.cpp +++ b/src/Interpreters/evaluateConstantExpression.cpp @@ -28,7 +28,7 @@ namespace ErrorCodes extern const int BAD_ARGUMENTS; } -static std::pair> getFieldAndDataTypeFromLiteral(ASTLiteral * literal) +static EvaluateConstantExpressionResult getFieldAndDataTypeFromLiteral(ASTLiteral * literal) { auto type = applyVisitor(FieldToDataType(), literal->value); /// In case of Array field nested fields can have different types. @@ -39,7 +39,7 @@ static std::pair> getFieldAndDataTypeFro return {res, type}; } -std::pair> evaluateConstantExpression(const ASTPtr & node, const ContextPtr & context) +std::optional evaluateConstantExpressionImpl(const ASTPtr & node, const ContextPtr & context, bool no_throw) { if (ASTLiteral * literal = node->as()) return getFieldAndDataTypeFromLiteral(literal); @@ -67,7 +67,9 @@ std::pair> evaluateConstantExpression(co if (context->getClientInfo().query_kind != ClientInfo::QueryKind::SECONDARY_QUERY && context->getSettingsRef().normalize_function_names) FunctionNameNormalizer().visit(ast.get()); - auto syntax_result = TreeRewriter(context).analyze(ast, source_columns); + auto syntax_result = TreeRewriter(context, no_throw).analyze(ast, source_columns); + if (!syntax_result) + return {}; /// AST potentially could be transformed to literal during TreeRewriter analyze. /// For example if we have SQL user defined function that return literal AS subquery. @@ -108,6 +110,18 @@ std::pair> evaluateConstantExpression(co return std::make_pair((*result_column)[0], result_type); } +std::optional tryEvaluateConstantExpression(const ASTPtr & node, const ContextPtr & context) +{ + return evaluateConstantExpressionImpl(node, context, true); +} + +EvaluateConstantExpressionResult evaluateConstantExpression(const ASTPtr & node, const ContextPtr & context) +{ + auto res = evaluateConstantExpressionImpl(node, context, false); + if (!res) + throw Exception(ErrorCodes::LOGICAL_ERROR, "evaluateConstantExpression expected to return a result or throw an exception"); + return *res; +} ASTPtr evaluateConstantExpressionAsLiteral(const ASTPtr & node, const ContextPtr & context) { diff --git a/src/Interpreters/evaluateConstantExpression.h b/src/Interpreters/evaluateConstantExpression.h index 91f3ac5dffd5..7efb498c9ead 100644 --- a/src/Interpreters/evaluateConstantExpression.h +++ b/src/Interpreters/evaluateConstantExpression.h @@ -17,13 +17,16 @@ class IDataType; using ExpressionActionsPtr = std::shared_ptr; +using EvaluateConstantExpressionResult = std::pair>; + /** Evaluate constant expression and its type. * Used in rare cases - for elements of set for IN, for data to INSERT. * Throws exception if it's not a constant expression. * Quite suboptimal. */ -std::pair> evaluateConstantExpression(const ASTPtr & node, const ContextPtr & context); +EvaluateConstantExpressionResult evaluateConstantExpression(const ASTPtr & node, const ContextPtr & context); +std::optional tryEvaluateConstantExpression(const ASTPtr & node, const ContextPtr & context); /** Evaluate constant expression and returns ASTLiteral with its value. */ diff --git a/tests/queries/0_stateless/02871_join_on_system_errors.reference b/tests/queries/0_stateless/02871_join_on_system_errors.reference new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/queries/0_stateless/02871_join_on_system_errors.sql b/tests/queries/0_stateless/02871_join_on_system_errors.sql new file mode 100644 index 000000000000..ae30ef8f743e --- /dev/null +++ b/tests/queries/0_stateless/02871_join_on_system_errors.sql @@ -0,0 +1,13 @@ + +-- Unique table alias to distinguish between errors from different queries +SELECT * FROM (SELECT 1 as a) t +JOIN (SELECT 2 as a) `89467d35-77c2-4f82-ae7a-f093ff40f4cd` +ON t.a = `89467d35-77c2-4f82-ae7a-f093ff40f4cd`.a +; + +SELECT * +FROM system.errors +WHERE name = 'UNKNOWN_IDENTIFIER' +AND last_error_time > now() - 1 +AND last_error_message LIKE '%Missing columns%89467d35-77c2-4f82-ae7a-f093ff40f4cd%' +;