Add metrics for Accumulator and GroupByHash #24015
Conversation
| private long accumulatorTimeNanos; | ||
| private boolean hasAccumulator; | ||
| private long groupByHashTimeNanos; | ||
| private boolean hasGroupByHash; | ||
| private long inputRowsProcessedWithPartialAggregationDisabled; | ||
| private boolean hasInputRowsProcessedWithPartialAggregationDisabled; |
There was a problem hiding this comment.
why do we need those boolean flags?
Why not just return 0 for all those values by defualt?
There was a problem hiding this comment.
I followed what we already have in PageProcessorMetrics. Additionally, inputRowsProcessedWithPartialAggregationDisabled was handled in a similar way.
I assumed the reasoning was to avoid polluting the explain output with metrics that weren’t reported, as unreported metrics are irrelevant for a given operator.
Perhaps we could replace the boolean flags by checking if a given metric has a value greater than zero, and only expose it in the AggregationMetrics::getMetrics method in that case. WDYT?
core/trino-main/src/main/java/io/trino/operator/AggregationMetrics.java
Outdated
Show resolved
Hide resolved
| static final String INPUT_ROWS_WITH_PARTIAL_AGGREGATION_DISABLED_METRIC_NAME = "Input rows processed without partial aggregation enabled"; | ||
|
|
||
| private long inputRowsProcessedWithPartialAggregationDisabled; | ||
| private boolean hasInputRowsProcessedWithPartialAggregationDisabled; |
There was a problem hiding this comment.
Why not just check if inputRowsProcessedWithPartialAggregationDisabled > 0?
| { | ||
| ImmutableMap.Builder<String, Metric<?>> builder = ImmutableMap.builder(); | ||
| if (hasInputRowsProcessedWithPartialAggregationDisabled) { | ||
| builder.put(INPUT_ROWS_WITH_PARTIAL_AGGREGATION_DISABLED_METRIC_NAME, new LongCount(inputRowsProcessedWithPartialAggregationDisabled)); |
There was a problem hiding this comment.
Since this is part of explain analyze verbose we probably could print it always
There was a problem hiding this comment.
I changed this so that all these metrics are always printed.
core/trino-main/src/main/java/io/trino/operator/AggregationMetrics.java
Outdated
Show resolved
Hide resolved
| { | ||
| long start = System.nanoTime(); | ||
| boolean result = delegate.process(); | ||
| metrics.recordGroupByHashUpdateTimeSince(start); |
There was a problem hiding this comment.
nit: alternatively we could use OperationTiming as in
But I'm fine keeping it as is for now
This is in preparation for including more metrics reported by aggregation operators
piotrrzysko
force-pushed
the
group-by-stats
branch
from
9c84a9a
to
433a184
Compare
|
comments addressed |
Description
Additional context and related issues
Fixes #21925
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: