amrishlal commented on a change in pull request #8029:
URL: https://github.com/apache/pinot/pull/8029#discussion_r804423493
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextUtils.java
##########
@@ -38,7 +39,11 @@ private QueryContextUtils() {
* Returns {@code true} if the given query is a selection query, {@code
false} otherwise.
*/
public static boolean isSelectionQuery(QueryContext query) {
- return query.getAggregationFunctions() == null;
+ if (GapfillUtils.isGapfill(query)) {
Review comment:
This call appears to be expensive since it traverses the entire select
list. Can this call be made more efficient since isSelectionQuery will be
called frequently by even non gapfill queries? Maybe a flag can be set during
compile time to indicate whether this is a gapfill query?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GapfillGroupByOrderByCombineOperator.java
##########
@@ -0,0 +1,263 @@
+/**
+ * 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.core.operator.combine;
+
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.List;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.pinot.common.exception.QueryException;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.response.ProcessingException;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.data.table.ConcurrentIndexedTable;
+import org.apache.pinot.core.data.table.IndexedTable;
+import org.apache.pinot.core.data.table.IntermediateRecord;
+import org.apache.pinot.core.data.table.Key;
+import org.apache.pinot.core.data.table.Record;
+import org.apache.pinot.core.data.table.UnboundedConcurrentIndexedTable;
+import org.apache.pinot.core.operator.AcquireReleaseColumnsSegmentOperator;
+import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import
org.apache.pinot.core.query.aggregation.groupby.AggregationGroupByResult;
+import org.apache.pinot.core.query.aggregation.groupby.GroupKeyGenerator;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.util.GapfillUtils;
+import org.apache.pinot.core.util.GroupByUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Combine operator for aggregation group-by queries with SQL semantic.
+ * TODO: Use CombineOperatorUtils.getNumThreadsForQuery() to get the
parallelism of the query instead of using
+ * all threads
+ */
+@SuppressWarnings("rawtypes")
+public class GapfillGroupByOrderByCombineOperator extends BaseCombineOperator {
+ public static final int MAX_TRIM_THRESHOLD = 1_000_000_000;
+ private static final Logger LOGGER =
LoggerFactory.getLogger(GapfillGroupByOrderByCombineOperator.class);
+ private static final String OPERATOR_NAME =
"GapfillGroupByOrderByCombineOperator";
+ private static final String EXPLAIN_NAME = "GAPFILL_COMBINE_GROUPBY_ORDERBY";
Review comment:
This name should be changed to COMBINE_GROUPBY_ORDERBY_GAPFILL to keep
the naming scheme consistent with other Combine operators.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextUtils.java
##########
@@ -47,16 +52,22 @@ public static boolean isSelectionQuery(QueryContext query) {
* Selection-only query at this moment means selection query without
order-by.
*/
public static boolean isSelectionOnlyQuery(QueryContext query) {
- return query.getAggregationFunctions() == null &&
query.getOrderByExpressions() == null;
+ return query.getAggregationFunctions() == null
+ && query.getOrderByExpressions() == null
+ && !GapfillUtils.isGapfill(query);
Review comment:
A null check should be sufficient here to find out if the query is is a
gapfill query or not? `query.getSubQueryContext() == null `
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextUtils.java
##########
@@ -38,7 +39,11 @@ private QueryContextUtils() {
* Returns {@code true} if the given query is a selection query, {@code
false} otherwise.
*/
public static boolean isSelectionQuery(QueryContext query) {
- return query.getAggregationFunctions() == null;
+ if (GapfillUtils.isGapfill(query)) {
+ return isSelectionOnlyQuery(query.getSubQueryContext());
+ } else {
+ return query.getAggregationFunctions() == null;
+ }
Review comment:
Can this be simplified to:
`return query.getAggregationFunctions() == null &&
query.getSubQueryContext() == null`
because if subquery exists then this is automatically a gapfill query?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/GapfillAggregationPlanNode.java
##########
@@ -0,0 +1,175 @@
+/**
+ * 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.core.plan;
+
+import com.google.common.base.Preconditions;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.operator.filter.BaseFilterOperator;
+import org.apache.pinot.core.operator.query.AggregationOperator;
+import org.apache.pinot.core.operator.query.DictionaryBasedAggregationOperator;
+import org.apache.pinot.core.operator.query.MetadataBasedAggregationOperator;
+import org.apache.pinot.core.operator.transform.TransformOperator;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import
org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.startree.CompositePredicateEvaluator;
+import org.apache.pinot.core.startree.StarTreeUtils;
+import org.apache.pinot.core.startree.plan.StarTreeTransformPlanNode;
+import org.apache.pinot.core.util.GapfillUtils;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.index.reader.Dictionary;
+import
org.apache.pinot.segment.spi.index.startree.AggregationFunctionColumnPair;
+import org.apache.pinot.segment.spi.index.startree.StarTreeV2;
+
+
+/**
+ * The <code>GapfillAggregationPlanNode</code> class provides the execution
plan for gapfill aggregation only query on
+ * a single segment.
+ */
+@SuppressWarnings("rawtypes")
+public class GapfillAggregationPlanNode implements PlanNode {
Review comment:
Would it be possible to extend this class from `AggregationPlanNode` or
do some sort of refactoring that would allow both classes to share common code?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GapfillGroupByOrderByCombineOperator.java
##########
@@ -0,0 +1,263 @@
+/**
+ * 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.core.operator.combine;
+
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.List;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.pinot.common.exception.QueryException;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.response.ProcessingException;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.data.table.ConcurrentIndexedTable;
+import org.apache.pinot.core.data.table.IndexedTable;
+import org.apache.pinot.core.data.table.IntermediateRecord;
+import org.apache.pinot.core.data.table.Key;
+import org.apache.pinot.core.data.table.Record;
+import org.apache.pinot.core.data.table.UnboundedConcurrentIndexedTable;
+import org.apache.pinot.core.operator.AcquireReleaseColumnsSegmentOperator;
+import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import
org.apache.pinot.core.query.aggregation.groupby.AggregationGroupByResult;
+import org.apache.pinot.core.query.aggregation.groupby.GroupKeyGenerator;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.util.GapfillUtils;
+import org.apache.pinot.core.util.GroupByUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Combine operator for aggregation group-by queries with SQL semantic.
+ * TODO: Use CombineOperatorUtils.getNumThreadsForQuery() to get the
parallelism of the query instead of using
+ * all threads
+ */
+@SuppressWarnings("rawtypes")
+public class GapfillGroupByOrderByCombineOperator extends BaseCombineOperator {
Review comment:
Except for setting some variables in the constructor, this file appears
to be a close copy of `GroupByOrderByCombineOperator`. I am wondering if it
would be possible to extend `GapfillGroupByOrderByCombineOperator` from
GroupByOrderByCombineOperator to avoid code duplication?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/utils/QueryContextUtils.java
##########
@@ -47,16 +52,22 @@ public static boolean isSelectionQuery(QueryContext query) {
* Selection-only query at this moment means selection query without
order-by.
*/
public static boolean isSelectionOnlyQuery(QueryContext query) {
- return query.getAggregationFunctions() == null &&
query.getOrderByExpressions() == null;
+ return query.getAggregationFunctions() == null
+ && query.getOrderByExpressions() == null
+ && !GapfillUtils.isGapfill(query);
}
/**
- * Returns {@code true} if the given query is an aggregation query, {@code
false} otherwise.
+ * Returns {@code trgue} if the given query is an agregation query, {@code
false} otherwise.
*/
public static boolean isAggregationQuery(QueryContext query) {
- AggregationFunction[] aggregationFunctions =
query.getAggregationFunctions();
- return aggregationFunctions != null && (aggregationFunctions.length != 1
- || !(aggregationFunctions[0] instanceof DistinctAggregationFunction));
+ if (GapfillUtils.isGapfill(query)) {
Review comment:
Are all gapfill queries aggregate queries or if there are exceptions?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/GapfillAggregationGroupByOrderByPlanNode.java
##########
@@ -0,0 +1,110 @@
+/**
+ * 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.core.plan;
+
+import com.google.common.base.Preconditions;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.core.operator.filter.BaseFilterOperator;
+import org.apache.pinot.core.operator.query.AggregationGroupByOrderByOperator;
+import org.apache.pinot.core.operator.transform.TransformOperator;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import
org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.startree.CompositePredicateEvaluator;
+import org.apache.pinot.core.startree.StarTreeUtils;
+import org.apache.pinot.core.startree.plan.StarTreeTransformPlanNode;
+import org.apache.pinot.core.util.GapfillUtils;
+import org.apache.pinot.segment.spi.IndexSegment;
+import
org.apache.pinot.segment.spi.index.startree.AggregationFunctionColumnPair;
+import org.apache.pinot.segment.spi.index.startree.StarTreeV2;
+
+
+/**
+ * The <code>GapfillAggregationGroupByOrderByPlanNode</code> class provides
the execution plan for gapfill aggregation
+ * group-by order-by query on a single segment.
+ */
+@SuppressWarnings("rawtypes")
+public class GapfillAggregationGroupByOrderByPlanNode implements PlanNode {
Review comment:
Would it be possible to extend this class from
`AggregationGroupByOrderByPlanNode` for code reuse or do some refactoring which
would allow both classes to share common code?
--
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
