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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
