samarthjain commented on code in PR #17439:
URL: https://github.com/apache/druid/pull/17439#discussion_r1841213890
##########
processing/src/main/java/org/apache/druid/query/topn/BaseTopNAlgorithm.java:
##########
@@ -41,8 +41,11 @@
public abstract class BaseTopNAlgorithm<DimValSelector, DimValAggregateStore,
Parameters extends TopNParams>
implements TopNAlgorithm<DimValSelector, Parameters>
{
- public static Aggregator[] makeAggregators(Cursor cursor,
List<AggregatorFactory> aggregatorSpecs)
+
+ public static Aggregator[] makeAggregators(TopNQuery query, Cursor cursor)
{
+ query.getAggregatorHelper().addAggregatorMemory();
Review Comment:
nit: I think it would be cleaner if there was a method named like
`trackAggregatorMemory` in TopNQuery itself. Something like this:
```
public void trackAggregatorMemory() {
aggregatorHelper.addAggregatorMemory();
}
```
##########
processing/src/main/java/org/apache/druid/query/topn/TopNQuery.java:
##########
@@ -60,6 +61,7 @@ public class TopNQuery extends
BaseQuery<Result<TopNResultValue>>
private final DimFilter dimFilter;
private final List<AggregatorFactory> aggregatorSpecs;
private final List<PostAggregator> postAggregatorSpecs;
+ private TopNAggregatorResourceHelper aggregatorHelper;
Review Comment:
I see it is getting initialized in the constructor. So it can be made final
as well, no?
##########
processing/src/main/java/org/apache/druid/query/topn/TopNQuery.java:
##########
@@ -97,9 +99,18 @@ public TopNQuery(
: postAggregatorSpecs
);
+ final long expectedAllocBytes =
aggregatorSpecs.stream().mapToLong(AggregatorFactory::getMaxIntermediateSizeWithNulls).sum();
+ final long maxAggregatorHeapSizeBytes =
this.context().getLong(QueryContexts.MAX_TOP_N_AGGREGATOR_HEAP_SIZE_BYTES,
TopNQueryConfig.DEFAULT_MAX_AGGREGATOR_HEAP_SIZE_BYTES);
+ this.aggregatorHelper = new
TopNAggregatorResourceHelper(expectedAllocBytes, new
TopNAggregatorResourceHelper.Config(maxAggregatorHeapSizeBytes));
+
topNMetricSpec.verifyPreconditions(this.aggregatorSpecs,
this.postAggregatorSpecs);
}
+ public TopNAggregatorResourceHelper getAggregatorHelper()
Review Comment:
If the code is changed like I suggested earlier, then this method can be
removed.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
