Fix broken SET reuse during projection analysis. #35631
Conversation
amosbird
force-pushed
the
projection-fix1
branch
from
0653122
to
6749e84
Compare
|
@Mergifyio update |
✅ Branch has been successfully updated |
| @@ -205,6 +208,7 @@ class InterpreterSelectQuery : public IInterpreterUnionOrSelectQuery | |||
| StorageSnapshotPtr storage_snapshot; | |||
|
|
|||
| /// Reuse already built sets for multiple passes of analysis, possibly across interpreters. | |||
| std::shared_ptr<SubqueriesForSets> subquery_for_sets; | |||
There was a problem hiding this comment.
You don't share ownership over subquery_for_sets so there is no need to use std::shared_ptr.
You can just store SubqueriesForSets as well as prepared_sets or if you want to store it in a separate memory use std::unique_ptr.
There was a problem hiding this comment.
The reason to use std::shared_ptr is somehow hacky: to make it copyable.
There was a problem hiding this comment.
I understand but I don't see where it's used for the creation of the copy.
There was a problem hiding this comment.
It's because we need SelectQueryInfo to be copyable.
https://github.com/ClickHouse/ClickHouse/blob/master/src/Storages/StorageMerge.cpp#L218
I'll use a different implementation.
| @@ -551,8 +550,8 @@ InterpreterSelectQuery::InterpreterSelectQuery( | |||
| LOG_TRACE(log, "Running 'analyze' second time"); | |||
|
|
|||
| /// Reuse already built sets for multiple passes of analysis | |||
| subquery_for_sets = std::move(query_analyzer->getSubqueriesForSets()); | |||
| prepared_sets = query_info.sets.empty() ? query_analyzer->getPreparedSets() : query_info.sets; | |||
| subquery_for_sets = std::make_shared<SubqueriesForSets>(std::move(query_analyzer->getSubqueriesForSets())); | |||
There was a problem hiding this comment.
subquery_for_sets is always initialized, so I'd prefer to use here move-assignment without new memory allocation.
There was a problem hiding this comment.
I'll add a comment why shared_ptr is used.
| query_info.sets = select.getQueryAnalyzer()->getPreparedSets(); | ||
| query_info.subquery_for_sets = std::make_shared<SubqueriesForSets>(std::move(select.getQueryAnalyzer()->getSubqueriesForSets())); |
amosbird
force-pushed
the
projection-fix1
branch
2 times, most recently
from
141f92d
to
e9dc26d
Compare
amosbird
force-pushed
the
projection-fix1
branch
from
e9dc26d
to
1238bab
Compare
robot-ch-test-poll
added
the
pr-bugfix
|
@Mergifyio update |
✅ Branch has been successfully updated |
novikd
added
the
pr-must-backport
Manual backport #35631 to 22.3: Fix broken SET reuse during projection analysis.
Felixoid
added
the
pr-backports-created
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix projection analysis which might lead to wrong query result when IN subquery is used. This fixes #35336