Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not rewrite sum() to count() if return value differs in analyzer #59926

Merged
merged 2 commits into from Feb 16, 2024
Merged

Do not rewrite sum() to count() if return value differs in analyzer #59926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c01705
Run ValidationChecker for sanitizers build as well
azat Feb 13, 2024
504a2dd
Do not rewrite sum() to count() if return value differs in analyzer
azat Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Analyzer/Passes/NormalizeCountVariantsPass.cpp
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@
#include <Analyzer/ConstantNode.h>
#include <Analyzer/FunctionNode.h>
#include <Interpreters/Context.h>
#include <DataTypes/DataTypesNumber.h>

namespace DB
{
Expand All @@ -32,6 +33,11 @@ class NormalizeCountVariantsVisitor : public InDepthQueryTreeVisitorWithContext<
if (function_node->getArguments().getNodes().size() != 1)
return;

/// forbid the optimization if return value of sum() and count() differs:
/// count() returns only UInt64 type, while sum() could return Nullable().
if (!function_node->getResultType()->equals(DataTypeUInt64()))
return;

auto & first_argument = function_node->getArguments().getNodes()[0];
auto * first_argument_constant_node = first_argument->as<ConstantNode>();
if (!first_argument_constant_node)
Expand Down
6 changes: 3 additions & 3 deletions src/Analyzer/QueryTreePassManager.cpp
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ namespace ErrorCodes
namespace
{

#ifndef NDEBUG
#if defined(ABORT_ON_LOGICAL_ERROR)
azat marked this conversation as resolved.
Show resolved Hide resolved

/** This visitor checks if Query Tree structure is valid after each pass
* in debug build.
Expand Down Expand Up @@ -184,7 +184,7 @@ void QueryTreePassManager::run(QueryTreeNodePtr query_tree_node)
for (size_t i = 0; i < passes_size; ++i)
{
passes[i]->run(query_tree_node, current_context);
#ifndef NDEBUG
#if defined(ABORT_ON_LOGICAL_ERROR)
ValidationChecker(passes[i]->getName()).visit(query_tree_node);
#endif
}
Expand All @@ -209,7 +209,7 @@ void QueryTreePassManager::run(QueryTreeNodePtr query_tree_node, size_t up_to_pa
for (size_t i = 0; i < up_to_pass_index; ++i)
{
passes[i]->run(query_tree_node, current_context);
#ifndef NDEBUG
#if defined(ABORT_ON_LOGICAL_ERROR)
ValidationChecker(passes[i]->getName()).visit(query_tree_node);
#endif
}
Expand Down
4 changes: 4 additions & 0 deletions tests/queries/0_stateless/02991_count_rewrite_analyzer.reference
View file
Open in desktop
@@ -0,0 +1,4 @@
Nullable(UInt64)
UInt64
Nullable(UInt64)
UInt64
7 changes: 7 additions & 0 deletions tests/queries/0_stateless/02991_count_rewrite_analyzer.sql
View file
Open in desktop
@@ -0,0 +1,7 @@
-- Regression test for https://github.com/ClickHouse/ClickHouse/issues/59919
SET allow_experimental_analyzer=1;

SELECT toTypeName(sum(toNullable('a') IN toNullable('a'))) AS x;
SELECT toTypeName(count(toNullable('a') IN toNullable('a'))) AS x;
SELECT toTypeName(sum(toFixedString('a', toLowCardinality(toNullable(1))) IN toFixedString('a', 1))) AS x;
SELECT toTypeName(count(toFixedString('a', toLowCardinality(toNullable(1))) IN toFixedString('a', 1))) AS x;