Conversation
| class FunctionGroupingBase : public IFunction | ||
| { | ||
| protected: | ||
| static constexpr UInt64 ONE = 1; |
There was a problem hiding this comment.
I compute expressions like 1 << a several times and to avoid overflow I'd need to cast 1 literal to UInt64. It's simpler and more readable to introduce constexpr value.
| return FunctionGroupingBase::executeImpl(arguments, input_rows_count, | ||
| [this](UInt64 set_index, UInt64 arg_index) | ||
| { | ||
| return arg_index < aggregation_keys_number - set_index; |
There was a problem hiding this comment.
I've spend more than 5 min trying to understand what is happening in this row.
Can you add a comment?
There was a problem hiding this comment.
Also I hope this will be inlined ...
There was a problem hiding this comment.
I added comments for ROLLUP and CUBE, now it should be much easier to understand.
Here I use static polymorphism and compiler tends to inline this code. The same we have when use std::sort with comparator.
There was a problem hiding this comment.
Ok, now it's better - for ~5min I have managed to generally understand what's going on :)
| 1 5 | ||
| 1 6 | ||
| 2 5 | ||
| 2 6 | ||
| 3 5 | ||
| 3 6 | ||
| 4 5 | ||
| 4 6 | ||
| 5 5 | ||
| 5 6 | ||
| 6 5 | ||
| 6 6 | ||
| 7 5 | ||
| 7 6 | ||
| 8 5 | ||
| 8 6 | ||
| 9 5 | ||
| 9 6 |
There was a problem hiding this comment.
Is it correct? I would not except that grouping(number, number % 2) would return 5 or 6
-- { echoOn }
may make tests more readable
There was a problem hiding this comment.
In this test we have grouping(number, number % 2) + 3
KochetovNicolai
left a comment
KochetovNicolai
left a comment
There was a problem hiding this comment.
Generally looks ok.
Could you add more comments?
|
|
||
| ActionsVisitor::Data visitor_data( | ||
| context, SizeLimits{}, 1, {}, std::move(actions), prepared_sets, subqueries_for_sets, true, true, true, false); | ||
| context, SizeLimits{}, 1, source_columns, std::move(actions), prepared_sets, subqueries_for_sets, true, true, true, false, |
There was a problem hiding this comment.
Here we for more than a year had a potential undefined behavior. ActionsVisitor::Data stores const reference to source_columns but temporary object was provided to the constructor.
I explicitly restricted passing of temporary objects to ActionsVisitor::Data constructor.
3 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add GROUPING function. Closes #19426