From 27fb90bb58d0cfc5e63ed1538109742a521bee07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Wed, 20 Nov 2024 17:37:32 +0100 Subject: [PATCH 1/2] Fix bugs when using UDF in join on expression with the old analyzer --- .../UserDefinedSQLFunctionVisitor.cpp | 16 +++++------ src/Interpreters/CrossToInnerJoinVisitor.cpp | 2 +- src/Parsers/ASTColumnDeclaration.cpp | 27 ++++++++++++++----- src/Parsers/ASTTablesInSelectQuery.cpp | 23 ++++++++++++++++ src/Parsers/ASTTablesInSelectQuery.h | 3 +++ .../0_stateless/03274_udf_in_join.reference | 7 +++++ .../queries/0_stateless/03274_udf_in_join.sh | 21 +++++++++++++++ 7 files changed, 83 insertions(+), 16 deletions(-) create mode 100644 tests/queries/0_stateless/03274_udf_in_join.reference create mode 100755 tests/queries/0_stateless/03274_udf_in_join.sh diff --git a/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp b/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp index a04b8d7b998..57867eeb5cf 100644 --- a/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp +++ b/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp @@ -26,14 +26,6 @@ void UserDefinedSQLFunctionVisitor::visit(ASTPtr & ast) { chassert(ast); - if (const auto * function = ast->template as()) - { - std::unordered_set udf_in_replace_process; - auto replace_result = tryToReplaceFunction(*function, udf_in_replace_process); - if (replace_result) - ast = replace_result; - } - for (auto & child : ast->children) { if (!child) @@ -48,6 +40,14 @@ void UserDefinedSQLFunctionVisitor::visit(ASTPtr & ast) if (new_ptr != old_ptr) ast->updatePointerToChild(old_ptr, new_ptr); } + + if (const auto * function = ast->template as()) + { + std::unordered_set udf_in_replace_process; + auto replace_result = tryToReplaceFunction(*function, udf_in_replace_process); + if (replace_result) + ast = replace_result; + } } void UserDefinedSQLFunctionVisitor::visit(IAST * ast) diff --git a/src/Interpreters/CrossToInnerJoinVisitor.cpp b/src/Interpreters/CrossToInnerJoinVisitor.cpp index e3e8b80e437..dbe10045c98 100644 --- a/src/Interpreters/CrossToInnerJoinVisitor.cpp +++ b/src/Interpreters/CrossToInnerJoinVisitor.cpp @@ -70,7 +70,7 @@ struct JoinedElement join->strictness = JoinStrictness::All; join->on_expression = on_expression; - join->children.push_back(join->on_expression); + join->children = {join->on_expression}; return true; } diff --git a/src/Parsers/ASTColumnDeclaration.cpp b/src/Parsers/ASTColumnDeclaration.cpp index 1c7d72bafcc..d96a52da61c 100644 --- a/src/Parsers/ASTColumnDeclaration.cpp +++ b/src/Parsers/ASTColumnDeclaration.cpp @@ -130,12 +130,25 @@ void ASTColumnDeclaration::formatImpl(const FormatSettings & format_settings, Fo void ASTColumnDeclaration::forEachPointerToChild(std::function f) { - f(reinterpret_cast(&default_expression)); - f(reinterpret_cast(&comment)); - f(reinterpret_cast(&codec)); - f(reinterpret_cast(&statistics_desc)); - f(reinterpret_cast(&ttl)); - f(reinterpret_cast(&collation)); - f(reinterpret_cast(&settings)); + auto visit_child = [&f](ASTPtr & member) + { + IAST * new_member_ptr = member.get(); + f(reinterpret_cast(&new_member_ptr)); + if (new_member_ptr != member.get()) + { + if (new_member_ptr) + member = new_member_ptr->ptr(); + else + member.reset(); + } + }; + + visit_child(default_expression); + visit_child(comment); + visit_child(codec); + visit_child(statistics_desc); + visit_child(ttl); + visit_child(collation); + visit_child(settings); } } diff --git a/src/Parsers/ASTTablesInSelectQuery.cpp b/src/Parsers/ASTTablesInSelectQuery.cpp index b6d42513aa7..0046f603c7d 100644 --- a/src/Parsers/ASTTablesInSelectQuery.cpp +++ b/src/Parsers/ASTTablesInSelectQuery.cpp @@ -61,6 +61,29 @@ ASTPtr ASTTableJoin::clone() const return res; } +void ASTTableJoin::forEachPointerToChild(std::function f) +{ + IAST * new_using_expression_list = using_expression_list.get(); + f(reinterpret_cast(&new_using_expression_list)); + if (new_using_expression_list != using_expression_list.get()) + { + if (new_using_expression_list) + using_expression_list = new_using_expression_list->ptr(); + else + using_expression_list.reset(); + } + + IAST * new_on_expression = on_expression.get(); + f(reinterpret_cast(&new_on_expression)); + if (new_on_expression != on_expression.get()) + { + if (new_on_expression) + on_expression = new_on_expression->ptr(); + else + on_expression.reset(); + } +} + void ASTArrayJoin::updateTreeHashImpl(SipHash & hash_state, bool ignore_aliases) const { hash_state.update(kind); diff --git a/src/Parsers/ASTTablesInSelectQuery.h b/src/Parsers/ASTTablesInSelectQuery.h index f3f329ca2b6..ab32e51a1b4 100644 --- a/src/Parsers/ASTTablesInSelectQuery.h +++ b/src/Parsers/ASTTablesInSelectQuery.h @@ -80,6 +80,9 @@ struct ASTTableJoin : public IAST void formatImplAfterTable(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const; void formatImpl(const FormatSettings & settings, FormatState & state, FormatStateStacked frame) const override; void updateTreeHashImpl(SipHash & hash_state, bool ignore_aliases) const override; + +protected: + void forEachPointerToChild(std::function f) override; }; /// Specification of ARRAY JOIN. diff --git a/tests/queries/0_stateless/03274_udf_in_join.reference b/tests/queries/0_stateless/03274_udf_in_join.reference new file mode 100644 index 00000000000..c05f6ac7c8a --- /dev/null +++ b/tests/queries/0_stateless/03274_udf_in_join.reference @@ -0,0 +1,7 @@ +SELECT 1 +FROM +( + SELECT 1 AS c0 +) AS v0 +ALL INNER JOIN v0 AS vx ON c0 = vx.c0 +1 diff --git a/tests/queries/0_stateless/03274_udf_in_join.sh b/tests/queries/0_stateless/03274_udf_in_join.sh new file mode 100755 index 00000000000..052534d7a3c --- /dev/null +++ b/tests/queries/0_stateless/03274_udf_in_join.sh @@ -0,0 +1,21 @@ +#!/usr/bin/env bash + +CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CUR_DIR"/../shell_config.sh + +$CLICKHOUSE_CLIENT -q " + CREATE VIEW v0 AS SELECT 1 AS c0; + CREATE FUNCTION ${CLICKHOUSE_DATABASE}_second AS (x, y) -> y; + CREATE FUNCTION ${CLICKHOUSE_DATABASE}_equals AS (x, y) -> x = y; + -- SET optimize_rewrite_array_exists_to_has = 1; + + EXPLAIN SYNTAX SELECT 1 FROM v0 JOIN v0 vx ON ${CLICKHOUSE_DATABASE}_second(v0.c0, vx.c0); -- { serverError INVALID_JOIN_ON_EXPRESSION } + EXPLAIN SYNTAX SELECT 1 FROM v0 JOIN v0 vx ON ${CLICKHOUSE_DATABASE}_equals(v0.c0, vx.c0); + + SELECT 1 FROM v0 JOIN v0 vx ON ${CLICKHOUSE_DATABASE}_equals(v0.c0, vx.c0); + + DROP view v0; + DROP FUNCTION ${CLICKHOUSE_DATABASE}_second; + DROP FUNCTION ${CLICKHOUSE_DATABASE}_equals; +" From f6610790a84ca2ff13cb2dc69d8b6cd9f0ff1954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Wed, 20 Nov 2024 17:44:01 +0100 Subject: [PATCH 2/2] Fix broken check --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- tests/ci/run_check.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 976c69d3c34..b4ca4cb8d98 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -12,7 +12,7 @@ tests/ci/cancel_and_rerun_workflow_lambda/app.py - Backward Incompatible Change - Build/Testing/Packaging Improvement - Documentation (changelog entry is not required) -- Critical Bug Fix (crash, data loss, RBAC) +- Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR - Bug Fix (user-visible misbehavior in an official stable release) - CI Fix or Improvement (changelog entry is not required) - Not for changelog (changelog entry is not required) diff --git a/tests/ci/run_check.py b/tests/ci/run_check.py index 7f665165c59..61c3c0c4ec4 100644 --- a/tests/ci/run_check.py +++ b/tests/ci/run_check.py @@ -56,7 +56,9 @@ LABEL_CATEGORIES = { "Bug Fix (user-visible misbehaviour in official stable or prestable release)", "Bug Fix (user-visible misbehavior in official stable or prestable release)", ], - "pr-critical-bugfix": ["Critical Bug Fix (crash, LOGICAL_ERROR, data loss, RBAC)"], + "pr-critical-bugfix": [ + "Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR" + ], "pr-build": [ "Build/Testing/Packaging Improvement", "Build Improvement",