-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Check what would be broken if do not add all the identifiers to functions map. #63993
Check what would be broken if do not add all the identifiers to functions map. #63993
Conversation
This is an automated comment for commit 741e0ae 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
|
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.
In general LGTM. It's better to remove the commented code, QueryAnalysisPass.cpp
is already quite big.
@@ -471,7 +471,6 @@ struct TableExpressionData | |||
return buffer.str(); | |||
} | |||
}; | |||
|
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.
Accidental change?
if (!alias_node) | ||
throw Exception(ErrorCodes::LOGICAL_ERROR, | ||
"Node with alias {} is not valid. In scope {}", | ||
identifier_bind_part, |
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.
I think it's better to move this check inside of ScopeAliases
and make ScopeAliases::find
return QueryTreeNodePtr
instead of QueryTreeNodePtr *
. This line checks that the alias map is filled with correct data.
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.
I don't know how to do it properly:
- There is a place where it's possible to not find the alias:
ClickHouse/src/Analyzer/Passes/QueryAnalysisPass.cpp
Lines 4198 to 4204 in 51afec4
auto * alias_it = scope.aliases.find(identifier_lookup, ScopeAliases::FindOption::FULL_NAME); //auto alias_it = scope.alias_name_to_expression_node->find(identifier_lookup.identifier.getFullName()); if (alias_it && (*alias_it)->getNodeType() == QueryTreeNodeType::COLUMN) { const auto & column_node = (*alias_it)->as<ColumnNode &>(); if (column_node.getColumnSource()->getNodeType() == QueryTreeNodeType::ARRAY_JOIN) prefer_column_name_to_alias = true; - We can replace alias node to resolved one later
alias_node = lookup_result.resolved_identifier;
So I can't return QueryTreeNodePtr
because it won't be possible to update the alias map, and I can't use QueryTreeNodePtr &
because it's ok to not find an alias, and I can't use std::optional<QueryTreeNodePtr &>
because this is not supported.
Co-authored-by: Dmitry Novik <n0vik@clickhouse.com>
…0d3ac170eb3e26e2f4fb59cd1addb Cherry pick #63993 to 24.5: Check what would be broken if do not add all the identifiers to functions map.
… the identifiers to functions map.
Backport #63993 to 24.5: Check what would be broken if do not add all the identifiers to functions map.
…0d3ac170eb3e26e2f4fb59cd1addb Cherry pick #63993 to 24.3: Check what would be broken if do not add all the identifiers to functions map.
… the identifiers to functions map.
…0d3ac170eb3e26e2f4fb59cd1addb Cherry pick #63993 to 24.4: Check what would be broken if do not add all the identifiers to functions map.
… the identifiers to functions map.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix an
Cyclic aliases
error for cyclic aliases of different type (expression and function). Fixes #63205