yashmayya commented on code in PR #19972:
URL: https://github.com/apache/pinot/pull/19972#discussion_r4329890095
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunction.java:
##########
@@ -316,6 +316,18 @@ default FinalResult mergeFinalResult(FinalResult
finalResult1, FinalResult final
throw new UnsupportedOperationException("Cannot merge final results for
function: " + getType());
}
+ /// Returns whether [#extractFinalResult] has returned an approximate result
for a function that can also answer
+ /// exactly. The SMART functions start exact and switch a group to a sketch
once it crosses their conversion
+ /// threshold. A merge with a sketch always returns a sketch, so the
extraction sees the switch, and records it here.
+ /// Read it after extracting, to report the result as approximate.
+ ///
+ /// A function instance belongs to one query, and so does the flag. Group
trimming also extracts final results, so a
+ /// group that is trimmed away can still set it. Functions that are always
exact or always approximate keep the
+ /// default.
+ default boolean hasApproximatedResult() {
Review Comment:
Agreed that one choke point (for example `QueryThreadContext`) would be
better and would also cover gapfill. I am keeping this PR as it is and leaving
that as a follow-up.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/BaseDistinctCountSmartSketchAggregationFunction.java:
##########
@@ -52,11 +52,19 @@ abstract class
BaseDistinctCountSmartSketchAggregationFunction
// Use empty IntOpenHashSet as a placeholder for empty result
protected static final IntSet EMPTY_PLACEHOLDER = new IntOpenHashSet();
+ /// Set by `extractFinalResult` when it reads a sketch, see
[AggregationFunction#hasApproximatedResult()].
+ protected volatile boolean _approximatedResult;
Review Comment:
Done in d704f8cf42: the SMART functions now read the flag before they write
it.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -296,6 +298,9 @@ public void setStats(String rawTableName,
BrokerResponseNative brokerResponseNat
brokerResponseNative.setGroupsTrimmed(_groupsTrimmed);
brokerResponseNative.setNumGroupsLimitReached(_numGroupsLimitReached);
brokerResponseNative.setNumGroupsWarningLimitReached(_numGroupsWarningLimitReached);
+ if (_approximatedResult) {
Review Comment:
Done: it now sets the flag directly, like its neighbors.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/GroupByResultsBlock.java:
##########
@@ -237,6 +238,10 @@ public Map<String, String> getResultsMetadata() {
if (_numGroupsWarningLimitReached) {
metadata.put(MetadataKey.NUM_GROUPS_WARNING_LIMIT_REACHED.getName(),
"true");
}
+ // The combine step has already extracted the final results when the
server returns them.
Review Comment:
Done: the comment now says that ORDER BY trimming also extracts final
results, also for groups that it trims away.
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlockTest.java:
##########
@@ -0,0 +1,61 @@
+/**
+ * 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.blocks.results;
+
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.common.datatable.DataTable;
+import org.apache.pinot.common.datatable.DataTable.MetadataKey;
+import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import
org.apache.pinot.core.query.request.context.utils.QueryContextConverterUtils;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+public class AggregationResultsBlockTest {
Review Comment:
Done.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/SmartFunctionApproximatedResultTest.java:
##########
@@ -0,0 +1,83 @@
+/**
+ * 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 it.unimi.dsi.fastutil.doubles.DoubleArrayList;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import java.util.List;
+import org.apache.pinot.common.request.Literal;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+
+/// Covers [AggregationFunction#hasApproximatedResult()] for the SMART
functions: extracting an exact accumulator leaves
+/// it unset, and extracting one that a merge switched to a sketch sets it.
+@SuppressWarnings({"rawtypes", "unchecked"})
+public class SmartFunctionApproximatedResultTest {
+ private static final ExpressionContext COLUMN =
ExpressionContext.forIdentifier("col");
+ private static final ExpressionContext THRESHOLD_ONE =
+ ExpressionContext.forLiteral(Literal.stringValue("threshold=1"));
+
+ @DataProvider
+ public static Object[][] distinctCountFunctions() {
+ List<ExpressionContext> arguments = List.of(COLUMN, THRESHOLD_ONE);
+ return new Object[][]{
+ {new DistinctCountSmartHLLAggregationFunction(arguments, false)},
+ {new DistinctCountSmartHLLPlusAggregationFunction(arguments, false)},
+ {new DistinctCountSmartULLAggregationFunction(arguments, false)}
+ };
+ }
+
+ @Test(dataProvider = "distinctCountFunctions")
+ public void testDistinctCount(AggregationFunction function) {
+ assertEquals(function.extractFinalResult(null), 0);
+ assertEquals(function.extractFinalResult(new IntOpenHashSet(new
int[]{1})), 1);
+ assertFalse(function.hasApproximatedResult());
+
+ // Two values cross the threshold of one, so the merge switches the
accumulator to a sketch.
+ Object merged = function.merge(new IntOpenHashSet(new int[]{1}), new
IntOpenHashSet(new int[]{2}));
+ function.extractFinalResult(merged);
+ assertTrue(function.hasApproximatedResult());
+ }
+
+ @Test
+ public void testPercentile() {
+ PercentileSmartTDigestAggregationFunction function = new
PercentileSmartTDigestAggregationFunction(
+ List.of(COLUMN,
ExpressionContext.forLiteral(Literal.doubleValue(50.0)), THRESHOLD_ONE), false);
+ function.extractFinalResult(null);
Review Comment:
Done: it now asserts the `Double.NEGATIVE_INFINITY` 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
