search

apache / druid

Apache Druid: a high performance real-time analytics database.
https://druid.apache.org/
Apache License 2.0
13.53k stars 3.71k forks source link

Remove CardinalityAggregatorColumnSelectorStrategy and CardinalityAggregatorColumnSelectorStrategyFactory as too shallow abstractions. #6909

Open leventov opened 5 years ago

leventov commented 5 years ago

The logic of CardinalityAggregatorColumnSelectorStrategyFactory could be inlined in CardinalityAggregatorFactory.factorize() and factorizeBuffered() methods, and the logic of CardinalityAggregatorColumnSelectorStrategy could be expressed in subclasses of CardinalityBufferAggregator and CardinalityAggregator.

This change is similar to what is proposed here: https://github.com/apache/incubator-druid/pull/6397#discussion_r250564034

hpandeycodeit commented 5 years ago

@leventov I will work on this one.

gianm commented 5 years ago

I accidentally stumbled into this by suggesting the opposite refactoring on #7496: https://github.com/apache/incubator-druid/pull/7496#discussion_r276413326. I think the way that CardinalityAggregator works now is pretty nice, for the reasons I mentioned in that thread. I'm not really interested in requiring that everything be done that way but I did want to speak up for why the design is the way it is.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue is no longer marked as stale.

github-actions[bot] commented 1 year ago

This issue has been marked as stale due to 280 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.