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



##########
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:
       Instead of setting `_gapfillType` to null, can we set it to `GAP_FILL_NONE` as that will clearly show that no gap filling will happen by default?

##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/GapfillQueriesTest.java
##########
@@ -0,0 +1,3615 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.queries;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.common.response.broker.ResultTable;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
+import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
+import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.DateTimeGranularitySpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.utils.ReadMode;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * Queries test for Gapfill queries.
+ */
+// TODO: Item 1. table alias for subquery in next PR
+// TODO: Item 2. Deprecate PostAggregateGapfill implementation in next PR
+@SuppressWarnings("rawtypes")
+public class GapfillQueriesTest extends BaseQueriesTest {
+  private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "PostAggregationGapfillQueriesTest");

Review comment:
       Seems like `PostAggregationGapfillQueriesTest` should be replaced with `GapfillQueriesTest`

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -369,6 +369,9 @@ private static PinotQuery compileCalciteSqlToPinotQuery(String sql) {
       DataSource dataSource = new DataSource();
       dataSource.setTableName(fromNode.toString());
       pinotQuery.setDataSource(dataSource);
+      if (fromNode instanceof SqlSelect || fromNode instanceof SqlOrderBy) {

Review comment:
       Not sure if I understand why `fromNode` should be an instance of SqlOrderBy? I don't think we can have an SQL statement that has an order by clause within from clause ( `SELECT x FROM ORDER BY x`)

##########
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));

Review comment:
       Seems like this should be done using the `QueryContext.Builder` pattern inside of `convertSQL` function?

##########
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:
       It seems like this validation is just checking syntax, can it be done on the Broker? If I am not mistaken almost all of Pinot's syntax checks happen on the Broker side.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/GapfillUtils.java
##########
@@ -119,4 +130,138 @@ static public Serializable getDefaultValue(DataSchema.ColumnDataType dataType) {
   private static String canonicalizeFunctionName(String functionName) {
     return StringUtils.remove(functionName, '_').toLowerCase();
   }
+
+  public static boolean isGapfill(ExpressionContext expressionContext) {
+    if (expressionContext.getType() != ExpressionContext.Type.FUNCTION) {
+      return false;
+    }
+
+    return GAP_FILL.equals(canonicalizeFunctionName(expressionContext.getFunction().getFunctionName()));
+  }
+
+  private static boolean isGapfill(QueryContext queryContext) {
+    for (ExpressionContext expressionContext : queryContext.getSelectExpressions()) {
+      if (isGapfill(expressionContext)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  public static GapfillType getGapfillType(QueryContext queryContext) {
+    if (queryContext.getSubQueryContext() == null) {
+      if (isGapfill(queryContext)) {
+        Preconditions.checkArgument(queryContext.getAggregationFunctions() == null,
+            "Aggregation and Gapfill can not be in the same sql statement.");

Review comment:
       Would it be possible to do these syntax checks all together in one place on the broker side when the initial query is received? That way bad queries can quickly be filtered out and we don't have to do syntax checks deep inside code logic.

##########
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:
       Is this a left over from previous implementation? I thought `postaggregategapfill` function was no longer being used? I don't see any of the test cases using postaggregategapfill function?




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