amrishlal commented on a change in pull request #6811:
URL: https://github.com/apache/incubator-pinot/pull/6811#discussion_r619889565
##########
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:
Ordering doesn't seem to be very important here with respect to
performance since we are working on small constant size operator trees. Seems
to me that it would be better to keep these rewrite classes order independent,
so that new rewrites can be easily plugged in and obsolete once can be deleted
without worrying out ordering dependencies. But let me know if you still prefer
moving it. The only thing that may cause rewrite issues would probably be IN
clause with large number of values and in that case it would make sense to
traverse IN clause only once.
##########
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:
Reworked this a bit by using TRUE/FALSE Boolean literal constants.
##########
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:
Fixed.
##########
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 had made this change to make `CalciteSqlCompilerTest` pass, but it
seems like those test cases also pass by using getIntValue() instead of
getLongValue() so reverted this change. But I am not seeing much of a benefit
in overloading long to represent both int and long. This can be confusing
specially since int and long are different and can also cause correctness
issues.
--
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]
