You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/07/29 14:28:52 UTC
[GitHub] [pinot] mayankshriv commented on a change in pull request #7226: Add functions to return raw results for Percentile TDigest and Est
mayankshriv commented on a change in pull request #7226:
URL: https://github.com/apache/pinot/pull/7226#discussion_r679198631
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawEstAggregationFunction.java
##########
@@ -0,0 +1,137 @@
+/**
+ * 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.aggregation.function;
+
+import java.util.Map;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import org.apache.pinot.segment.local.customobject.QuantileDigest;
+import org.apache.pinot.segment.local.utils.CustomSerDeUtils;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.spi.utils.BytesUtils;
+
+
+public class PercentileRawEstAggregationFunction extends BaseSingleInputAggregationFunction<QuantileDigest, String> {
Review comment:
Please add java docs for all public class/methods?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawEstAggregationFunction.java
##########
@@ -0,0 +1,137 @@
+/**
+ * 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.aggregation.function;
+
+import java.util.Map;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import org.apache.pinot.segment.local.customobject.QuantileDigest;
+import org.apache.pinot.segment.local.utils.CustomSerDeUtils;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.spi.utils.BytesUtils;
+
+
+public class PercentileRawEstAggregationFunction extends BaseSingleInputAggregationFunction<QuantileDigest, String> {
+ private final PercentileEstAggregationFunction _percentileRawEstAggregationFunction;
+
+ public PercentileRawEstAggregationFunction(ExpressionContext expressionContext, double percentile) {
+ this(expressionContext, new PercentileEstAggregationFunction(expressionContext, percentile));
+ }
+
+ public PercentileRawEstAggregationFunction(ExpressionContext expressionContext, int percentile) {
+ this(expressionContext, new PercentileEstAggregationFunction(expressionContext, percentile));
+ }
+
+ PercentileRawEstAggregationFunction(ExpressionContext expression,
+ PercentileEstAggregationFunction percentileRawEstAggregationFunction) {
+ super(expression);
+ _percentileRawEstAggregationFunction = percentileRawEstAggregationFunction;
+ }
+
+ @Override
+ public AggregationFunctionType getType() {
+ return AggregationFunctionType.PERCENTILERAWEST;
+ }
+
+ @Override
+ public String getColumnName() {
+ final double percentile = _percentileRawEstAggregationFunction._percentile;
+ final int version = _percentileRawEstAggregationFunction._version;
+ final String type = getType().getName();
+
+ return version == 0 ? type + (int) percentile + "_" + _expression : type + percentile + "_" + _expression;
+ }
+
+ @Override
+ public String getResultColumnName() {
+ final double percentile = _percentileRawEstAggregationFunction._percentile;
+ final int version = _percentileRawEstAggregationFunction._version;
+ final String type = getType().getName().toLowerCase();
+
+ return version == 0 ? type + (int) percentile + "(" + _expression + ")"
+ : type + "(" + _expression + ", " + percentile + ")";
+ }
+
+ @Override
+ public AggregationResultHolder createAggregationResultHolder() {
+ return _percentileRawEstAggregationFunction.createAggregationResultHolder();
+ }
+
+ @Override
+ public GroupByResultHolder createGroupByResultHolder(int initialCapacity, int maxCapacity) {
+ return _percentileRawEstAggregationFunction.createGroupByResultHolder(initialCapacity, maxCapacity);
+ }
+
+ @Override
+ public void aggregate(int length, AggregationResultHolder aggregationResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ _percentileRawEstAggregationFunction.aggregate(length, aggregationResultHolder, blockValSetMap);
+ }
+
+ @Override
+ public void aggregateGroupBySV(int length, int[] groupKeyArray, GroupByResultHolder groupByResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ _percentileRawEstAggregationFunction.aggregateGroupBySV(length, groupKeyArray, groupByResultHolder, blockValSetMap);
+ }
+
+ @Override
+ public void aggregateGroupByMV(int length, int[][] groupKeysArray, GroupByResultHolder groupByResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ _percentileRawEstAggregationFunction
+ .aggregateGroupByMV(length, groupKeysArray, groupByResultHolder, blockValSetMap);
+ }
+
+ @Override
+ public QuantileDigest extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
+ return _percentileRawEstAggregationFunction.extractAggregationResult(aggregationResultHolder);
+ }
+
+ @Override
+ public QuantileDigest extractGroupByResult(GroupByResultHolder groupByResultHolder, int groupKey) {
+ return _percentileRawEstAggregationFunction.extractGroupByResult(groupByResultHolder, groupKey);
+ }
+
+ @Override
+ public QuantileDigest merge(QuantileDigest intermediateResult1, QuantileDigest intermediateResult2) {
+ return _percentileRawEstAggregationFunction.merge(intermediateResult1, intermediateResult2);
+ }
+
+ @Override
+ public boolean isIntermediateResultComparable() {
+ return _percentileRawEstAggregationFunction.isIntermediateResultComparable();
+ }
+
+ @Override
+ public ColumnDataType getIntermediateResultColumnType() {
+ return _percentileRawEstAggregationFunction.getIntermediateResultColumnType();
+ }
+
+ @Override
+ public ColumnDataType getFinalResultColumnType() {
+ return ColumnDataType.STRING;
+ }
+
+ @Override
+ public String extractFinalResult(QuantileDigest intermediateResult) {
+ return BytesUtils.toHexString(CustomSerDeUtils.QUANTILE_DIGEST_SER_DE.serialize(intermediateResult));
Review comment:
Just talking to myself here - we have used this utility function in several places, and I can never remember its name (including ByteUtils). Perhaps we should make a Pinot Utils wrapper and use that. (Not a blocker for this PR).
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentAggregationMultiValueQueriesTest.java
##########
@@ -519,6 +519,210 @@ public void testPercentileEst99MV() {
.testInterSegmentAggregationResult(brokerResponse, 400000L, 0L, 800000L, 400000L, new String[]{"2147483647"});
}
+ @Test
+ public void testPercentileRawEst50MV() {
Review comment:
We should be able to refactor these tests so that we don't have to write the same code for each of the percentile value test right?
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentAggregationSingleValueQueriesTest.java
##########
@@ -412,6 +412,217 @@ public void testPercentileEst99() {
new String[]{"2146232405", "999309554"});
}
+ @Test
+ public void testPercentileRawEst50() {
Review comment:
same here.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileRawEstAggregationFunction.java
##########
@@ -0,0 +1,137 @@
+/**
+ * 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.aggregation.function;
+
+import java.util.Map;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import org.apache.pinot.segment.local.customobject.QuantileDigest;
+import org.apache.pinot.segment.local.utils.CustomSerDeUtils;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.spi.utils.BytesUtils;
+
+
+public class PercentileRawEstAggregationFunction extends BaseSingleInputAggregationFunction<QuantileDigest, String> {
+ private final PercentileEstAggregationFunction _percentileRawEstAggregationFunction;
+
+ public PercentileRawEstAggregationFunction(ExpressionContext expressionContext, double percentile) {
+ this(expressionContext, new PercentileEstAggregationFunction(expressionContext, percentile));
+ }
+
+ public PercentileRawEstAggregationFunction(ExpressionContext expressionContext, int percentile) {
+ this(expressionContext, new PercentileEstAggregationFunction(expressionContext, percentile));
+ }
+
+ PercentileRawEstAggregationFunction(ExpressionContext expression,
Review comment:
Add explicit access modifier?
--
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