Refactor parameterized view & add support for new analyzer#54211
Refactor parameterized view & add support for new analyzer#54211
Conversation
|
This is an automated comment for commit 8397b85 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
|
| if (!is_parameterized_view_) | ||
| { | ||
| if (!query.isParameterizedView()) |
There was a problem hiding this comment.
This looks a bit strange, shouldn't we expect that is_parametrized_view <=> query.isParametrizedView()?
There was a problem hiding this comment.
isParametrizedView() is only for CREATE queries, not SELECT. But I get your point, the names are confusing. Will update
There was a problem hiding this comment.
Added comment, Hope this clarifies
| if (table_function_node->getOriginalAST()->as<ASTFunction>()->is_compound_name) | ||
| { | ||
| std::vector<std::string> parts; | ||
| splitInto<'.'>(parts, table_function_node->getOriginalAST()->as<ASTFunction>()->name); |
There was a problem hiding this comment.
what if table name or database name contains dots?
There was a problem hiding this comment.
This is how we separated the names here as well (https://github.com/ClickHouse/ClickHouse/blob/master/src/Interpreters/Context.cpp#L1718) , so we dont support if table name had '.'.
Is it supported for normal table creation with '.' ?
src/Analyzer/TableFunctionNode.cpp
Outdated
| } | ||
|
|
||
| void TableFunctionNode::resolve(StoragePtr storage_value, ContextPtr context) | ||
| { |
There was a problem hiding this comment.
But now table_function is not assigned, so method isResolved will return false, even though resolve is called. Seems this will lead to incorrect behaviour? Why cannot we use resolve from void resolve(TableFunctionPtr table_function_value, StoragePtr storage_value, ContextPtr context, std::vector<size_t> unresolved_arguments_indexes_);?
There was a problem hiding this comment.
For parameteriezed view, there is no TableFunctionPtr. I will update isResolved to only consider Storage. Please let me know if you have any suggestions
|
@SmitaRKulkarni @kssenii The implementation of parametrized views in the new analyzer was wrong. I've rewritten it almost completely. Now it's okay. Please, review it and let's merge if tests pass. |
Backport #54211 to 23.8: Refactor parameterized view & add support for new analyzer
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added support for parameterized view with analyzer to not analyze create parameterized view.
Refactor existing parameterized view logic to not analyze create parameterized view.