amrishlal commented on a change in pull request #8029:
URL: https://github.com/apache/pinot/pull/8029#discussion_r819826352



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/GapfillUtils.java
##########
@@ -31,7 +36,10 @@
  */
 public class GapfillUtils {
   private static final String POST_AGGREGATE_GAP_FILL = "postaggregategapfill";

Review comment:
       ok

##########
File path: 
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/BrokerRequestToQueryContextConverter.java
##########
@@ -53,12 +55,81 @@ private BrokerRequestToQueryContextConverter() {
    * Converts the given {@link BrokerRequest} into a {@link QueryContext}.
    */
   public static QueryContext convert(BrokerRequest brokerRequest) {
-    return brokerRequest.getPinotQuery() != null ? convertSQL(brokerRequest) : 
convertPQL(brokerRequest);
+    if (brokerRequest.getPinotQuery() != null) {
+      QueryContext queryContext = convertSQL(brokerRequest.getPinotQuery(), 
brokerRequest);
+      queryContext.setGapfillType(GapfillUtils.getGapfillType(queryContext));
+      validateForGapfillQuery(queryContext);

Review comment:
       From what I can see these syntax checks are happening on the Server 
side. Server receives an `InstanceRequest` from Broker in 
`InstanceRequestHandler.channelRead0` method and invokes the 
`ServerQueryRequest` constructor to create `ServerQueryRequest` object. The 
`ServerQueryRequest` constructor then calls makes the call:
   
       `_queryContext = 
BrokerRequestToQueryContextConverter.convert(brokerRequest);`
   
   We should move the syntax checks as far up the stack as possible and reject 
bad queries as early as possible so that unnecessary processing can be avoided.

##########
File path: 
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -133,6 +137,8 @@ private QueryContext(String tableName, 
List<ExpressionContext> selectExpressions
     _queryOptions = queryOptions;
     _debugOptions = debugOptions;
     _brokerRequest = brokerRequest;
+    _gapfillType = null;

Review comment:
       Let's leave it as null as Jackie suggested.




-- 
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]

Reply via email to