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
Analyzer: fix storage replacement with insertion block #58958
Conversation
This is an automated comment for commit 7826706 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.
Do we have a tests dedicated to this specific case? If not it's worth adding
if (auto * table_node = table_expression_to_replace->as<TableNode>(); table_node && table_node->getStorageID().getFullNameNotQuoted() != storage->getStorageID().getFullNameNotQuoted()) | ||
return; | ||
/// Don't replace storage if table name differs | ||
if (table_node.getStorageID().getFullNameNotQuoted() != storage->getStorageID().getFullNameNotQuoted()) |
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.
Wouldn't it be better to compare uuid
s? Though, it's difficult to imagine a case when comparing full names is a problem.
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. We need to finish it ASAP.
I pushed some changes after the review. Let's merge this PR if CI doesn't find any issues. I don't think full table name vs UUID comparison is a stopper here. |
src/Analyzer/Utils.cpp
Outdated
for (const auto & argument_node : node_to_process->as<TableFunctionNode>()->getArgumentsNode()->getChildren()) | ||
nodes_to_process.push_back(argument_node); |
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 suspect that tests fail because it's incorrect. There are no TableNodes in the table function arguments. There can be QueryNodes but they are unresolved, we can't do anything with them.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix analyzer - insertion from select with subquery referencing insertion table should process only insertion block for all table expressions.
Fixes #58080.
follow-up #50857