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
Remove more old code of projection analysis #55579
Remove more old code of projection analysis #55579
Conversation
This is an automated comment for commit 3e933c9 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.
I've left some minor comments about things I've seen in the changes.
The test failures seem related:
ClickHouse Stateless Tests (release, Analyzer)
- > Analyzer + projections + prewhere (02796_projection_date_filter_on_view
and01710_aggregate_projections
)ClickHouse Stateless Tests (tsan, S3 Storage)
. TSAN detected a couple of issues but they look related to CH local and not to these changes, but it should be confirmed.
/// This flag is also used for projection analysis. | ||
/// It is needed because lazy normal projections require special planning in FetchColumns stage, such as adding WHERE transform. | ||
/// It is also used to avoid adding aggregating step when aggregate projection is chosen. | ||
bool is_projection_query = false; | ||
/// This flag is needed for projection description. |
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.
Should we update the comments around ignore_ast_optimizations
and ignore_setting_constraints
related to projections?
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 think no, because we still use them in ProjectionDescription
.
Yes, they are related. I tried to fix it but unsuccessfully currently. |
Tests related to projections have been magically fixed by merging fresh master. I think it's because of #56502. So now we can merge this PR. |
There is still some failing tests with the analyzer -> https://s3.amazonaws.com/clickhouse-test-reports/55579/69896d94bead717ebc75bdf1907935e6057ae98c/stateless_tests__release__analyzer_.html. Both involving projections and PREWHERE |
e13ed63
to
cf4604b
Compare
I reverted commit a01acf5 and returned back old behaviour when we used |
@@ -2556,56 +2493,28 @@ void InterpreterSelectQuery::executeFetchColumns(QueryProcessingStage::Enum proc | |||
|
|||
/// Create optimizer with prepared actions. | |||
/// Maybe we will need to calc input_order_info later, e.g. while reading from StorageMerge. | |||
if ((optimize_read_in_order || optimize_aggregation_in_order) | |||
&& (!query_info.projection || query_info.projection->complete)) | |||
if (optimize_read_in_order || optimize_aggregation_in_order) |
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 if now would probably be easier to read if it was:
if (optimize_read_in_order)
...
else if (optimize_aggregation_in_order)
...
As both options are separate there is no need of doing (if (a || b) { if (a) else ...)
Integration tests: failed tests with keeper which are flaky now. Performance Comparison: Stateless tests (release, DatabaseReplicated): |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Addition to #55112.