-
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
Fix final=1 for distributed over non-mt table. #64037
Conversation
This is an automated comment for commit d1127bf 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
|
b69b9a2
to
d1127bf
Compare
clang tidy build fails looks like unrelated, maybe merging master will fix |
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.
LGTM
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.
LGTM
@@ -192,7 +192,7 @@ void QueryTreePassManager::run(QueryTreeNodePtr query_tree_node) | |||
void QueryTreePassManager::runOnlyResolve(QueryTreeNodePtr query_tree_node) | |||
{ | |||
// Run only QueryAnalysisPass and GroupingFunctionsResolvePass passes. | |||
run(query_tree_node, 2); | |||
run(query_tree_node, 3); |
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.
Probably we need to invent something better than indexing here.
@@ -42,7 +42,7 @@ class AutoFinalOnQueryPassVisitor : public InDepthQueryTreeVisitorWithContext<Au | |||
return; | |||
|
|||
const auto & storage = table_node ? table_node->getStorage() : table_function_node->getStorage(); | |||
bool is_final_supported = storage && storage->supportsFinal(); | |||
bool is_final_supported = storage && !storage->isRemote() && storage->supportsFinal(); |
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.
Just curious: how did it work in old infrastructure?
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.
Same. I have copied from this
bool is_final_supported = storage && storage->supportsFinal() && !storage->isRemote() && query.tables(); |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Do not throw
Storage doesn't support FINAL
error for remote queries over non-MergeTree tables withfinal = true
and new analyzer. Fixes #63960