Jackie-Jiang commented on a change in pull request #6991:
URL: https://github.com/apache/incubator-pinot/pull/6991#discussion_r640240892
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/IntermediateResultsBlock.java
##########
@@ -117,6 +119,19 @@ public IntermediateResultsBlock(AggregationFunction[]
aggregationFunctions,
_dataSchema = dataSchema;
}
+ /**
+ * Constructor for aggregation group-by order-by result with {@link
AggregationGroupByResult}.
+ */
+ public IntermediateResultsBlock(AggregationFunction[] aggregationFunctions,
+ @Nullable AggregationGroupByResult
aggregationGroupByResults,
Review comment:
Follow
https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup
to setup the code style in your IDE
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -61,7 +61,11 @@
public static final String MAX_INITIAL_RESULT_HOLDER_CAPACITY_KEY =
"max.init.group.holder.capacity";
public static final int DEFAULT_MAX_INITIAL_RESULT_HOLDER_CAPACITY = 10_000;
public static final String NUM_GROUPS_LIMIT = "num.groups.limit";
+ public static final String SEGMENT_TRIM_SIZE = "num.segment.trim.limit";
+ public static final String SEGMENT_TRIM_OPT = "bool.segment.trim";
Review comment:
```suggestion
public static final String ENABLE_SEGMENT_GROUP_TRIM =
"enable.segment.group.trim";
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java
##########
@@ -144,4 +147,22 @@ protected void aggregate(TransformBlock transformBlock,
int length, int function
public AggregationGroupByResult getResult() {
return new AggregationGroupByResult(_groupKeyGenerator,
_aggregationFunctions, _groupByResultHolders);
}
+
+ @Override
+ public Collection<TableResizer.fullIntermediateResult> trimGroupByResult(int
threshold, TableResizer tableResizer) {
+ // Check if a trim is necessary
+ int keyNum = 0;
+ if (_hasMVGroupByExpression) {
+ keyNum = _mvGroupKeys.length;
+ } else {
+ keyNum = _svGroupKeys.length;
+ }
+ Iterator<GroupKeyGenerator.GroupKey> groupKeyIterator =
_groupKeyGenerator.getGroupKeys();
+ if (keyNum > threshold && threshold != -1) {
+ return tableResizer.trimInSegmentResults(groupKeyIterator,
_groupByResultHolders, threshold);
Review comment:
threshold should be calculated based on the `limit` instead of hardcoded
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java
##########
@@ -357,4 +372,88 @@ public Comparable extract(Record record) {
}
}
}
+
+ public static class fullIntermediateResult extends IntermediateRecord {
Review comment:
You can directly change the `IntermediateRecord` to include the record
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -61,7 +61,11 @@
public static final String MAX_INITIAL_RESULT_HOLDER_CAPACITY_KEY =
"max.init.group.holder.capacity";
public static final int DEFAULT_MAX_INITIAL_RESULT_HOLDER_CAPACITY = 10_000;
public static final String NUM_GROUPS_LIMIT = "num.groups.limit";
+ public static final String SEGMENT_TRIM_SIZE = "num.segment.trim.limit";
Review comment:
Let's not make this configurable in step 1. Use `max(limit * 5, 5000)`
(same as inter segment trim). You can use `GroupByUtils` to get the trim size
from the `limit`
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java
##########
@@ -357,4 +372,88 @@ public Comparable extract(Record record) {
}
}
}
+
+ public static class fullIntermediateResult extends IntermediateRecord {
Review comment:
Class name should be capitalized
--
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]
