Remove LowCardinality from window function args.#31888
Remove LowCardinality from window function args.#31888alexey-milovidov merged 9 commits intomasterfrom
Conversation
robot-clickhouse
added
the
pr-bugfix
| auto columns = chunk.detachColumns(); | ||
| for (auto & column : columns) | ||
| column = std::move(column)->convertToFullColumnIfConst(); | ||
| column = recursiveRemoveLowCardinality(std::move(column)->convertToFullColumnIfConst()); |
There was a problem hiding this comment.
Could you please provide some insights?
add comment
| // Another problem with Const columns is that the aggregate functions | ||
| // can't work with them, so we have to materialize them like the | ||
| // Aggregator does. | ||
| // Likewise, aggregate functions can't work with LowCardinality, |
There was a problem hiding this comment.
Then why we didn't get exception in their factory methods?
There was a problem hiding this comment.
I was looking into this the other day (for a crazy idea of handling const columns in the aggregation functions) and saw it here:
ClickHouse/src/Interpreters/Aggregator.cpp
Line 878 in be5bb7f
Both const and low cardinality columns are converted to full. Not very efficient but it works.
There was a problem hiding this comment.
I know. But the difference is that const does not affect the data type and LowCardinality does.
So, why AggregateFunctionFactory accepted LowCardinality if aggregate functions don't support it?
There was a problem hiding this comment.
Aggregate function factory also converts all LC types to ordinary
ClickHouse/src/AggregateFunctions/AggregateFunctionFactory.cpp
Lines 70 to 73 in 7a43a87
There was a problem hiding this comment.
Ok, let's fix the checks and merge.
There was a problem hiding this comment.
Aggregate function factory also converts all LC types to ordinary
This is a tech debt after introducing LowCardinality.
Your code is doing one thing and expecting that some other unrelated code is doing the same thing.
It is very inconsistent.
It should work in the following way:
- if Aggregate Function Factory has created an aggregate function with some argument types, we should be able to pass the values of these types inside the aggregate function.
Backport #31888 to 21.11: Remove LowCardinality from window function args.
Backport #31888 to 21.12: Remove LowCardinality from window function args.
…371fc4fd525c7a8a404425c17fcee Cherry pick #31888 to 21.8: Remove LowCardinality from window function args.
…371fc4fd525c7a8a404425c17fcee Cherry pick #31888 to 21.9: Remove LowCardinality from window function args.
…0371fc4fd525c7a8a404425c17fcee Cherry pick #31888 to 21.10: Remove LowCardinality from window function args.
Backport #31888 to 21.10: Remove LowCardinality from window function args.
Backport #31888 to 21.8: Remove LowCardinality from window function args.
4 participants
Changelog category (leave one):
Changelog Entry:
Fix possible crash (or incorrect result) in case of
LowCardinalityarguments of window function. Fixes #31114