[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8029: Add Pre-Aggregation Gapfilling functionality.

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #8029:
URL: https://github.com/apache/pinot/pull/8029#discussion_r787054185



##########
File path: pinot-common/src/thrift/query.thrift
##########
@@ -20,6 +20,7 @@ namespace java org.apache.pinot.common.request
 
 struct DataSource {
   1: optional string tableName;
+  2: optional PinotQuery preAggregateGapfillQuery;

Review comment:
       Let's name it `subquery`. We want it to be general in the query syntax

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -85,6 +85,7 @@
   // Keep the BrokerRequest to make incremental changes
   // TODO: Remove it once the whole query engine is using the QueryContext
   private final BrokerRequest _brokerRequest;
+  private QueryContext _preAggregateGapFillQueryContext;

Review comment:
       Can we generify this to `subquery`? I think this can be modeled as general subquery

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -117,21 +117,50 @@ private static String removeTerminatingSemicolon(String sql) {
     return sql;
   }
 
+  private static SqlNode parse(String sql) {
+    SqlParser sqlParser = SqlParser.create(sql, PARSER_CONFIG);
+    try {
+      return sqlParser.parseQuery();
+    } catch (SqlParseException e) {
+      throw new SqlCompilationException("Caught exception while parsing query: " + sql, e);
+    }
+  }
+
+  public static PinotQuery compileToPinotQueryWithSubquery(String sql)
+      throws SqlCompilationException {
+    return compileToPinotQuery(sql, true);
+  }
+
   public static PinotQuery compileToPinotQuery(String sql)
       throws SqlCompilationException {
-    // Remove the comments from the query
-    sql = removeComments(sql);

Review comment:
       This should not be removed

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -117,21 +117,50 @@ private static String removeTerminatingSemicolon(String sql) {
     return sql;
   }
 
+  private static SqlNode parse(String sql) {
+    SqlParser sqlParser = SqlParser.create(sql, PARSER_CONFIG);
+    try {
+      return sqlParser.parseQuery();
+    } catch (SqlParseException e) {
+      throw new SqlCompilationException("Caught exception while parsing query: " + sql, e);
+    }
+  }
+
+  public static PinotQuery compileToPinotQueryWithSubquery(String sql)
+      throws SqlCompilationException {
+    return compileToPinotQuery(sql, true);
+  }
+
   public static PinotQuery compileToPinotQuery(String sql)
       throws SqlCompilationException {
-    // Remove the comments from the query
-    sql = removeComments(sql);
+    return compileToPinotQuery(sql, false);

Review comment:
       Let's directly extend this method to support subquery within the `FROM` clause. The subquery parsing should always be enabled regardless of the query type




-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org