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

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

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



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

Review comment:
       I am wondering if this function should be renamed to such gapfill since the "pre aggregate" part is now clearly visible with the subquery structure?

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -130,8 +130,25 @@ public static PinotQuery compileToPinotQuery(String sql)
     if (!options.isEmpty()) {
       sql = removeOptionsFromSql(sql);
     }
+
+    SqlParser sqlParser = SqlParser.create(sql, PARSER_CONFIG);
+    SqlNode sqlNode;
+    try {
+      sqlNode = sqlParser.parseQuery();
+    } catch (SqlParseException e) {
+      throw new SqlCompilationException("Caught exception while parsing query: " + sql, e);
+    }
+
     // Compile Sql without OPTION statements.
-    PinotQuery pinotQuery = compileCalciteSqlToPinotQuery(sql);
+    PinotQuery pinotQuery = compileSqlNodeToPinotQuery(sqlNode);
+
+    SqlSelect sqlSelect = getSelectNode(sqlNode);
+    if (sqlSelect != null) {
+      SqlNode fromNode = sqlSelect.getFrom();
+      if (fromNode != null && (fromNode instanceof SqlSelect || fromNode instanceof SqlOrderBy)) {
+        pinotQuery.getDataSource().setSubquery(compileSqlNodeToPinotQuery(fromNode));
+      }
+    }

Review comment:
       Is all this change to original code necessary? For example the original function `compileCalciteSqlToPinotQuery` could still be retained, but modified to set the subquery node in pinotQuery.

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -321,32 +338,30 @@ private static void setOptions(PinotQuery pinotQuery, List<String> optionsStatem
     pinotQuery.setQueryOptions(options);
   }
 
-  private static PinotQuery compileCalciteSqlToPinotQuery(String sql) {
-    SqlParser sqlParser = SqlParser.create(sql, PARSER_CONFIG);
-    SqlNode sqlNode;
-    try {
-      sqlNode = sqlParser.parseQuery();
-    } catch (SqlParseException e) {
-      throw new SqlCompilationException("Caught exception while parsing query: " + sql, e);
-    }
-
-    PinotQuery pinotQuery = new PinotQuery();
-    if (sqlNode instanceof SqlExplain) {
-      // Extract sql node for the query
-      sqlNode = ((SqlExplain) sqlNode).getExplicandum();
-      pinotQuery.setExplain(true);
-    }

Review comment:
       We need this for EXPLAIN queries to work.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/CombinePlanNode.java
##########
@@ -180,6 +181,8 @@ public BaseCombineOperator run() {
         // Selection order-by
         return new SelectionOrderByCombineOperator(operators, _queryContext, _executorService);
       }
+    } else if (GapfillUtils.isPreAggregateGapfill(_queryContext)) {
+        return new SelectionOnlyCombineOperator(operators, _queryContext, _executorService);

Review comment:
       Will this still work if an ORDER BY clause is present in the gap fill query? If not then perhaps some validation checks should be done to filter out unsupported usage of gapfill queries (ordery by, having, group by etc.)




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