yashmayya commented on code in PR #19972:
URL: https://github.com/apache/pinot/pull/19972#discussion_r4329888052
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java:
##########
@@ -152,6 +166,20 @@ public DataTable getDataTable()
return dataTableBuilder.build();
}
+ /// Extracts the final results before the data table is built when the
server returns them, because the metadata
+ /// is read first, and has to report a function that returned an approximate
result.
+ @Override
+ public Map<String, String> getResultsMetadata() {
Review Comment:
Confirmed, and this PR introduced it: before, only the consumer extracted.
It also affected exact `PERCENTILE`, not only the SMART one, because
`getFinalResults()` extracts every function in the block. Fixed in d704f8cf42:
`getFinalResults()` is now `synchronized` and caches the result, so the
metadata, the rows and the data table share one extraction.
`testFinalResultsAreExtractedOnce` covers the single extraction.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java:
##########
@@ -132,6 +133,11 @@ public BrokerResponseNative
reduceOnDataTable(BrokerRequest brokerRequest, Broke
"Caught exception while reducing data tables: " + e.getMessage()));
}
}
+ // The reducers extract the final results with the functions of this query
context, which record a result that
+ // turned approximate. Servers that extracted final results themselves
report it through the aggregator instead.
+ if
(AggregationFunctionUtils.hasApproximatedResult(serverQueryContext.getAggregationFunctions()))
{
Review Comment:
Agreed, this is a false negative. I am keeping it out of this PR. It fits
better with the single choke point from your design comment, as a follow-up.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ApproximateFunctionOverrideIntegrationTest.java:
##########
@@ -98,58 +100,73 @@ public void testClusterConfigAppliesWithoutRestart(boolean
useMultiStageQueryEng
JsonNode exactDistinctCount = postQuery(DISTINCT_COUNT_QUERY);
JsonNode exactPercentile = postQuery(PERCENTILE_QUERY);
- assertApplied(exactDistinctCount, false);
- assertApplied(exactPercentile, false);
+ assertApproximated(exactDistinctCount, false);
+ assertApproximated(exactPercentile, false);
// A threshold far above the cardinality of the test data keeps the
rewritten functions exact, which is what makes
- // the answers comparable before and after the flip.
+ // the answers comparable before and after the flip. The results stay
exact, so the flag stays off.
setApproximateFunctionOverride("threshold=1000000");
waitForOverride(true);
JsonNode distinctCount = postQuery(DISTINCT_COUNT_QUERY);
- assertApplied(distinctCount, true);
+ assertApproximated(distinctCount, false);
assertSameResult(exactDistinctCount, distinctCount);
JsonNode percentile = postQuery(PERCENTILE_QUERY);
- assertApplied(percentile, true);
+ assertApproximated(percentile, false);
assertSameResult(exactPercentile, percentile);
+ // A threshold below the cardinality has to make the answer approximate
rather than fail, keeping the same shape,
+ // and the flag reports it. This is also what proves the parameters reach
the servers and parse there.
+ setApproximateFunctionOverride("threshold=1");
+ waitForOverride(true);
Review Comment:
Good catch, fixed. After the switch to `threshold=1`, the test now waits
until both engines report `approximatedResult=true` on the percentile query.
The percentile params are the last key that the test writes, so the distinct
count params have arrived too by then.
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -167,6 +163,11 @@ public class BrokerMeter implements AbstractMetrics.Meter {
public static final BrokerMeter
BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED = create(
"BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED", "badResponses", false);
+ // This metric tracks the number of broker responses with an approximated
result: a function that can answer exactly,
Review Comment:
Agreed, this has to reach the 1.6.0 line before 1.6.0 ships, or the old
names become released API.
On a rewrite-activity signal: the single-stage column name and the
multi-stage plan already show that the rewrite is on. I would rather not add
another meter or flag for it unless operators ask for one.
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTable.java:
##########
@@ -173,11 +173,14 @@ enum MetadataKey {
// The subset of the remote accesses made directly, i.e. outside the
buffers fetched for the query, and the number
// of bytes they read.
NUM_DIRECT_REMOTE_ACCESSES(47, "numDirectRemoteAccesses",
MetadataValueType.LONG),
- DIRECT_REMOTE_ACCESS_BYTES(48, "directRemoteAccessBytes",
MetadataValueType.LONG);
+ DIRECT_REMOTE_ACCESS_BYTES(48, "directRemoteAccessBytes",
MetadataValueType.LONG),
+ // Set when an aggregation function that can answer exactly returned an
approximate final result on the server,
Review Comment:
Agreed. I added this to the compatibility notes in the PR description:
upgrading brokers before servers avoids it, and canary servers or a downgrade
do not. A real fix of the skip needs the value type of an unknown key, so I am
leaving it out of this PR.
You were also right in the review summary about the `StatMap` case: the
receiving mailbox catches the error and drops the stats of that query. I
corrected the PR description, which said the query fails.
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTable.java:
##########
@@ -173,11 +173,14 @@ enum MetadataKey {
// The subset of the remote accesses made directly, i.e. outside the
buffers fetched for the query, and the number
// of bytes they read.
NUM_DIRECT_REMOTE_ACCESSES(47, "numDirectRemoteAccesses",
MetadataValueType.LONG),
- DIRECT_REMOTE_ACCESS_BYTES(48, "directRemoteAccessBytes",
MetadataValueType.LONG);
+ DIRECT_REMOTE_ACCESS_BYTES(48, "directRemoteAccessBytes",
MetadataValueType.LONG),
+ // Set when an aggregation function that can answer exactly returned an
approximate final result on the server,
+ // for example a SMART function that switched to a sketch.
+ APPROXIMATED_RESULT(49, "approximatedResult", MetadataValueType.STRING);
// We keep this constant to track the max id added so far for backward
compatibility.
Review Comment:
Agreed. Added to the PR description as a release note.
--
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]
