gianm commented on a change in pull request #7496: refactor druid-bloom-filter aggregators URL: https://github.com/apache/incubator-druid/pull/7496#discussion_r276715440
########## File path: extensions-core/druid-bloom-filter/src/main/java/org/apache/druid/query/aggregation/bloom/DoubleBloomFilterAggregator.java ########## @@ -23,20 +23,22 @@ import org.apache.druid.query.filter.BloomKFilter; import org.apache.druid.segment.BaseDoubleColumnValueSelector; +import java.nio.ByteBuffer; + public final class DoubleBloomFilterAggregator extends BaseBloomFilterAggregator<BaseDoubleColumnValueSelector> Review comment: Oh. Well, I think the way the CardinalityAggregator does it is cleaner, since it only splits out the logic for how to read from differently-typed inputs, and then composes that logic into a single Aggregator class, instead of using inheritance and a ton of Aggregator subclasses. I just generally find inheritance based structures less easy to follow since the logic seems 'inside out' to me. But, I guess if reasonable people could differ on this point, it's up to you how you want to structure it. I don't think this inheritance based approach is _better_ than the type-based-input-strategy approach, but it's acceptable. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
