-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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.
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
Disable optimize_rewrite_sum_if_to_count_if
if return type is nullable (new analyzer)
#61389
Conversation
@@ -32,7 +32,7 @@ class SumIfToCountIfVisitor : public InDepthQueryTreeVisitorWithContext<SumIfToC | |||
return; | |||
|
|||
auto * function_node = node->as<FunctionNode>(); | |||
if (!function_node || !function_node->isAggregateFunction()) | |||
if (!function_node || !function_node->isAggregateFunction() || function_node->getResultType()->isNullable()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me of f0354ee.
Both bugs are introduced because getValue()
ignores nullability.
Probably constant_value_literal.getType()
should be changed to literal->getResultType()
, and getType()
should be refactored to something with a completely different name to avoid this confusion again as getResultType()
and getType()
and pretty much identical and getType()
does unexpected things. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it was confusing at first when reading the code because you don't pay attention to the fact that the Nullability info is lost when you get Field
not sure what would be the best solution
This is an automated comment for commit 060f798 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
d675ff5
to
b000849
Compare
Changelog category (leave one):
https://s3.amazonaws.com/clickhouse-test-reports/0/0b97f3ac16507cd72ed4e33739d355d16a48a741/ast_fuzzer__tsan_.html
In old analyzer it was disabled because
toNullable
is a function and notASTLiteral
.Documentation entry for user-facing changes