Jackie-Jiang commented on a change in pull request #7052:
URL: https://github.com/apache/incubator-pinot/pull/7052#discussion_r651297625
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java
##########
@@ -213,6 +213,8 @@
public static final String RESPONSE_FORMAT = "responseFormat";
public static final String GROUP_BY_MODE = "groupByMode";
public static final String SKIP_UPSERT = "skipUpsert";
+ public static final String SEGMENT_ENABLE_TRIM = "segmentEnableTrim";
+ public static final String SEGMENT_MIN_TRIM_SIZE =
"segmentMinTrimSize";
Review comment:
```suggestion
public static final String ENABLE_SEGMENT_TRIM = "enableSegmentTrim";
public static final String MIN_SEGMENT_TRIM_SIZE =
"minSegmentTrimSize";
```
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/QueryOptions.java
##########
@@ -33,6 +33,8 @@
private final boolean _responseFormatSQL;
private final boolean _preserveType;
private final boolean _skipUpsert;
+ private final Boolean _enableSegmentTrim;
+ private final Integer _segmentTrimSize;
Review comment:
No need to use object
```suggestion
private final boolean _enableSegmentTrim;
private final int _minSegmentTrimSize;
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java
##########
@@ -145,4 +148,19 @@ public ExecutionStatistics getExecutionStatistics() {
return new ExecutionStatistics(_numDocsScanned, numEntriesScannedInFilter,
numEntriesScannedPostFilter,
_numTotalDocs);
}
+
+ private int calculateMinTrimSize () {
Review comment:
(nit) `calculateMinSegmentTrimSize`
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/QueryOptions.java
##########
@@ -82,4 +98,24 @@ public static Long getTimeoutMs(Map<String, String>
queryOptions) {
return null;
}
}
+
+ @Nullable
+ public static Boolean getEnableSegmentTrim(Map<String, String> queryOptions)
{
+ String enableSegmentTrim =
queryOptions.get(Request.QueryOptionKey.SEGMENT_ENABLE_TRIM);
+ if (enableSegmentTrim != null) {
+ return Boolean.parseBoolean(enableSegmentTrim);
+ } else {
+ return null;
+ }
+ }
Review comment:
```suggestion
public static boolean getEnableSegmentTrim(Map<String, String>
queryOptions) {
return
Boolean.parseBoolean(queryOptions.get(Request.QueryOptionKey.SEGMENT_ENABLE_TRIM));
}
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java
##########
@@ -145,4 +148,19 @@ public ExecutionStatistics getExecutionStatistics() {
return new ExecutionStatistics(_numDocsScanned, numEntriesScannedInFilter,
numEntriesScannedPostFilter,
_numTotalDocs);
}
+
+ private int calculateMinTrimSize () {
+ // In query option, if a positive min trim size is given, we use it to
override the server settings. Otherwise
Review comment:
Move the comment as javadoc for the method
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java
##########
@@ -145,4 +148,19 @@ public ExecutionStatistics getExecutionStatistics() {
return new ExecutionStatistics(_numDocsScanned, numEntriesScannedInFilter,
numEntriesScannedPostFilter,
_numTotalDocs);
}
+
+ private int calculateMinTrimSize () {
+ // In query option, if a positive min trim size is given, we use it to
override the server settings. Otherwise
+ // check if a simple boolean option is given and use default trim size.
+ QueryOptions queryOptions = new
QueryOptions(_queryContext.getQueryOptions());
+ Boolean queryOptionEnableTrim = queryOptions.isEnableSegmentTrim();
+ Integer queryOptionTrimSize = queryOptions.getMinSegmentTrimSize();
+ if (queryOptionTrimSize != null && queryOptionTrimSize > 0) {
+ return queryOptionTrimSize;
+ } else if (queryOptionEnableTrim != null && queryOptionEnableTrim) {
+ // TODO: if query option is true and a server minTrimSize is given,
shall we use default value or server config?
Review comment:
Here if `_minSegmentTrimSize` is configured (> 0), we should use the
server config. If not, use the default
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java
##########
@@ -145,4 +148,19 @@ public ExecutionStatistics getExecutionStatistics() {
return new ExecutionStatistics(_numDocsScanned, numEntriesScannedInFilter,
numEntriesScannedPostFilter,
_numTotalDocs);
}
+
+ private int calculateMinTrimSize () {
+ // In query option, if a positive min trim size is given, we use it to
override the server settings. Otherwise
+ // check if a simple boolean option is given and use default trim size.
+ QueryOptions queryOptions = new
QueryOptions(_queryContext.getQueryOptions());
Review comment:
Use the static method instead of the constructor to avoid the overhead
of parsing unrelated fields
--
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
