Jackie-Jiang commented on code in PR #16454:
URL: https://github.com/apache/pinot/pull/16454#discussion_r2255457770
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java:
##########
@@ -245,7 +233,17 @@ public int getNumKeys() {
return _rawKeyHolder.getNumKeys();
}
- private interface RawKeyHolder {
+ /// clear and trim thread-local map of _rawKeyHolder
+ @Override
+ public void close() {
+ try {
+ _rawKeyHolder.close();
+ } catch (Exception e) {
Review Comment:
You can override `close()` in `RawKeyHolder` to not allow it to throw
exception
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByCombineOperator.java:
##########
@@ -143,18 +143,23 @@ protected void processSegments() {
AggregationGroupByResult aggregationGroupByResult =
resultsBlock.getAggregationGroupByResult();
if (aggregationGroupByResult != null) {
// Iterate over the group-by keys, for each key, update the
group-by result in the indexedTable
- Iterator<GroupKeyGenerator.GroupKey> dicGroupKeyIterator =
aggregationGroupByResult.getGroupKeyIterator();
- while (dicGroupKeyIterator.hasNext()) {
- GroupKeyGenerator.GroupKey groupKey = dicGroupKeyIterator.next();
- Object[] keys = groupKey._keys;
- Object[] values = Arrays.copyOf(keys, _numColumns);
- int groupId = groupKey._groupId;
- for (int i = 0; i < _numAggregationFunctions; i++) {
- values[_numGroupByExpressions + i] =
aggregationGroupByResult.getResultForGroupId(i, groupId);
+ try {
+ Iterator<GroupKeyGenerator.GroupKey> dicGroupKeyIterator =
aggregationGroupByResult.getGroupKeyIterator();
+ while (dicGroupKeyIterator.hasNext()) {
+ GroupKeyGenerator.GroupKey groupKey =
dicGroupKeyIterator.next();
+ Object[] keys = groupKey._keys;
+ Object[] values = Arrays.copyOf(keys, _numColumns);
+ int groupId = groupKey._groupId;
+ for (int i = 0; i < _numAggregationFunctions; i++) {
+ values[_numGroupByExpressions + i] =
aggregationGroupByResult.getResultForGroupId(i, groupId);
+ }
+ _indexedTable.upsert(new Key(keys), new Record(values));
+
Tracing.ThreadAccountantOps.sampleAndCheckInterruptionPeriodically(mergedKeys);
+ mergedKeys++;
}
- _indexedTable.upsert(new Key(keys), new Record(values));
-
Tracing.ThreadAccountantOps.sampleAndCheckInterruptionPeriodically(mergedKeys);
- mergedKeys++;
+ } finally {
+ // clear thread-local map used in
DictionaryBasedGroupKeyGenerator
Review Comment:
(minor) From abstraction level, this is to release the resources used by the
generator
```suggestion
// Release the resources used by the group key generator
```
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/query/FilteredGroupByOperator.java:
##########
@@ -195,6 +195,8 @@ protected GroupByResultsBlock getNextBlock() {
TableResizer tableResizer = new TableResizer(_dataSchema,
_queryContext);
Collection<IntermediateRecord> intermediateRecords =
tableResizer.trimInSegmentResults(groupKeyGenerator,
groupByResultHolders, trimSize);
+ // trim groupKeyGenerator after getting intermediateRecords
Review Comment:
(minor) Update the comment, same for other places
```suggestion
// Release the resources used by the group key generator
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java:
##########
@@ -245,7 +233,17 @@ public int getNumKeys() {
return _rawKeyHolder.getNumKeys();
}
- private interface RawKeyHolder {
+ /// clear and trim thread-local map of _rawKeyHolder
Review Comment:
(minor) Capitalize the javadoc
--
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]
