-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Use ActionsDAG in KeyCondition #25563
Conversation
…se into use-dag-in-key-condition
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, but maybe you want to randomly change query_plan_optimize_primary_key
value in stress tests.
@@ -94,6 +94,9 @@ class QueryPipelineBuilder | |||
/// Changes the number of output ports if needed. Adds ResizeTransform. | |||
void resize(size_t num_streams, bool force = false, bool strict = false); | |||
|
|||
/// Concat some ports to have no more then size outputs. | |||
void narrow(size_t size); |
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.
This part is unclear.
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.
Yes, I will add comment
return res; | ||
} | ||
|
||
bool getConstant(const Block & block_with_constants, Field & out_value, DataTypePtr & out_type) const |
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.
bool getConstant(const Block & block_with_constants, Field & out_value, DataTypePtr & out_type) const | |
bool tryGetConstant(const Block & block_with_constants, Field & out_value, DataTypePtr & out_type) const |
case (ActionsDAG::ActionType::ARRAY_JOIN): | ||
{ | ||
const auto & arg = cloneASTWithInversionPushDown(*node.children.front(), inverted_dag, to_inverted, context, false); | ||
res = &inverted_dag.addArrayJoin(arg, ""); |
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.
why ""
?
@@ -5885,6 +5902,8 @@ std::optional<ProjectionCandidate> MergeTreeData::getQueryProcessingStageWithAgg | |||
selected_candidate->aggregate_descriptions = select.getQueryAnalyzer()->aggregates(); | |||
} | |||
|
|||
/// Just in case, reset prewhere info calculated from projection. | |||
query_info.prewhere_info.reset(); |
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.
This part is very tricky
auto current_info = query_info.order_optimizer->getInputOrder(storage_metadata_snapshot, modified_context); | ||
if (it == selected_tables.begin()) | ||
input_sorting_info = current_info; | ||
else if (!current_info || (input_sorting_info && *current_info != *input_sorting_info)) |
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.
Maybe we can remember a common prefix?
@novikd I want to have |
it introduced a bug #40599 |
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Change the way how PK is analyzed for MergeTree.