Jackie-Jiang commented on a change in pull request #8408:
URL: https://github.com/apache/pinot/pull/8408#discussion_r836675161
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/MetadataBasedAggregationOperator.java
##########
@@ -43,29 +45,45 @@
private final AggregationFunction[] _aggregationFunctions;
private final SegmentMetadata _segmentMetadata;
- private final Map<String, DataSource> _dataSourceMap;
+ private final DataSource[] _dataSources;
public MetadataBasedAggregationOperator(AggregationFunction[]
aggregationFunctions, SegmentMetadata segmentMetadata,
- Map<String, DataSource> dataSourceMap) {
+ DataSource[] dataSources) {
_aggregationFunctions = aggregationFunctions;
_segmentMetadata = segmentMetadata;
-
- // Datasource is currently not used, but will start getting used as we add
support for aggregation
- // functions other than count(*).
- _dataSourceMap = dataSourceMap;
+ _dataSources = dataSources;
}
@Override
protected IntermediateResultsBlock getNextBlock() {
int numAggregationFunctions = _aggregationFunctions.length;
List<Object> aggregationResults = new ArrayList<>(numAggregationFunctions);
- long numTotalDocs = _segmentMetadata.getTotalDocs();
- for (AggregationFunction aggregationFunction : _aggregationFunctions) {
- Preconditions.checkState(aggregationFunction.getType() ==
AggregationFunctionType.COUNT,
- "Metadata based aggregation operator does not support function type:
" + aggregationFunction.getType());
- aggregationResults.add(numTotalDocs);
+ for (int i = 0; i < _aggregationFunctions.length; i++) {
Review comment:
(minor)
```suggestion
for (int i = 0; i < numAggregationFunctions; i++) {
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationOperatorUtils.java
##########
@@ -0,0 +1,35 @@
+/**
+ * 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.query;
+
+public class AggregationOperatorUtils {
+
+ private AggregationOperatorUtils() {
+ }
+
+ public static Double toDouble(Comparable<?> value) {
+ if (value instanceof Double) {
Review comment:
Is this check required?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/DictionaryBasedAggregationOperator.java
##########
@@ -44,6 +44,8 @@
import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.utils.ByteArray;
+import static
org.apache.pinot.core.operator.query.AggregationOperatorUtils.toDouble;
Review comment:
Suggest not using static function import for readability. Since we
already put the rule in the checkstyle, let's not adding this exception
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java
##########
@@ -176,11 +181,18 @@ private TransformOperator
buildTransformOperatorForFilteredAggregates(BaseFilter
BaseFilterOperator filterOperator = filterPlanNode.run();
// Use metadata/dictionary to solve the query if possible
- // TODO: Use the same operator for both of them so that COUNT(*), MAX(col)
can be optimized
Review comment:
Don't remove this TODO because it is not solved yet (if a query contains
half of the aggregation solvable by metadata, and another half solvable by
dictionary, we cannot optimize it now). Since we are already doing the
optimization, we can probably solve this TODO in this PR by combining both
operators into one?
--
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
