From 2273acaa30548da72c31f39bb5f8adde06ce4a95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 20 Feb 2023 16:47:25 +0100 Subject: [PATCH 1/3] Fix incorrect alias recursion in QueryNormalizer --- src/Interpreters/QueryNormalizer.cpp | 4 +++ ...2667_order_by_aggregation_result.reference | 4 +++ .../02667_order_by_aggregation_result.sql | 36 +++++++++++++++++++ 3 files changed, 44 insertions(+) create mode 100644 tests/queries/0_stateless/02667_order_by_aggregation_result.reference create mode 100644 tests/queries/0_stateless/02667_order_by_aggregation_result.sql diff --git a/src/Interpreters/QueryNormalizer.cpp b/src/Interpreters/QueryNormalizer.cpp index 4db61501d3d..df454aecac1 100644 --- a/src/Interpreters/QueryNormalizer.cpp +++ b/src/Interpreters/QueryNormalizer.cpp @@ -118,6 +118,8 @@ void QueryNormalizer::visit(ASTIdentifier & node, ASTPtr & ast, Data & data) alias_node->checkSize(data.settings.max_expanded_ast_elements); ast = alias_node->clone(); ast->setAlias(node_alias); + if (data.finished_asts.contains(alias_node)) + data.finished_asts[ast] = ast; } } else @@ -127,6 +129,8 @@ void QueryNormalizer::visit(ASTIdentifier & node, ASTPtr & ast, Data & data) auto alias_name = ast->getAliasOrColumnName(); ast = alias_node->clone(); ast->setAlias(alias_name); + if (data.finished_asts.contains(alias_node)) + data.finished_asts[ast] = ast; } } } diff --git a/tests/queries/0_stateless/02667_order_by_aggregation_result.reference b/tests/queries/0_stateless/02667_order_by_aggregation_result.reference new file mode 100644 index 00000000000..642fc2ed645 --- /dev/null +++ b/tests/queries/0_stateless/02667_order_by_aggregation_result.reference @@ -0,0 +1,4 @@ +0 0 +0 1 █████████████████████████████████████████████████▉ +1 2 0.5 +1 0.1 1.1 diff --git a/tests/queries/0_stateless/02667_order_by_aggregation_result.sql b/tests/queries/0_stateless/02667_order_by_aggregation_result.sql new file mode 100644 index 00000000000..277430888d7 --- /dev/null +++ b/tests/queries/0_stateless/02667_order_by_aggregation_result.sql @@ -0,0 +1,36 @@ +-- Github issues: +-- - https://github.com/ClickHouse/ClickHouse/issues/46268 +-- - https://github.com/ClickHouse/ClickHouse/issues/46273 + +-- Queries that the original PR (https://github.com/ClickHouse/ClickHouse/pull/42827) tried to fix +SELECT (number = 1) AND (number = 2) AS value, sum(value) OVER () FROM numbers(1) WHERE 1; +SELECT time, round(exp_smooth, 10), bar(exp_smooth, -9223372036854775807, 1048575, 50) AS bar FROM (SELECT 2 OR (number = 0) OR (number >= 1) AS value, number AS time, exponentialTimeDecayedSum(2147483646)(value, time) OVER (RANGE BETWEEN CURRENT ROW AND CURRENT ROW) AS exp_smooth FROM numbers(1) WHERE 10) WHERE 25; + +CREATE TABLE ttttttt +( + `timestamp` DateTime, + `col1` Float64, + `col2` Float64, + `col3` Float64 +) +ENGINE = MergeTree() +ORDER BY tuple(); + +INSERT INTO ttttttt VALUES ('2023-02-20 00:00:00', 1, 2, 3); + +-- Query that https://github.com/ClickHouse/ClickHouse/pull/42827 broke +SELECT + argMax(col1, timestamp) AS col1, + argMax(col2, timestamp) AS col2, + col1 / col2 AS final_col +FROM ttttttt +GROUP BY + col3 +ORDER BY final_col DESC; + +SELECT + argMax(col1, timestamp) AS col1, + col1 / 10 AS final_col, + final_col + 1 AS final_col2 +FROM ttttttt +GROUP BY col3; From 87a4fb025275d1837868952f7f47e0305023b230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 20 Feb 2023 19:08:54 +0100 Subject: [PATCH 2/3] Update aliases when clone happens --- src/Interpreters/QueryNormalizer.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/Interpreters/QueryNormalizer.cpp b/src/Interpreters/QueryNormalizer.cpp index df454aecac1..56b81b3d224 100644 --- a/src/Interpreters/QueryNormalizer.cpp +++ b/src/Interpreters/QueryNormalizer.cpp @@ -118,8 +118,15 @@ void QueryNormalizer::visit(ASTIdentifier & node, ASTPtr & ast, Data & data) alias_node->checkSize(data.settings.max_expanded_ast_elements); ast = alias_node->clone(); ast->setAlias(node_alias); + + /// If the cloned AST was finished, this one should also be considered finished if (data.finished_asts.contains(alias_node)) data.finished_asts[ast] = ast; + + /// If we had an alias for node_alias, point it instead to the new node so we don't have to revisit it + /// on subsequent calls + if (auto existing_alias = data.aliases.find(node_alias); existing_alias != data.aliases.end()) + existing_alias->second = ast; } } else @@ -129,8 +136,15 @@ void QueryNormalizer::visit(ASTIdentifier & node, ASTPtr & ast, Data & data) auto alias_name = ast->getAliasOrColumnName(); ast = alias_node->clone(); ast->setAlias(alias_name); + + /// If the cloned AST was finished, this one should also be considered finished if (data.finished_asts.contains(alias_node)) data.finished_asts[ast] = ast; + + /// If we had an alias for node_alias, point it instead to the new node so we don't have to revisit it + /// on subsequent calls + if (auto existing_alias = data.aliases.find(node_alias); existing_alias != data.aliases.end()) + existing_alias->second = ast; } } } From e8094c9707d46ef950232e475f671627b809a46b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Wed, 22 Feb 2023 14:20:48 +0100 Subject: [PATCH 3/3] Add test for #46724 --- ...2667_order_by_aggregation_result.reference | 3 +++ .../02667_order_by_aggregation_result.sql | 24 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/queries/0_stateless/02667_order_by_aggregation_result.reference b/tests/queries/0_stateless/02667_order_by_aggregation_result.reference index 642fc2ed645..a89e39d87b3 100644 --- a/tests/queries/0_stateless/02667_order_by_aggregation_result.reference +++ b/tests/queries/0_stateless/02667_order_by_aggregation_result.reference @@ -2,3 +2,6 @@ 0 1 █████████████████████████████████████████████████▉ 1 2 0.5 1 0.1 1.1 +00000000-0000-0000-0000-000000000000 b 1 +417ddc5d-e556-4d27-95dd-a34d84e46a50 c 1 +notEmpty a 1 diff --git a/tests/queries/0_stateless/02667_order_by_aggregation_result.sql b/tests/queries/0_stateless/02667_order_by_aggregation_result.sql index 277430888d7..3fef0374d83 100644 --- a/tests/queries/0_stateless/02667_order_by_aggregation_result.sql +++ b/tests/queries/0_stateless/02667_order_by_aggregation_result.sql @@ -34,3 +34,27 @@ SELECT final_col + 1 AS final_col2 FROM ttttttt GROUP BY col3; + +-- https://github.com/ClickHouse/ClickHouse/issues/46724 + +CREATE TABLE table1 +( + id String, + device UUID +) +ENGINE = MergeTree() ORDER BY tuple(); + +INSERT INTO table1 VALUES ('notEmpty', '417ddc5d-e556-4d27-95dd-a34d84e46a50'); +INSERT INTO table1 VALUES ('', '417ddc5d-e556-4d27-95dd-a34d84e46a50'); +INSERT INTO table1 VALUES ('', '00000000-0000-0000-0000-000000000000'); + +SELECT + if(empty(id), toString(device), id) AS device, + multiIf( + notEmpty(id),'a', + device == '00000000-0000-0000-0000-000000000000', 'b', + 'c' ) AS device_id_type, + count() +FROM table1 +GROUP BY device, device_id_type +ORDER BY device;