Jackie-Jiang commented on a change in pull request #6811:
URL: https://github.com/apache/incubator-pinot/pull/6811#discussion_r616345882
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -428,6 +437,38 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
_brokerMetrics.addMeteredTableValue(rawTableName,
BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED, 1);
}
+ logBrokerResponse(requestStatistics, requestId, query,
compilationStartTimeNs, brokerRequest,
+ numUnavailableSegments, serverStats, brokerResponse,
executionEndTimeNs);
+ return brokerResponse;
+ }
+
+ /**
+ * Given a {@link BrokerRequest}, this function will determine if we can
return a response without server-side query
+ * evaluation. This usually happens when the optimizer determines that the
entire WHERE clause evaluates to false.
+ */
+ private boolean isResponsePossible(BrokerRequest brokerRequest) {
+ if (brokerRequest == null) { return true;}
Review comment:
We should handle `null` before calling this method
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java
##########
@@ -31,12 +31,14 @@
import
org.apache.pinot.core.query.optimizer.filter.FlattenAndOrFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.MergeEqInFilterOptimizer;
import org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer;
+import org.apache.pinot.core.query.optimizer.filter.NumericalFilterOptimizer;
import org.apache.pinot.spi.data.Schema;
public class QueryOptimizer {
private static final List<FilterOptimizer> FILTER_OPTIMIZERS = Arrays
- .asList(new FlattenAndOrFilterOptimizer(), new
MergeEqInFilterOptimizer(), new MergeRangeFilterOptimizer());
+ .asList(new FlattenAndOrFilterOptimizer(), new
MergeEqInFilterOptimizer(), new MergeRangeFilterOptimizer(),
+ new NumericalFilterOptimizer());
Review comment:
`NumericalFilterOptimizer` should be inserted before the
`MergeEqInFilterOptimizer` so that `MergeEqInFilterOptimizer` and
`MergeRangeFilterOptimizer` can read the optimized values
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -428,6 +437,38 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
_brokerMetrics.addMeteredTableValue(rawTableName,
BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED, 1);
}
+ logBrokerResponse(requestStatistics, requestId, query,
compilationStartTimeNs, brokerRequest,
+ numUnavailableSegments, serverStats, brokerResponse,
executionEndTimeNs);
+ return brokerResponse;
+ }
+
+ /**
+ * Given a {@link BrokerRequest}, this function will determine if we can
return a response without server-side query
+ * evaluation. This usually happens when the optimizer determines that the
entire WHERE clause evaluates to false.
+ */
+ private boolean isResponsePossible(BrokerRequest brokerRequest) {
+ if (brokerRequest == null) { return true;}
+
+ Expression filterExpression =
brokerRequest.getPinotQuery().getFilterExpression();
+ if (filterExpression != null &&
filterExpression.getType().equals(ExpressionType.LITERAL) && filterExpression
Review comment:
We can add constants for literal `true` and `false` expression, and
check if the filter expression equals to them for better readability
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java
##########
@@ -151,7 +157,10 @@ public static Expression getLiteralExpression(byte[]
value) {
}
public static Expression getLiteralExpression(Object object) {
- if (object instanceof Integer || object instanceof Long) {
+ if (object instanceof Integer) {
Review comment:
I feel it is not needed to introduce int literal here. `long`, `double`,
`string` should be enough to represent all literals
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -428,6 +437,38 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
_brokerMetrics.addMeteredTableValue(rawTableName,
BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED, 1);
}
+ logBrokerResponse(requestStatistics, requestId, query,
compilationStartTimeNs, brokerRequest,
+ numUnavailableSegments, serverStats, brokerResponse,
executionEndTimeNs);
+ return brokerResponse;
+ }
+
+ /**
+ * Given a {@link BrokerRequest}, this function will determine if we can
return a response without server-side query
+ * evaluation. This usually happens when the optimizer determines that the
entire WHERE clause evaluates to false.
+ */
+ private boolean isResponsePossible(BrokerRequest brokerRequest) {
+ if (brokerRequest == null) { return true;}
+
+ Expression filterExpression =
brokerRequest.getPinotQuery().getFilterExpression();
Review comment:
This will throw NPE for PQL query (`PinotQuery` will be `null`)
--
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]
