Move-away "uniqCombined" as a separate aggregated function #3406
Conversation
| AggregateFunctionPtr createAggregateFunctionUniqCombined( | ||
| const std::string & name, const DataTypes & argument_types, const Array & params) | ||
| { | ||
| UInt8 precision = 17; /// default value - must correlate with default ctor of |AggregateFunctionUniqCombinedData| |
There was a problem hiding this comment.
If it MUST be correlated it's better to set named constant and use it both here and in ctor.
There was a problem hiding this comment.
It correlates with a hardcoded union member initialization, the constant won't help - so I left the comment. The code now looks like: AggregateFunctionUniqCombinedDataWithKey() : set_17() {} - Also I can use a macro-definition as constant, if it's better.
There was a problem hiding this comment.
My point is there should be only one place there magic number 17 set.
| SELECT Y, uniqCombined(X) FROM (SELECT number AS X, (3*X*X - 7*X + 11) % 37 AS Y FROM system.numbers LIMIT 15) GROUP BY Y; | ||
| SELECT Y, uniqCombined(X) FROM (SELECT number AS X, (3*X*X - 7*X + 11) % 37 AS Y FROM system.numbers LIMIT 3000) GROUP BY Y; | ||
| SELECT Y, uniqCombined(X) FROM (SELECT number AS X, (3*X*X - 7*X + 11) % 37 AS Y FROM system.numbers LIMIT 1000000) GROUP BY Y; | ||
| SELECT Y, uniqCombined(15)(X) FROM (SELECT number AS X, (3*X*X - 7*X + 11) % 37 AS Y FROM system.numbers LIMIT 15) GROUP BY Y; |
There was a problem hiding this comment.
Would it change default behavior? What if someone already use uniqCombined without parameter. Would he (or she :) ) get the same values after update?
Also update stateless tests.
…to 3958/many_templates
|
https://gist.github.com/alexey-milovidov/9cbbc2916f8d946594049d424487e458 Now the |
Accepts single param - the HLL precision.