Reset column argument for runningAccumulate#8326
Reset column argument for runningAccumulate#8326alexey-milovidov merged 5 commits intoClickHouse:masterfrom
Conversation
| column_with_groups->compareAt(i, i - 1, *column_with_groups, 1) != 0) | ||
| { | ||
| agg_func.destroy(place.data()); | ||
| agg_func.create(place.data()); |
There was a problem hiding this comment.
Need something for exception safety if create will fail.
For example, you can add a bool flag and check it in SCOPE_EXIT above.
There was a problem hiding this comment.
There is SCOPE_EXIT with agg_func.destroy callback installed at line 93. And if I understood everything correctly
SCOPE_EXITuses destructor and it will be destroyed in case of exception too;agg_func.destroy(as I saw in other places) must be called, whenever creation failure occurred.
Is it really need to handle agg_func.create calling exceptions?
There was a problem hiding this comment.
Yes, create may fail due to memory limits. In that case, destroy will be called twice. And it may lead to double free.
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
Except this minor issue, the code looks perfect!
|
|
|
I’ll fix it on weekend! |
Now arguments count are tested before invocation. |
|
|
||
| try | ||
| { | ||
| agg_func.create(place.data()); |
There was a problem hiding this comment.
No... if create has thrown an exception, the data is not created and calling "destroy" is incorrect.
There was a problem hiding this comment.
I take an example from FunctionArrayReduce.
You mean that if create fails - there's no reason to destroy?
There was a problem hiding this comment.
Fixed solution below
There was a problem hiding this comment.
I take an example from FunctionArrayReduce.
That's strange. I think it's bug - will validate.
There was a problem hiding this comment.
That’s all for this PR?
|
Almost. Because all tests have failed and server has crashed :) |
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
Will be merged with #8450
…ingAccumulate Merging #8326
|
Docs need to be updated. And probably the same should be done for RunningDifference and neighbor |
4 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):
runningAccumulatefunction that reset aggregation result for different expressions. This fixes runningAccumulate function with reset key #6528