mayankshriv commented on a change in pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#discussion_r664737964
##########
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";
+ public static final boolean DEFAULT_USE_DICTIONARY_FOR_DISTINCT = false;
Review comment:
What steps need to be done to make it ON by default?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/DictionaryBasedAggregationOperator.java
##########
@@ -80,46 +84,73 @@ protected IntermediateResultsBlock getNextBlock() {
aggregationResults
.add(new MinMaxRangePair(dictionary.getDoubleValue(0),
dictionary.getDoubleValue(dictionarySize - 1)));
break;
+ case DISTINCT:
case DISTINCTCOUNT:
switch (dictionary.getValueType()) {
case INT:
IntOpenHashSet intSet = new IntOpenHashSet(dictionarySize);
for (int dictId = 0; dictId < dictionarySize; dictId++) {
+
+ // Enforce limits only for DISTINCT
+ if (aggregationFunction.getType() == DISTINCT && dictId ==
_limit) {
Review comment:
Would be (arguably) faster if we avoid the `if` inside of this for loop?
That means we will have to have separate the `DISTINCT` and `DISTINCTCOUNT`
cases.
##########
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:
One issue with server side configs is that different servers of the same
table may potentially have different settings (due to user error). Perhaps this
could be a query option, if we want to start with off-by-default?
##########
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:
Shouldn't only the DistinctOperator be dictionary based at segment level
(since dictionary id's are scoped within segment level)? If so, why do we need
a combine and reduce operator for the dictionary based approach? Beyond segment
level, we should be able to reuse the existing combine/reduce operators?
Nit: `DictionaryBasedCombineOperator` to match existing naming convention?
##########
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 {
+ private static final String OPERATOR_NAME =
"DistinctUsingDictionaryCombineOperator";
+
+ private final boolean _hasOrderBy;
+
+ public DistinctUsingDictionaryCombineOperator(List<Operator> operators,
QueryContext queryContext, ExecutorService executorService,
+ long endTimeMs) {
+ super(operators, queryContext, executorService, endTimeMs);
Review comment:
Some of these seem similar to `DistinctCombineOperator`, does it make
sense fo this class to extend from it?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/DistinctUsingDictionaryDataTableReducer.java
##########
@@ -0,0 +1,164 @@
+/**
+ * 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.query.reduce;
+
+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.io.Serializable;
+import java.util.AbstractCollection;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.common.response.broker.ResultTable;
+import org.apache.pinot.common.response.broker.SelectionResults;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataTable;
+import org.apache.pinot.core.data.table.Record;
+import
org.apache.pinot.core.query.aggregation.function.DistinctAggregationFunction;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.transport.ServerRoutingInstance;
+import org.apache.pinot.core.util.QueryOptions;
+
+/**
+ * Reducer for reducing partial results for DISTINCT operation when being
executed using dictionary based plan
+ */
+public class DistinctUsingDictionaryDataTableReducer implements
DataTableReducer {
Review comment:
Same nit for name `DictionaryBasedDistinctDataTableReducer`.
##########
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:
Hmm, any way to avoid this special casing here? I know we have
special-cased DISTINCT in other places, but that is something we want to remove.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/DistinctUsingDictionaryDataTableReducer.java
##########
@@ -0,0 +1,164 @@
+/**
+ * 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.query.reduce;
+
+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.io.Serializable;
+import java.util.AbstractCollection;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.common.response.broker.ResultTable;
+import org.apache.pinot.common.response.broker.SelectionResults;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataTable;
+import org.apache.pinot.core.data.table.Record;
+import
org.apache.pinot.core.query.aggregation.function.DistinctAggregationFunction;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.transport.ServerRoutingInstance;
+import org.apache.pinot.core.util.QueryOptions;
+
+/**
+ * Reducer for reducing partial results for DISTINCT operation when being
executed using dictionary based plan
+ */
+public class DistinctUsingDictionaryDataTableReducer implements
DataTableReducer {
+ private final DistinctAggregationFunction _distinctAggregationFunction;
+ private final boolean _responseFormatSql;
+
+ // TODO: queryOptions.isPreserveType() is ignored for DISTINCT queries.
Review comment:
What's the implication of this? Is this a blocker for on-by-default?
--
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
