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
Optimize group by constant keys #53549
Conversation
This is an automated comment for commit a075d03 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
|
src/Interpreters/Aggregator.cpp
Outdated
executeImplBatch<false, false, false>( | ||
method, state, aggregates_pool, row_begin, row_end, aggregate_instructions, overflow_row); | ||
} | ||
BoolArgsToTemplateArgsDispatcher<decltype(call_execute_impl_batch)>::call(call_execute_impl_batch, no_more_keys, use_compiled_functions, prefetch, all_keys_are_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.
Why is it better? I didn't understand why we need this class.
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.
Otherwise we will need to write ~16 if/else branches to check all 4 bool arguments (maybe some of them won't actually appear, but still a lot).
Like https://pastila.nl/?01f3324d/a170ed3f747f063f81f5821188dd8aa3#B7nUAYOQzbzMvGUQ6wClTw==
And IMHO do it in a few lines with some dispatcher class is better. But you can disagree and I can bring back this batch of if/else
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 do not like dispatchers as well. I think the issue is with executeImplBatch and we should refactor it somehow.
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.
The change is good and clear generally.
However, I do not like how our dispatching is done.
Looks like something is wrong with a function that takes 4 bool template args.
I would like to refactor it somehow, but do not have good ideas right now.
Actually, the new bool flag |
I don't really mind :) explain syntax select count(), pi() from numbers_mt(1e6) group by pi(), e()
EXPLAIN SYNTAX
SELECT
count(),
pi()
FROM numbers_mt(1000000.)
GROUP BY
pi(),
e()
┌─explain───────────────────┐
│ SELECT │
│ count(), │
│ pi() │
│ FROM numbers_mt(1000000.) │
└───────────────────────────┘ |
But afaiu that's not the same and we cannot reuse it. We can still have different keys, but some blocks of data can contain constant columns with keys. For example:
Reading from |
ok, I indeed assumed that we're talking about globally constant keys. |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Optimize group by constant keys. Will optimize queries with group by
_file/_path
after #53529Documentation entry for user-facing changes