From 947a489ded05aa86b509d1b45cbf52429a8999e6 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Thu, 7 Nov 2024 10:06:54 +0000 Subject: [PATCH 1/4] Backport #71541 to 24.3: Avoid crash when using a UDF in a constraint --- .../UserDefinedSQLFunctionVisitor.cpp | 99 +++---------------- src/Parsers/ASTColumnDeclaration.cpp | 10 ++ src/Parsers/ASTColumnDeclaration.h | 3 + .../03262_udf_in_constraint.reference | 2 + .../0_stateless/03262_udf_in_constraint.sh | 17 ++++ 5 files changed, 45 insertions(+), 86 deletions(-) create mode 100644 tests/queries/0_stateless/03262_udf_in_constraint.reference create mode 100755 tests/queries/0_stateless/03262_udf_in_constraint.sh diff --git a/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp b/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp index ebd65471449..a04b8d7b998 100644 --- a/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp +++ b/src/Functions/UserDefined/UserDefinedSQLFunctionVisitor.cpp @@ -24,92 +24,7 @@ namespace ErrorCodes void UserDefinedSQLFunctionVisitor::visit(ASTPtr & ast) { - if (!ast) - { - chassert(false); - return; - } - - /// FIXME: this helper should use updatePointerToChild(), but - /// forEachPointerToChild() is not implemented for ASTColumnDeclaration - /// (and also some members should be adjusted for this). - const auto visit_child_with_shared_ptr = [&](ASTPtr & child) - { - if (!child) - return; - - auto * old_value = child.get(); - visit(child); - - // child did not change - if (old_value == child.get()) - return; - - // child changed, we need to modify it in the list of children of the parent also - for (auto & current_child : ast->children) - { - if (current_child.get() == old_value) - current_child = child; - } - }; - - if (auto * col_decl = ast->as()) - { - visit_child_with_shared_ptr(col_decl->default_expression); - visit_child_with_shared_ptr(col_decl->ttl); - return; - } - - if (auto * storage = ast->as()) - { - const auto visit_child = [&](IAST * & child) - { - if (!child) - return; - - if (const auto * function = child->template as()) - { - std::unordered_set udf_in_replace_process; - auto replace_result = tryToReplaceFunction(*function, udf_in_replace_process); - if (replace_result) - ast->setOrReplace(child, replace_result); - } - - visit(child); - }; - - visit_child(storage->partition_by); - visit_child(storage->primary_key); - visit_child(storage->order_by); - visit_child(storage->sample_by); - visit_child(storage->ttl_table); - - return; - } - - if (auto * alter = ast->as()) - { - /// It is OK to use updatePointerToChild() because ASTAlterCommand implements forEachPointerToChild() - const auto visit_child_update_parent = [&](ASTPtr & child) - { - if (!child) - return; - - auto * old_ptr = child.get(); - visit(child); - auto * new_ptr = child.get(); - - /// Some AST classes have naked pointers to children elements as members. - /// We have to replace them if the child was replaced. - if (new_ptr != old_ptr) - ast->updatePointerToChild(old_ptr, new_ptr); - }; - - for (auto & children : alter->children) - visit_child_update_parent(children); - - return; - } + chassert(ast); if (const auto * function = ast->template as()) { @@ -120,7 +35,19 @@ void UserDefinedSQLFunctionVisitor::visit(ASTPtr & ast) } for (auto & child : ast->children) + { + if (!child) + return; + + auto * old_ptr = child.get(); visit(child); + auto * new_ptr = child.get(); + + /// Some AST classes have naked pointers to children elements as members. + /// We have to replace them if the child was replaced. + if (new_ptr != old_ptr) + ast->updatePointerToChild(old_ptr, new_ptr); + } } void UserDefinedSQLFunctionVisitor::visit(IAST * ast) diff --git a/src/Parsers/ASTColumnDeclaration.cpp b/src/Parsers/ASTColumnDeclaration.cpp index 6c29e0bf9d5..b46ad368ce2 100644 --- a/src/Parsers/ASTColumnDeclaration.cpp +++ b/src/Parsers/ASTColumnDeclaration.cpp @@ -137,4 +137,14 @@ 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)); +} } diff --git a/src/Parsers/ASTColumnDeclaration.h b/src/Parsers/ASTColumnDeclaration.h index d775928d05c..0467ee95d9f 100644 --- a/src/Parsers/ASTColumnDeclaration.h +++ b/src/Parsers/ASTColumnDeclaration.h @@ -29,6 +29,9 @@ public: ASTPtr clone() const override; void formatImpl(const FormatSettings & format_settings, FormatState & state, FormatStateStacked frame) const override; + +protected: + void forEachPointerToChild(std::function f) override; }; } diff --git a/tests/queries/0_stateless/03262_udf_in_constraint.reference b/tests/queries/0_stateless/03262_udf_in_constraint.reference new file mode 100644 index 00000000000..29d403b85a8 --- /dev/null +++ b/tests/queries/0_stateless/03262_udf_in_constraint.reference @@ -0,0 +1,2 @@ +CREATE TABLE default.t0\n(\n `c0` Int32,\n CONSTRAINT c1 CHECK c0 > 5\n)\nENGINE = MergeTree\nORDER BY tuple()\nSETTINGS index_granularity = 8192 +10 diff --git a/tests/queries/0_stateless/03262_udf_in_constraint.sh b/tests/queries/0_stateless/03262_udf_in_constraint.sh new file mode 100755 index 00000000000..3c36e7caeb4 --- /dev/null +++ b/tests/queries/0_stateless/03262_udf_in_constraint.sh @@ -0,0 +1,17 @@ +#!/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 FUNCTION ${CLICKHOUSE_DATABASE}_function AS (x) -> x > 5; + CREATE TABLE t0 (c0 Int, CONSTRAINT c1 CHECK ${CLICKHOUSE_DATABASE}_function(c0)) ENGINE = MergeTree() ORDER BY tuple(); + SHOW CREATE TABLE t0; + INSERT INTO t0(c0) VALUES (10); + INSERT INTO t0(c0) VALUES (3); -- {serverError VIOLATED_CONSTRAINT} + SELECT * FROM t0; + + DROP TABLE t0; + DROP FUNCTION ${CLICKHOUSE_DATABASE}_function; +" From e7fd47a3cd9876143cc734f26ac6ca05f3d1f35e Mon Sep 17 00:00:00 2001 From: Vladimir Cherkasov Date: Thu, 7 Nov 2024 16:06:46 +0100 Subject: [PATCH 2/4] fix build --- src/Parsers/ASTColumnDeclaration.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Parsers/ASTColumnDeclaration.cpp b/src/Parsers/ASTColumnDeclaration.cpp index b46ad368ce2..5e78cf4f8b3 100644 --- a/src/Parsers/ASTColumnDeclaration.cpp +++ b/src/Parsers/ASTColumnDeclaration.cpp @@ -142,7 +142,6 @@ 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)); From 058872beadabb17fcf69ede527972383f487f1f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 7 Nov 2024 19:31:04 +0100 Subject: [PATCH 3/4] Update ASTColumnDeclaration.cpp --- src/Parsers/ASTColumnDeclaration.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Parsers/ASTColumnDeclaration.cpp b/src/Parsers/ASTColumnDeclaration.cpp index 5e78cf4f8b3..0c38c145196 100644 --- a/src/Parsers/ASTColumnDeclaration.cpp +++ b/src/Parsers/ASTColumnDeclaration.cpp @@ -142,6 +142,7 @@ void ASTColumnDeclaration::forEachPointerToChild(std::function f) f(reinterpret_cast(&default_expression)); f(reinterpret_cast(&comment)); f(reinterpret_cast(&codec)); + f(reinterpret_cast(&stat_type)); f(reinterpret_cast(&ttl)); f(reinterpret_cast(&collation)); f(reinterpret_cast(&settings)); From 9253403d12ae6d181309aa1a31f33e663b1d8cec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Thu, 7 Nov 2024 19:31:47 +0100 Subject: [PATCH 4/4] Update 03262_udf_in_constraint.sh --- tests/queries/0_stateless/03262_udf_in_constraint.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/03262_udf_in_constraint.sh b/tests/queries/0_stateless/03262_udf_in_constraint.sh index 3c36e7caeb4..1ef49e93c0d 100755 --- a/tests/queries/0_stateless/03262_udf_in_constraint.sh +++ b/tests/queries/0_stateless/03262_udf_in_constraint.sh @@ -4,7 +4,7 @@ CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) # shellcheck source=../shell_config.sh . "$CUR_DIR"/../shell_config.sh -$CLICKHOUSE_CLIENT -q " +$CLICKHOUSE_CLIENT -nm -q " CREATE FUNCTION ${CLICKHOUSE_DATABASE}_function AS (x) -> x > 5; CREATE TABLE t0 (c0 Int, CONSTRAINT c1 CHECK ${CLICKHOUSE_DATABASE}_function(c0)) ENGINE = MergeTree() ORDER BY tuple(); SHOW CREATE TABLE t0;