Jackie-Jiang commented on a change in pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#discussion_r664935764
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java
##########
@@ -180,6 +180,7 @@ public static boolean
isFitForDictionaryBasedComputation(String functionName) {
return functionName.equalsIgnoreCase(AggregationFunctionType.MIN.name())
|| functionName.equalsIgnoreCase(AggregationFunctionType.MAX.name())
||
functionName.equalsIgnoreCase(AggregationFunctionType.MINMAXRANGE.name())
+ ||
functionName.equalsIgnoreCase(AggregationFunctionType.DISTINCT.name())
Review comment:
`DISTINCT` is not modeled as aggregation on server side. We have it as a
aggregation function type for historical reason, but we should not reach here
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -158,7 +170,8 @@ public PlanNode makeSegmentPlanNode(IndexSegment
indexSegment, QueryContext quer
} else {
indexSegment.prefetch(queryContext.getColumns());
}
- if (QueryContextUtils.isAggregationQuery(queryContext)) {
+ if (QueryContextUtils.isAggregationQuery(queryContext) ||
+ (QueryContextUtils.isDistinctQuery(queryContext) &&
_useDictionaryForDistinct)) {
Review comment:
We have a branch for distinct queries (line 189 for existing code). You
can check whether the query is fit for dictionary based plan similar to
aggregation queries.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctUsingDictionaryCombineOperator.java
##########
@@ -0,0 +1,127 @@
+/**
+ * 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 it.unimi.dsi.fastutil.doubles.DoubleOpenHashSet;
+import it.unimi.dsi.fastutil.floats.FloatOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.longs.LongOpenHashSet;
+import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import java.util.AbstractCollection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.spi.utils.ByteArray;
+
+/**
+ * Operator which combines partial results for DISTINCT operation when being
executed
+ * using the dictionary
+ */
+public class DistinctUsingDictionaryCombineOperator extends
BaseCombineOperator {
Review comment:
+1. We should not need a separate combine operator
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -69,6 +69,10 @@
public static final String GROUPBY_TRIM_THRESHOLD = "groupby.trim.threshold";
public static final int DEFAULT_GROUPBY_TRIM_THRESHOLD = 1_000_000;
+ // set as pinot.server.query.executor.use.dictionary.for.distinct;
+ public static final String USE_DICTIONARY_FOR_DISTINCT =
"use.dictionary.for.distinct";
Review comment:
This should not be configurable.
We should us the dictionary based operator when:
- There is no filter
- There is only one projection column
- The column is dictionary encoded
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##########
@@ -272,7 +274,20 @@ public BrokerResponseNative
reduceOnDataTable(BrokerRequest brokerRequest,
}
QueryContext queryContext =
BrokerRequestToQueryContextConverter.convert(brokerRequest);
- DataTableReducer dataTableReducer =
ResultReducerFactory.getResultReducer(queryContext);
+
+ // Assuming all elements in the merged data table are of same type
+ Object internalObject = null;
+
+ if (QueryContextUtils.isDistinctQuery(queryContext)) {
Review comment:
There should be no change on broker side, server side should return the
same response with/without the optimization. If not, it is protocol change and
will cause backward incompatible issue
--
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
