CLICKHOUSE-3723 Multiply aggregate states. Fix and optimize #2527. #3034
Conversation
CurtizJ
changed the title
| if (m % 2) | ||
| { | ||
| for (size_t i = 0; i < input_rows_count; ++i) | ||
| function->merge(vec_to[i], vec_from[i], arena.get()); |
There was a problem hiding this comment.
arena will get out of scope, because it is not attached to ColumnAggregateFunction.
Need to add a test for functions that really use arena during merge.
| else | ||
| { | ||
| for (size_t i = 0; i < input_rows_count; ++i) | ||
| function->merge(vec_from[i], vec_from[i], arena.get()); |
There was a problem hiding this comment.
Are we modifying source column? It's not permitted.
(It should not even be able to compile, but actually you get AggregateDataPtr instead of ConstAggregateDataPtr from const ColumnAggregateFunction.)
There was a problem hiding this comment.
Need to add a test that contain both source and multiplied states.
| SELECT sumMerge(y) AS z FROM ( SELECT sumState(x) * 11 AS y FROM ( SELECT 1 AS x UNION ALL SELECT 2 AS x)); | ||
| SELECT countMerge(x) AS y FROM ( SELECT 2 * countState() AS x FROM ( SELECT 1 )); | ||
| SELECT countMerge(x) AS y FROM ( SELECT 0 * countState() AS x FROM ( SELECT 1 UNION ALL SELECT 2)); | ||
| SELECT sumMerge(y) AS z FROM ( SELECT 3 * sumState(x) * 2 AS y FROM ( SELECT 1 AS x UNION ALL SELECT 2 AS x)); |
There was a problem hiding this comment.
Please also add tests for groupArray, uniq, groupUniqArray and something very unusual up to your choice.
There was a problem hiding this comment.
You forgot about something unusual.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en