Jackie-Jiang commented on a change in pull request #6559:
URL: https://github.com/apache/incubator-pinot/pull/6559#discussion_r573381665
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java
##########
@@ -108,35 +116,35 @@ public DictionaryBasedGroupKeyGenerator(TransformOperator
transformOperator, Exp
_isSingleValueColumn[i] =
transformOperator.getResultMetadata(groupByExpression).isSingleValue();
}
+ // TODO: Clear the holder after processing the query instead of before
if (longOverflow) {
+ // ArrayMapBasedHolder
_globalGroupIdUpperBound = numGroupsLimit;
- Object mapInternal =
mapBasedRawKeyHolders.computeIfAbsent(ArrayMapBasedHolder.class.getName(),
- o -> new ArrayMapBasedHolder(INITIAL_MAP_SIZE).getInternal());
- _rawKeyHolder = new ArrayMapBasedHolder(mapInternal);
- if (((Object2IntOpenHashMap) mapInternal).size() > MAX_CACHING_MAP_SIZE)
{
- mapBasedRawKeyHolders
- .put(ArrayMapBasedHolder.class.getName(), new
ArrayMapBasedHolder(INITIAL_MAP_SIZE).getInternal());
+ Object2IntOpenHashMap<IntArray> groupIdMap =
THREAD_LOCAL_INT_ARRAY_MAP.get();
+ int size = groupIdMap.size();
+ groupIdMap.clear();
+ if (size > MAX_CACHING_MAP_SIZE) {
+ groupIdMap.trim();
}
+ _rawKeyHolder = new ArrayMapBasedHolder(groupIdMap);
} else {
if (cardinalityProduct > Integer.MAX_VALUE) {
+ // LongMapBasedHolder
_globalGroupIdUpperBound = numGroupsLimit;
- Object mapInternal =
mapBasedRawKeyHolders.computeIfAbsent(LongMapBasedHolder.class.getName(),
- o -> new LongMapBasedHolder(INITIAL_MAP_SIZE).getInternal());
- _rawKeyHolder = new LongMapBasedHolder(mapInternal);
- if (((Long2IntOpenHashMap) mapInternal).size() > MAX_CACHING_MAP_SIZE)
{
- mapBasedRawKeyHolders
- .put(ArrayMapBasedHolder.class.getName(), new
ArrayMapBasedHolder(INITIAL_MAP_SIZE).getInternal());
+ Long2IntOpenHashMap groupIdMap = THREAD_LOCAL_LONG_MAP.get();
+ int size = groupIdMap.size();
+ groupIdMap.clear();
+ if (size > MAX_CACHING_MAP_SIZE) {
+ groupIdMap.trim();
}
+ _rawKeyHolder = new LongMapBasedHolder(groupIdMap);
} else {
_globalGroupIdUpperBound = Math.min((int) cardinalityProduct,
numGroupsLimit);
if (cardinalityProduct > arrayBasedThreshold) {
- Object mapInternal =
mapBasedRawKeyHolders.computeIfAbsent(IntMapBasedHolder.class.getName(),
- o -> new IntMapBasedHolder(INITIAL_MAP_SIZE).getInternal());
- _rawKeyHolder = new IntMapBasedHolder(mapInternal);
- if (((Int2IntOpenHashMap) mapInternal).size() >
MAX_CACHING_MAP_SIZE) {
- mapBasedRawKeyHolders
- .put(ArrayMapBasedHolder.class.getName(), new
ArrayMapBasedHolder(INITIAL_MAP_SIZE).getInternal());
- }
+ // IntMapBasedHolder
+ IntGroupIdMap groupIdMap = THREAD_LOCAL_INT_MAP.get();
+ groupIdMap.clear();
Review comment:
`trim()` reduces the capacity of the map, but does not clear the map.
The `clear()` here handles the logic of clear the map, then check the map
capacity and trim it if it is over the caching threshold (as described in the
javadoc). We don't want to split this into 2 APIs because that will be more
costly. If we need to reduce the capacity, we can directly allocate a new array
instead of first clearing the old array.
----------------------------------------------------------------
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
