From 6e1048c94834e705ca9f1a91cf2f86b6ba08dcb3 Mon Sep 17 00:00:00 2001 From: GWphua Date: Wed, 26 Nov 2025 18:53:50 +0800 Subject: [PATCH 01/39] Metrics in GroupByQueryMetrics --- .../druid/query/DefaultQueryMetrics.java | 10 ++++ .../groupby/DefaultGroupByQueryMetrics.java | 54 +++++++++++++++++++ .../query/groupby/GroupByQueryMetrics.java | 21 ++++++++ 3 files changed, 85 insertions(+) diff --git a/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java index f3a4539f7808..c7e0f1af2118 100644 --- a/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java @@ -464,6 +464,16 @@ protected QueryMetrics reportMetric(String metricName, Number value) return this; } + protected QueryMetrics reportMetricsIfNotZero(String metricName, Number value) + { + checkModifiedFromOwnerThread(); + if (value != null && !value.equals(0)) { + metrics.put(metricName, value); + } + + return this; + } + private QueryMetrics reportMillisTimeMetric(String metricName, long timeNs) { return reportMetric(metricName, TimeUnit.NANOSECONDS.toMillis(timeNs)); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index 6b02d8c29bb3..0edf00f02b37 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -22,6 +22,8 @@ import org.apache.druid.query.DefaultQueryMetrics; import org.apache.druid.query.DruidMetrics; +import java.util.concurrent.atomic.LongAdder; + public class DefaultGroupByQueryMetrics extends DefaultQueryMetrics implements GroupByQueryMetrics { @Override @@ -58,4 +60,56 @@ public void granularity(GroupByQuery query) { //Don't emit by default } + + // GroupByQueries: + private final LongAdder mergeBufferAcquisitonTime = new LongAdder(); + private final LongAdder bytesSpilledToStorage = new LongAdder(); + private final LongAdder mergeBufferBytesUsed = new LongAdder(); + private final LongAdder mergeDictionarySize = new LongAdder(); + + @Override + public void reportGroupByStats() + { + reportMetricsIfNotZero("bytesSpilledToStorage", bytesSpilledToStorage); + reportMetricsIfNotZero("mergeDictionarySize", mergeDictionarySize); + reportMetricsIfNotZero("mergeBufferBytesUsed", mergeBufferBytesUsed); + reportMetricsIfNotZero("mergeBufferAcquisitonTimeNs", mergeBufferAcquisitonTime); + } + + @Override + public void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime) + { + this.mergeBufferAcquisitonTime.add(mergeBufferAcquisitionTime); + } + + @Override + public void bytesSpilledToStorage(long bytesSpilledToStorage) + { + this.bytesSpilledToStorage.add(bytesSpilledToStorage); + } + + @Override + public void mergeDictionarySize(long mergeDictionarySize) + { + this.mergeDictionarySize.add(mergeDictionarySize); + } + + @Override + public long getSpilledBytes() + { + return bytesSpilledToStorage.longValue(); + } + + @Override + public long getMergeDictionarySize() + { + return mergeDictionarySize.longValue(); + } + + @Override + public long getMergeBufferAcquisitionTime() + { + return mergeBufferAcquisitonTime.longValue(); + } + } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java index 292828fc9858..80e5d4af0193 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java @@ -54,4 +54,25 @@ public interface GroupByQueryMetrics extends QueryMetrics */ @PublicApi void granularity(GroupByQuery query); + + @PublicApi + void reportGroupByStats(); + + @PublicApi + void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime); + + @PublicApi + void bytesSpilledToStorage(long bytesSpilledToStorage); + + @PublicApi + void mergeDictionarySize(long mergeDictionarySize); + + @PublicApi + long getSpilledBytes(); + + @PublicApi + long getMergeDictionarySize(); + + @PublicApi + long getMergeBufferAcquisitionTime(); } From 0c97263c295ccf14422ce2f4fc3e1724a6a49dfc Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 27 Nov 2025 11:10:40 +0800 Subject: [PATCH 02/39] Time to remove PerQueryStats P1 --- .../groupby/DefaultGroupByQueryMetrics.java | 11 +-- .../groupby/GroupByQueryQueryToolChest.java | 2 +- .../query/groupby/GroupByStatsProvider.java | 82 +++++-------------- .../GroupByMergingQueryRunner.java | 4 + 4 files changed, 29 insertions(+), 70 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index 0edf00f02b37..2bec110f1fd9 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -26,6 +26,10 @@ public class DefaultGroupByQueryMetrics extends DefaultQueryMetrics implements GroupByQueryMetrics { + private final LongAdder mergeBufferAcquisitonTime = new LongAdder(); + private final LongAdder bytesSpilledToStorage = new LongAdder(); + private final LongAdder mergeDictionarySize = new LongAdder(); + @Override public void query(GroupByQuery query) { @@ -61,18 +65,11 @@ public void granularity(GroupByQuery query) //Don't emit by default } - // GroupByQueries: - private final LongAdder mergeBufferAcquisitonTime = new LongAdder(); - private final LongAdder bytesSpilledToStorage = new LongAdder(); - private final LongAdder mergeBufferBytesUsed = new LongAdder(); - private final LongAdder mergeDictionarySize = new LongAdder(); - @Override public void reportGroupByStats() { reportMetricsIfNotZero("bytesSpilledToStorage", bytesSpilledToStorage); reportMetricsIfNotZero("mergeDictionarySize", mergeDictionarySize); - reportMetricsIfNotZero("mergeBufferBytesUsed", mergeBufferBytesUsed); reportMetricsIfNotZero("mergeBufferAcquisitonTimeNs", mergeBufferAcquisitonTime); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index aa838591ccb7..0ddc2934c86a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -217,7 +217,7 @@ private Sequence initAndMergeGroupByResults( // Clean up the resources reserved during the execution of the query closer.register(() -> groupByResourcesReservationPool.clean(queryResourceId)); - closer.register(() -> groupByStatsProvider.closeQuery(query.context().getQueryResourceId())); +// closer.register(() -> groupByStatsProvider.closeQuery(query.context().getQueryResourceId())); return Sequences.withBaggage(mergedSequence, closer); } catch (Exception e) { diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java index a5ce31cb5f98..db97dac91012 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java @@ -22,9 +22,7 @@ import org.apache.druid.guice.LazySingleton; import org.apache.druid.query.QueryResourceId; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicLong; +import javax.annotation.Nullable; /** * Metrics collector for groupBy queries like spilled bytes, merge buffer acquistion time, dictionary size. @@ -32,21 +30,20 @@ @LazySingleton public class GroupByStatsProvider { - private final Map perQueryStats; private final AggregateStats aggregateStatsContainer; public GroupByStatsProvider() { - this.perQueryStats = new ConcurrentHashMap<>(); - this.aggregateStatsContainer = new AggregateStats(); + this.aggregateStatsContainer = AggregateStats.EMPTY_STATS; } - public PerQueryStats getPerQueryStatsContainer(QueryResourceId resourceId) + public void aggregateStats(@Nullable GroupByQueryMetrics groupByQueryMetrics) { - if (resourceId == null) { - return null; + if (groupByQueryMetrics == null) { + return; } - return perQueryStats.computeIfAbsent(resourceId, value -> new PerQueryStats()); + + aggregateStatsContainer.addQueryStats(groupByQueryMetrics); } public synchronized void closeQuery(QueryResourceId resourceId) @@ -65,15 +62,13 @@ public synchronized AggregateStats getStatsSince() public static class AggregateStats { - private long mergeBufferQueries = 0; - private long mergeBufferAcquisitionTimeNs = 0; - private long spilledQueries = 0; - private long spilledBytes = 0; - private long mergeDictionarySize = 0; + private long mergeBufferQueries; + private long mergeBufferAcquisitionTimeNs; + private long spilledQueries; + private long spilledBytes; + private long mergeDictionarySize; - public AggregateStats() - { - } + public static final AggregateStats EMPTY_STATS = new AggregateStats(0L, 0L, 0L, 0L, 0L); public AggregateStats( long mergeBufferQueries, @@ -115,22 +110,22 @@ public long getMergeDictionarySize() return mergeDictionarySize; } - public void addQueryStats(PerQueryStats perQueryStats) + private void addQueryStats(GroupByQueryMetrics groupByQueryMetrics) { - if (perQueryStats.getMergeBufferAcquisitionTimeNs() > 0) { + if (groupByQueryMetrics.getMergeBufferAcquisitionTime() > 0) { mergeBufferQueries++; - mergeBufferAcquisitionTimeNs += perQueryStats.getMergeBufferAcquisitionTimeNs(); + mergeBufferAcquisitionTimeNs += groupByQueryMetrics.getMergeBufferAcquisitionTime(); } - if (perQueryStats.getSpilledBytes() > 0) { + if (groupByQueryMetrics.getSpilledBytes() > 0) { spilledQueries++; - spilledBytes += perQueryStats.getSpilledBytes(); + spilledBytes += groupByQueryMetrics.getSpilledBytes(); } - mergeDictionarySize += perQueryStats.getMergeDictionarySize(); + mergeDictionarySize += groupByQueryMetrics.getMergeDictionarySize(); } - public AggregateStats reset() + private AggregateStats reset() { AggregateStats aggregateStats = new AggregateStats( @@ -150,41 +145,4 @@ public AggregateStats reset() return aggregateStats; } } - - public static class PerQueryStats - { - private final AtomicLong mergeBufferAcquisitionTimeNs = new AtomicLong(0); - private final AtomicLong spilledBytes = new AtomicLong(0); - private final AtomicLong mergeDictionarySize = new AtomicLong(0); - - public void mergeBufferAcquisitionTime(long delay) - { - mergeBufferAcquisitionTimeNs.addAndGet(delay); - } - - public void spilledBytes(long bytes) - { - spilledBytes.addAndGet(bytes); - } - - public void dictionarySize(long size) - { - mergeDictionarySize.addAndGet(size); - } - - public long getMergeBufferAcquisitionTimeNs() - { - return mergeBufferAcquisitionTimeNs.get(); - } - - public long getSpilledBytes() - { - return spilledBytes.get(); - } - - public long getMergeDictionarySize() - { - return mergeDictionarySize.get(); - } - } } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index 43bec66de7c5..9bc9c3235fc1 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -306,6 +306,10 @@ public AggregateResult call() waitForFutureCompletion(query, futures, hasTimeout, timeoutAt - System.currentTimeMillis()); } + // grouper.updateGroupByQueryMetrics(); + // groupByStatsProvider.aggregateStats(queryMetrics); +// queryMetrics.reportGroupByStats(); + return RowBasedGrouperHelper.makeGrouperIterator( grouper, query, From 34431e7ba17856da05b0fc9b8c3764a8515ffb08 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 27 Nov 2025 15:31:48 +0800 Subject: [PATCH 03/39] Time to remove PerQueryStats P2: Removed in QueryToolChest + LimitedStorage + MergingQueryRunner --- .../apache/druid/query/QueryDataSource.java | 2 +- .../groupby/GroupByQueryQueryToolChest.java | 238 ++++++++---------- .../GroupByResourcesReservationPool.java | 6 +- .../druid/query/groupby/GroupingEngine.java | 22 +- .../GroupByMergingQueryRunner.java | 14 +- .../epinephelinae/GroupByRowProcessor.java | 12 +- .../LimitedTemporaryStorage.java | 11 +- .../epinephelinae/ConcurrentGrouperTest.java | 13 +- 8 files changed, 138 insertions(+), 180 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/QueryDataSource.java b/processing/src/main/java/org/apache/druid/query/QueryDataSource.java index 5fc3ab5b8eeb..029daf8bfe57 100644 --- a/processing/src/main/java/org/apache/druid/query/QueryDataSource.java +++ b/processing/src/main/java/org/apache/druid/query/QueryDataSource.java @@ -56,7 +56,7 @@ public Set getTableNames() } @JsonProperty - public Query getQuery() + public Query getQuery() { return query; } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 0ddc2934c86a..52ae00194502 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -24,6 +24,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Functions; +import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import com.google.common.collect.Lists; import com.google.inject.Inject; @@ -96,7 +97,6 @@ public class GroupByQueryQueryToolChest extends QueryToolChest queryConfigSupplier, GroupByQueryMetricsFactory queryMetricsFactory, - @Merging GroupByResourcesReservationPool groupByResourcesReservationPool, - GroupByStatsProvider groupByStatsProvider + @Merging GroupByResourcesReservationPool groupByResourcesReservationPool ) { this.groupingEngine = groupingEngine; this.queryConfig = queryConfigSupplier.get(); this.queryMetricsFactory = queryMetricsFactory; this.groupByResourcesReservationPool = groupByResourcesReservationPool; - this.groupByStatsProvider = groupByStatsProvider; } @Override @@ -151,7 +132,6 @@ public QueryRunner mergeResults(final QueryRunner runner) return mergeResults(runner, true); } - @Override public QueryRunner mergeResults(final QueryRunner runner, boolean willMergeRunner) { @@ -160,8 +140,7 @@ public QueryRunner mergeResults(final QueryRunner runner, return runner.run(queryPlus, responseContext); } - final GroupByQuery groupByQuery = (GroupByQuery) queryPlus.getQuery(); - return initAndMergeGroupByResults(groupByQuery, runner, responseContext, willMergeRunner); + return initAndMergeGroupByResults(queryPlus, runner, responseContext, willMergeRunner); }; } @@ -178,23 +157,22 @@ public Comparator createResultComparator(Query query) } private Sequence initAndMergeGroupByResults( - final GroupByQuery query, + final QueryPlus queryPlus, QueryRunner runner, ResponseContext context, boolean willMergeRunner ) { + final GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); + final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); + // Reserve the group by resources (merge buffers) required for executing the query final QueryResourceId queryResourceId = query.context().getQueryResourceId(); - final GroupByStatsProvider.PerQueryStats perQueryStats = - groupByStatsProvider.getPerQueryStatsContainer(query.context().getQueryResourceId()); - - groupByResourcesReservationPool.reserve( - queryResourceId, - query, - willMergeRunner, - perQueryStats - ); + + long startNs = System.nanoTime(); + groupByResourcesReservationPool.reserve(queryResourceId, query, willMergeRunner); + groupByQueryMetrics.mergeBufferAcquisitionTime(System.nanoTime() - startNs); final GroupByQueryResources resource = groupByResourcesReservationPool.fetch(queryResourceId); if (resource == null) { @@ -207,17 +185,18 @@ private Sequence initAndMergeGroupByResults( Closer closer = Closer.create(); final Sequence mergedSequence = mergeGroupByResults( - query, + queryPlus, resource, runner, context, - closer, - perQueryStats + closer ); // Clean up the resources reserved during the execution of the query closer.register(() -> groupByResourcesReservationPool.clean(queryResourceId)); -// closer.register(() -> groupByStatsProvider.closeQuery(query.context().getQueryResourceId())); + // TODO: Maybe attach metrics reporting with the closer? + // closer.register(() -> groupByStatsProvider.closeQuery(query.context().getQueryResourceId())); + return Sequences.withBaggage(mergedSequence, closer); } catch (Exception e) { @@ -228,132 +207,118 @@ private Sequence initAndMergeGroupByResults( } private Sequence mergeGroupByResults( - final GroupByQuery query, + final QueryPlus queryPlus, GroupByQueryResources resource, QueryRunner runner, ResponseContext context, - Closer closer, - GroupByStatsProvider.PerQueryStats perQueryStats + Closer closer ) { - if (isNestedQueryPushDown(query)) { - return mergeResultsWithNestedQueryPushDown(query, resource, runner, context, perQueryStats); + if (isNestedQueryPushDown((GroupByQuery) queryPlus.getQuery())) { + return mergeResultsWithNestedQueryPushDown(queryPlus, resource, runner, context); } - return mergeGroupByResultsWithoutPushDown(query, resource, runner, context, closer, perQueryStats); + return mergeGroupByResultsWithoutPushDown(queryPlus, resource, runner, context, closer); } private Sequence mergeGroupByResultsWithoutPushDown( - GroupByQuery query, + QueryPlus queryPlus, GroupByQueryResources resource, QueryRunner runner, ResponseContext context, - Closer closer, - GroupByStatsProvider.PerQueryStats perQueryStats + Closer closer ) { // If there's a subquery, merge subquery results and then apply the aggregator + GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); - final DataSource dataSource = query.getDataSource(); - - if (dataSource instanceof QueryDataSource) { - final GroupByQuery subquery; - try { - // Inject outer query context keys into subquery if they don't already exist in the subquery context. - // Unlike withOverriddenContext's normal behavior, we want keys present in the subquery to win. - final Map subqueryContext = new TreeMap<>(); - if (query.getContext() != null) { - for (Map.Entry entry : query.getContext().entrySet()) { - if (entry.getValue() != null) { - subqueryContext.put(entry.getKey(), entry.getValue()); - } - } - } - if (((QueryDataSource) dataSource).getQuery().getContext() != null) { - subqueryContext.putAll(((QueryDataSource) dataSource).getQuery().getContext()); - } - subqueryContext.put(GroupByQuery.CTX_KEY_SORT_BY_DIMS_FIRST, false); - subquery = (GroupByQuery) ((QueryDataSource) dataSource).getQuery().withOverriddenContext(subqueryContext); + final DataSource maybeQueryDataSource = query.getDataSource(); - closer.register(() -> groupByStatsProvider.closeQuery(subquery.context().getQueryResourceId())); + if (!(maybeQueryDataSource instanceof QueryDataSource)) { + if (query.getSubtotalsSpec() == null) { + return groupingEngine.applyPostProcessing(groupingEngine.mergeResults(runner, queryPlus, context), query); + } else { + return groupingEngine.processSubtotalsSpec( + queryPlus, + resource, + groupingEngine.mergeResults(runner, queryPlus.withQuery(query.withSubtotalsSpec(null)), context) + ); } - catch (ClassCastException e) { - throw new UnsupportedOperationException("Subqueries must be of type 'group by'"); + } + + final QueryDataSource dataSource = (QueryDataSource) maybeQueryDataSource; + // Inject outer query context keys into subquery if they don't already exist in the subquery context. + // Unlike withOverriddenContext's normal behavior, we want keys present in the subquery to win. + final Map subqueryContext = new TreeMap<>(); + if (query.getContext() != null) { + for (Map.Entry entry : query.getContext().entrySet()) { + if (entry.getValue() != null) { + subqueryContext.put(entry.getKey(), entry.getValue()); + } } + } + if (dataSource.getQuery().getContext() != null) { + subqueryContext.putAll(dataSource.getQuery().getContext()); + } + subqueryContext.put(GroupByQuery.CTX_KEY_SORT_BY_DIMS_FIRST, false); - final Sequence subqueryResult = mergeGroupByResults( - subquery, - resource, - runner, - context, - closer, - perQueryStats - ); + final GroupByQuery subquery; + try { + subquery = (GroupByQuery) dataSource.getQuery().withOverriddenContext(subqueryContext); + } + catch (ClassCastException e) { + throw new UnsupportedOperationException("Subqueries must be of type 'group by'"); + } - final Sequence finalizingResults = finalizeSubqueryResults(subqueryResult, subquery); + // TODO: Check if we need to do a fitting action here. + // closer.register(() -> groupByStatsProvider.closeQuery(subquery.context().getQueryResourceId())); + final QueryPlus subqueryPlus = queryPlus.withQuery(subquery); - if (query.getSubtotalsSpec() != null) { - return groupingEngine.processSubtotalsSpec( - query, - resource, - groupingEngine.processSubqueryResult( - subquery, - query, resource, - finalizingResults, - false, - perQueryStats - ), - perQueryStats - ); - } else { - return groupingEngine.applyPostProcessing( - groupingEngine.processSubqueryResult( - subquery, - query, - resource, - finalizingResults, - false, - perQueryStats - ), - query - ); - } + final Sequence subqueryResult = mergeGroupByResults( + subqueryPlus, + resource, + runner, + context, + closer + ); + + final Sequence finalizingResults = finalizeSubqueryResults(subqueryResult, subquery); + final Sequence processedSubqueryResults = groupingEngine.processSubqueryResult( + subqueryPlus, + queryPlus, + resource, + finalizingResults, + false + ); + if (query.getSubtotalsSpec() == null) { + return groupingEngine.applyPostProcessing(processedSubqueryResults, query); } else { - if (query.getSubtotalsSpec() != null) { - return groupingEngine.processSubtotalsSpec( - query, - resource, - groupingEngine.mergeResults(runner, query.withSubtotalsSpec(null), context), - perQueryStats - ); - } else { - return groupingEngine.applyPostProcessing(groupingEngine.mergeResults(runner, query, context), query); - } + return groupingEngine.processSubtotalsSpec(queryPlus, resource, processedSubqueryResults); } } private Sequence mergeResultsWithNestedQueryPushDown( - GroupByQuery query, + QueryPlus queryPlus, GroupByQueryResources resource, QueryRunner runner, - ResponseContext context, - GroupByStatsProvider.PerQueryStats perQueryStats + ResponseContext context ) { - Sequence pushDownQueryResults = groupingEngine.mergeResults(runner, query, context); + GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); + Sequence pushDownQueryResults = groupingEngine.mergeResults(runner, queryPlus, context); + final Sequence finalizedResults = finalizeSubqueryResults(pushDownQueryResults, query); - GroupByQuery rewrittenQuery = rewriteNestedQueryForPushDown(query); - return groupingEngine.applyPostProcessing( - groupingEngine.processSubqueryResult( - query, - rewrittenQuery, - resource, - finalizedResults, - true, - perQueryStats - ), - query + QueryPlus rewrittenQueryPlus = queryPlus.withQuery(rewriteNestedQueryForPushDown(query)); + + Sequence processedSubqueryResult = groupingEngine.processSubqueryResult( + queryPlus, + rewrittenQueryPlus, + resource, + finalizedResults, + true ); + + return groupingEngine.applyPostProcessing(processedSubqueryResult, query); } /** @@ -376,19 +341,14 @@ GroupByQuery rewriteNestedQueryForPushDown(GroupByQuery query) private Sequence finalizeSubqueryResults(Sequence subqueryResult, GroupByQuery subquery) { - final Sequence finalizingResults; if (subquery.context().isFinalize(false)) { - finalizingResults = new MappedSequence<>( + return new MappedSequence<>( subqueryResult, - makePreComputeManipulatorFn( - subquery, - MetricManipulatorFns.finalizing() - )::apply + makePreComputeManipulatorFn(subquery, MetricManipulatorFns.finalizing())::apply ); } else { - finalizingResults = subqueryResult; + return subqueryResult; } - return finalizingResults; } public static boolean isNestedQueryPushDown(GroupByQuery q) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResourcesReservationPool.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResourcesReservationPool.java index bd585dc7218c..40f1b4b10f4d 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResourcesReservationPool.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResourcesReservationPool.java @@ -115,11 +115,9 @@ public GroupByResourcesReservationPool( public void reserve( QueryResourceId queryResourceId, GroupByQuery groupByQuery, - boolean willMergeRunner, - GroupByStatsProvider.PerQueryStats perQueryStats + boolean willMergeRunner ) { - long startNs = System.nanoTime(); if (queryResourceId == null) { throw DruidException.defensive("Query resource id must be populated"); } @@ -151,8 +149,6 @@ public void reserve( // Resources have been allocated, spot has been reserved. The reference would ALWAYS refer to 'null'. Refer the // allocated resources from it reference.compareAndSet(null, resources); - - perQueryStats.mergeBufferAcquisitionTime(System.nanoTime() - startNs); } /** diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 3f85bbf63e1f..8e63f4864b12 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -336,10 +336,11 @@ public GroupByQuery prepareGroupByQuery(GroupByQuery query) */ public Sequence mergeResults( final QueryRunner baseRunner, - final GroupByQuery query, + final QueryPlus queryPlus, final ResponseContext responseContext ) { + GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); // Merge streams using ResultMergeQueryRunner, then apply postaggregators, then apply limit (which may // involve materialization) final ResultMergeQueryRunner mergingQueryRunner = new ResultMergeQueryRunner<>( @@ -356,7 +357,7 @@ public Sequence mergeResults( final int timestampResultFieldIndexInOriginalDimensions = hasTimestampResultField ? queryContext.getInt(GroupByQuery.CTX_TIMESTAMP_RESULT_FIELD_INDEX) : 0; final GroupByQuery newQuery = prepareGroupByQuery(query); - final Sequence mergedResults = mergingQueryRunner.run(QueryPlus.wrap(newQuery), responseContext); + final Sequence mergedResults = mergingQueryRunner.run(queryPlus.withQuery(newQuery), responseContext); // Apply postaggregators if this is the outermost mergeResults (CTX_KEY_OUTERMOST) and we are not executing a // pushed-down subquery (CTX_KEY_EXECUTING_NESTED_QUERY). @@ -584,18 +585,18 @@ public Sequence applyPostProcessing(Sequence results, Grou * @return results of the outer query */ public Sequence processSubqueryResult( - GroupByQuery subquery, - GroupByQuery query, + QueryPlus subqueryPlus, + QueryPlus queryPlus, GroupByQueryResources resource, Sequence subqueryResult, - boolean wasQueryPushedDown, - GroupByStatsProvider.PerQueryStats perQueryStats + boolean wasQueryPushedDown ) { // Keep a reference to resultSupplier outside the "try" so we can close it if something goes wrong // while creating the sequence. GroupByRowProcessor.ResultSupplier resultSupplier = null; + GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); try { GroupByQuery queryToRun; @@ -623,8 +624,7 @@ public Sequence processSubqueryResult( resource, spillMapper, processingConfig.getTmpDir(), - processingConfig.intermediateComputeSizeBytes(), - perQueryStats + processingConfig.intermediateComputeSizeBytes() ); final GroupByRowProcessor.ResultSupplier finalResultSupplier = resultSupplier; @@ -652,10 +652,9 @@ public Sequence processSubqueryResult( * @return results for each list of subtotals in the query, concatenated together */ public Sequence processSubtotalsSpec( - GroupByQuery query, + QueryPlus queryPlus, GroupByQueryResources resource, - Sequence queryResult, - GroupByStatsProvider.PerQueryStats perQueryStats + Sequence queryResult ) { // How it works? @@ -672,6 +671,7 @@ public Sequence processSubtotalsSpec( // while creating the sequence. GroupByRowProcessor.ResultSupplier resultSupplierOne = null; + GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); try { // baseSubtotalQuery is the original query with dimensions and aggregators rewritten to apply to the *results* // rather than *inputs* of that query. It has its virtual columns and dim filter removed, because those only diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index 9bc9c3235fc1..c1608c647d54 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Function; +import com.google.common.base.Preconditions; import com.google.common.base.Predicates; import com.google.common.base.Suppliers; import com.google.common.base.Throwables; @@ -57,6 +58,7 @@ import org.apache.druid.query.context.ResponseContext; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; +import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQueryResources; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; import org.apache.druid.query.groupby.GroupByStatsProvider; @@ -138,6 +140,8 @@ public Sequence run(final QueryPlus queryPlus, final Respo { final GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); + final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); // CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION is here because realtime servers use nested mergeRunners calls // (one for the entire query and one for each sink). We only want the outer call to actually do merging with a @@ -167,9 +171,6 @@ public Sequence run(final QueryPlus queryPlus, final Respo StringUtils.format("druid-groupBy-%s_%s", UUID.randomUUID(), query.getId()) ); - GroupByStatsProvider.PerQueryStats perQueryStats = - groupByStatsProvider.getPerQueryStatsContainer(query.context().getQueryResourceId()); - final int priority = queryContext.getPriority(); // Figure out timeoutAt time now, so we can apply the timeout to both the mergeBufferPool.take and the actual @@ -192,7 +193,7 @@ public CloseableGrouperIterator make() final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryStorageDirectory, querySpecificConfig.getMaxOnDiskStorage().getBytes(), - perQueryStats + groupByQueryMetrics ); final ReferenceCountingResourceHolder temporaryStorageHolder = @@ -213,7 +214,7 @@ public CloseableGrouperIterator make() Pair, Accumulator> pair = RowBasedGrouperHelper.createGrouperAccumulatorPair( - query, + queryPlus, null, config, processingConfig, @@ -226,8 +227,7 @@ public CloseableGrouperIterator make() priority, hasTimeout, timeoutAt, - mergeBufferSize, - perQueryStats + mergeBufferSize ); final Grouper grouper = pair.lhs; final Accumulator accumulator = pair.rhs; diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java index e2ca5c7e83b1..daa6f07e126e 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java @@ -29,10 +29,12 @@ import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.java.util.common.io.Closer; import org.apache.druid.query.DruidProcessingConfig; +import org.apache.druid.query.QueryPlus; import org.apache.druid.query.ResourceLimitExceededException; import org.apache.druid.query.dimension.DimensionSpec; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; +import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQueryResources; import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.ResultRow; @@ -86,18 +88,18 @@ private GroupByRowProcessor() * calls to the {@link ResultSupplier}. Make sure to close it when you're done. */ public static ResultSupplier process( - final GroupByQuery query, - final GroupByQuery subquery, + final QueryPlus queryPlus, + final QueryPlus subqueryPlus, final Sequence rows, final GroupByQueryConfig config, final DruidProcessingConfig processingConfig, final GroupByQueryResources resource, final ObjectMapper spillMapper, final String processingTmpDir, - final int mergeBufferSize, - final GroupByStatsProvider.PerQueryStats perQueryStats + final int mergeBufferSize ) { + GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); final Closer closeOnExit = Closer.create(); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); @@ -109,7 +111,7 @@ public static ResultSupplier process( final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryStorageDirectory, querySpecificConfig.getMaxOnDiskStorage().getBytes(), - perQueryStats + (GroupByQueryMetrics) subqueryPlus.getQueryMetrics() ); closeOnExit.register(temporaryStorage); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java index 23bc2706a2da..edc315a0e3eb 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java @@ -25,7 +25,7 @@ import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.logger.Logger; -import org.apache.druid.query.groupby.GroupByStatsProvider; +import org.apache.druid.query.groupby.GroupByQueryMetrics; import java.io.Closeable; import java.io.File; @@ -48,7 +48,7 @@ public class LimitedTemporaryStorage implements Closeable { private static final Logger log = new Logger(LimitedTemporaryStorage.class); - private final GroupByStatsProvider.PerQueryStats perQueryStatsContainer; + private final GroupByQueryMetrics groupByQueryMetrics; private final File storageDirectory; private final long maxBytesUsed; @@ -63,12 +63,12 @@ public class LimitedTemporaryStorage implements Closeable public LimitedTemporaryStorage( File storageDirectory, long maxBytesUsed, - GroupByStatsProvider.PerQueryStats perQueryStatsContainer + GroupByQueryMetrics groupByQueryMetrics ) { this.storageDirectory = storageDirectory; this.maxBytesUsed = maxBytesUsed; - this.perQueryStatsContainer = perQueryStatsContainer; + this.groupByQueryMetrics = groupByQueryMetrics; } /** @@ -143,8 +143,7 @@ public void close() } closed = true; - perQueryStatsContainer.spilledBytes(bytesUsed.get()); - + groupByQueryMetrics.bytesSpilledToStorage(bytesUsed.get()); bytesUsed.set(0); for (File file : ImmutableSet.copyOf(files)) { diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java index 50e01b0f60c7..2c2ca13cc1c0 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java @@ -36,6 +36,8 @@ import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.CountAggregatorFactory; import org.apache.druid.query.dimension.DimensionSpec; +import org.apache.druid.query.groupby.DefaultGroupByQueryMetrics; +import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.epinephelinae.Grouper.BufferComparator; import org.apache.druid.query.groupby.epinephelinae.Grouper.Entry; @@ -148,11 +150,11 @@ public ByteBuffer get() @Test() public void testAggregate() throws InterruptedException, ExecutionException, IOException { - GroupByStatsProvider.PerQueryStats perQueryStats = new GroupByStatsProvider.PerQueryStats(); + GroupByQueryMetrics groupByQueryMetrics = new DefaultGroupByQueryMetrics(); final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryFolder.newFolder(), 1024 * 1024, - perQueryStats + groupByQueryMetrics ); final ListeningExecutorService service = MoreExecutors.listeningDecorator(exec); try { @@ -177,8 +179,7 @@ public void testAggregate() throws InterruptedException, ExecutionException, IOE 0, 4, parallelCombineThreads, - mergeThreadLocal, - perQueryStats + mergeThreadLocal ); closer.register(grouper); grouper.init(); @@ -231,7 +232,7 @@ public void testGrouperTimeout() throws Exception return; } - GroupByStatsProvider.PerQueryStats perQueryStats = new GroupByStatsProvider.PerQueryStats(); + GroupByQueryMetrics groupByQueryMetrics = new DefaultGroupByQueryMetrics(); ListeningExecutorService service = MoreExecutors.listeningDecorator(exec); try { final ConcurrentGrouper grouper = new ConcurrentGrouper<>( @@ -244,7 +245,7 @@ public void testGrouperTimeout() throws Exception 1024, 0.7f, 1, - new LimitedTemporaryStorage(temporaryFolder.newFolder(), 1024 * 1024, perQueryStats), + new LimitedTemporaryStorage(temporaryFolder.newFolder(), 1024 * 1024, groupByQueryMetrics), new DefaultObjectMapper(), concurrencyHint, null, From 7c8f2c4865476d4d2bc999129c6a8c9255c0075a Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 27 Nov 2025 16:35:52 +0800 Subject: [PATCH 04/39] Time to remove PerQueryStats P3: Removed in Groupers, GroupByRowProcessor --- .../druid/query/groupby/GroupingEngine.java | 41 +++++++------ .../epinephelinae/ConcurrentGrouper.java | 61 +++++++++++-------- .../GroupByMergingQueryRunner.java | 7 ++- .../epinephelinae/GroupByRowProcessor.java | 8 +-- .../query/groupby/epinephelinae/Grouper.java | 6 ++ .../epinephelinae/RowBasedGrouperHelper.java | 32 +++++----- .../epinephelinae/SpillingGrouper.java | 17 ++++-- .../epinephelinae/ConcurrentGrouperTest.java | 3 +- 8 files changed, 103 insertions(+), 72 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 8e63f4864b12..c522604629b9 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -615,9 +615,11 @@ public Sequence processSubqueryResult( queryToRun = queryToRun.withLimitSpec(((DefaultLimitSpec) queryToRun.getLimitSpec()).withOffsetToLimit()); } + final QueryPlus queryPlusToRun = queryPlus.withQuery(queryToRun); + resultSupplier = GroupByRowProcessor.process( - queryToRun, - wasQueryPushedDown ? queryToRun : subquery, + queryPlusToRun, + wasQueryPushedDown ? queryPlusToRun : subqueryPlus, subqueryResult, configSupplier.get(), processingConfig, @@ -630,8 +632,8 @@ public Sequence processSubqueryResult( final GroupByRowProcessor.ResultSupplier finalResultSupplier = resultSupplier; return Sequences.withBaggage( mergeResults( - (queryPlus, responseContext) -> finalResultSupplier.results(null), - query, + (qp, responseContext) -> finalResultSupplier.results(null), + queryPlus, ResponseContext.createEmpty() ), finalResultSupplier @@ -697,17 +699,18 @@ public Sequence processSubtotalsSpec( // timestampResult optimization is not for subtotal scenario, so disable it .withOverriddenContext(ImmutableMap.of(GroupByQuery.CTX_TIMESTAMP_RESULT_FIELD, "")); + QueryPlus baseSubtotalQueryPlus = queryPlus.withQuery(baseSubtotalQuery); + resultSupplierOne = GroupByRowProcessor.process( - baseSubtotalQuery, - baseSubtotalQuery, + baseSubtotalQueryPlus, + baseSubtotalQueryPlus, queryResult, configSupplier.get(), processingConfig, resource, spillMapper, processingConfig.getTmpDir(), - processingConfig.intermediateComputeSizeBytes(), - perQueryStats + processingConfig.intermediateComputeSizeBytes() ); List queryDimNamesInOrder = baseSubtotalQuery.getDimensionNamesInOrder(); @@ -743,15 +746,16 @@ public Sequence processSubtotalsSpec( subtotalQueryLimitSpec = baseSubtotalQuery.getLimitSpec().filterColumns(columns); } - GroupByQuery subtotalQuery = baseSubtotalQuery - .withLimitSpec(subtotalQueryLimitSpec); + QueryPlus subtotalQueryPlus = baseSubtotalQueryPlus.withQuery( + baseSubtotalQuery + .withLimitSpec(subtotalQueryLimitSpec)); final GroupByRowProcessor.ResultSupplier resultSupplierOneFinal = resultSupplierOne; if (Utils.isPrefix(subtotalSpec, queryDimNamesInOrder)) { // Since subtotalSpec is a prefix of base query dimensions, so results from base query are also sorted // by subtotalSpec as needed by stream merging. subtotalsResults.add( - processSubtotalsResultAndOptionallyClose(() -> resultSupplierOneFinal, subTotalDimensionSpec, subtotalQuery, false) + processSubtotalsResultAndOptionallyClose(() -> resultSupplierOneFinal, subTotalDimensionSpec, subtotalQueryPlus, false) ); } else { // Since subtotalSpec is not a prefix of base query dimensions, so results from base query are not sorted @@ -761,20 +765,19 @@ public Sequence processSubtotalsSpec( // Also note, we can't create the ResultSupplier eagerly here or as we don't want to eagerly allocate // merge buffers for processing subtotal. Supplier resultSupplierTwo = () -> GroupByRowProcessor.process( - baseSubtotalQuery, - subtotalQuery, + baseSubtotalQueryPlus, + subtotalQueryPlus, resultSupplierOneFinal.results(subTotalDimensionSpec), configSupplier.get(), processingConfig, resource, spillMapper, processingConfig.getTmpDir(), - processingConfig.intermediateComputeSizeBytes(), - perQueryStats + processingConfig.intermediateComputeSizeBytes() ); subtotalsResults.add( - processSubtotalsResultAndOptionallyClose(resultSupplierTwo, subTotalDimensionSpec, subtotalQuery, true) + processSubtotalsResultAndOptionallyClose(resultSupplierTwo, subTotalDimensionSpec, subtotalQueryPlus, true) ); } } @@ -792,7 +795,7 @@ public Sequence processSubtotalsSpec( private Sequence processSubtotalsResultAndOptionallyClose( Supplier baseResultsSupplier, List dimsToInclude, - GroupByQuery subtotalQuery, + QueryPlus subtotalQueryPlus, boolean closeOnSequenceRead ) { @@ -810,7 +813,7 @@ private Sequence processSubtotalsResultAndOptionallyClose( : () -> {} ) ), - subtotalQuery, + subtotalQueryPlus, ResponseContext.createEmpty() ); } @@ -849,7 +852,7 @@ private void moveOrReplicateTimestampInRow( private Set getAggregatorAndPostAggregatorNames(GroupByQuery query) { - Set aggsAndPostAggs = new HashSet(); + Set aggsAndPostAggs = new HashSet<>(); if (query.getAggregatorSpecs() != null) { for (AggregatorFactory af : query.getAggregatorSpecs()) { aggsAndPostAggs.add(af.getName()); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java index 8242c9d8cf5c..c039f5dbbcf7 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java @@ -38,9 +38,10 @@ import org.apache.druid.query.QueryTimeoutException; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.groupby.GroupByQueryConfig; -import org.apache.druid.query.groupby.GroupByStatsProvider; +import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; import org.apache.druid.segment.ColumnSelectorFactory; +import org.jetbrains.annotations.NotNull; import javax.annotation.Nullable; import java.nio.ByteBuffer; @@ -95,7 +96,7 @@ public class ConcurrentGrouper implements Grouper @Nullable private final ParallelCombiner parallelCombiner; private final boolean mergeThreadLocal; - private final GroupByStatsProvider.PerQueryStats perQueryStats; + private final GroupByQueryMetrics groupByQueryMetrics; private volatile boolean initialized = false; @@ -116,7 +117,7 @@ public ConcurrentGrouper( final int priority, final boolean hasQueryTimeout, final long queryTimeoutAt, - final GroupByStatsProvider.PerQueryStats perQueryStats + final GroupByQueryMetrics groupByQueryMetrics ) { this( @@ -141,7 +142,7 @@ public ConcurrentGrouper( groupByQueryConfig.getIntermediateCombineDegree(), groupByQueryConfig.getNumParallelCombineThreads(), groupByQueryConfig.isMergeThreadLocal(), - perQueryStats + groupByQueryMetrics ); } @@ -167,7 +168,7 @@ public ConcurrentGrouper( final int intermediateCombineDegree, final int numParallelCombineThreads, final boolean mergeThreadLocal, - final GroupByStatsProvider.PerQueryStats perQueryStats + final GroupByQueryMetrics groupByQueryMetrics ) { Preconditions.checkArgument(concurrencyHint > 0, "concurrencyHint > 0"); @@ -217,7 +218,7 @@ public ConcurrentGrouper( } this.mergeThreadLocal = mergeThreadLocal; - this.perQueryStats = perQueryStats; + this.groupByQueryMetrics = groupByQueryMetrics; } @Override @@ -231,23 +232,7 @@ public void init() for (int i = 0; i < concurrencyHint; i++) { final ByteBuffer slice = Groupers.getSlice(buffer, sliceSize, i); - final SpillingGrouper grouper = new SpillingGrouper<>( - Suppliers.ofInstance(slice), - keySerdeFactory, - columnSelectorFactory, - aggregatorFactories, - bufferGrouperMaxSize, - bufferGrouperMaxLoadFactor, - bufferGrouperInitialBuckets, - temporaryStorage, - spillMapper, - false, - limitSpec, - sortHasNonGroupingFields, - sliceSize, - perQueryStats - ); - grouper.init(); + final SpillingGrouper grouper = generateSpillingGrouperWithBufferSlice(slice, sliceSize); groupers.add(grouper); if (mergeThreadLocal) { @@ -261,6 +246,28 @@ public void init() } } + private @NotNull SpillingGrouper generateSpillingGrouperWithBufferSlice(ByteBuffer slice, int sliceSize) + { + final SpillingGrouper grouper = new SpillingGrouper<>( + Suppliers.ofInstance(slice), + keySerdeFactory, + columnSelectorFactory, + aggregatorFactories, + bufferGrouperMaxSize, + bufferGrouperMaxLoadFactor, + bufferGrouperInitialBuckets, + temporaryStorage, + spillMapper, + false, + limitSpec, + sortHasNonGroupingFields, + sliceSize, + groupByQueryMetrics + ); + grouper.init(); + return grouper; + } + @Override public boolean isInitialized() { @@ -491,12 +498,18 @@ private List tryMergeDictionary() return ImmutableList.copyOf(mergedDictionary); } + @Override + public void updateGroupByQueryMetrics() + { + groupers.forEach(SpillingGrouper::updateGroupByQueryMetrics); + } + @Override public void close() { if (!closed) { closed = true; - groupers.forEach(Grouper::close); + groupers.forEach(SpillingGrouper::close); } } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index c1608c647d54..2f9248ff73cf 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -106,6 +106,7 @@ public class GroupByMergingQueryRunner implements QueryRunner private final ObjectMapper spillMapper; private final String processingTmpDir; private final int mergeBufferSize; + // TODO: Apply aggregateStats private final GroupByStatsProvider groupByStatsProvider; public GroupByMergingQueryRunner( @@ -306,9 +307,9 @@ public AggregateResult call() waitForFutureCompletion(query, futures, hasTimeout, timeoutAt - System.currentTimeMillis()); } - // grouper.updateGroupByQueryMetrics(); - // groupByStatsProvider.aggregateStats(queryMetrics); -// queryMetrics.reportGroupByStats(); + grouper.updateGroupByQueryMetrics(); + groupByStatsProvider.aggregateStats(groupByQueryMetrics); + groupByQueryMetrics.reportGroupByStats(); return RowBasedGrouperHelper.makeGrouperIterator( grouper, diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java index daa6f07e126e..19e7f505377f 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java @@ -36,7 +36,6 @@ import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQueryResources; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.epinephelinae.RowBasedGrouperHelper.RowBasedKey; @@ -117,8 +116,8 @@ public static ResultSupplier process( closeOnExit.register(temporaryStorage); Pair, Accumulator> pair = RowBasedGrouperHelper.createGrouperAccumulatorPair( - query, - subquery, + queryPlus, + subqueryPlus, querySpecificConfig, processingConfig, new Supplier<>() @@ -133,8 +132,7 @@ public ByteBuffer get() }, temporaryStorage, spillMapper, - mergeBufferSize, - perQueryStats + mergeBufferSize ); final Grouper grouper = pair.lhs; final Accumulator accumulator = pair.rhs; diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java index 9ab8f738791e..d2763cb94877 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java @@ -93,6 +93,12 @@ default ToIntFunction hashFunction() return Groupers::hashObject; } + /** + * Update the results of the GroupByQueryMetrics. + * Currently only used by {@link ConcurrentGrouper} and {@link SpillingGrouper} + */ + default void updateGroupByQueryMetrics() {} + /** * Close the grouper and release associated resources. */ diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java index 6ea439d7a90b..9f4089e38c6b 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java @@ -48,6 +48,7 @@ import org.apache.druid.query.ColumnSelectorPlus; import org.apache.druid.query.DimensionComparisonUtils; import org.apache.druid.query.DruidProcessingConfig; +import org.apache.druid.query.QueryPlus; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.GroupingAggregatorFactory; import org.apache.druid.query.dimension.ColumnSelectorStrategy; @@ -57,7 +58,7 @@ import org.apache.druid.query.filter.ValueMatcher; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; -import org.apache.druid.query.groupby.GroupByStatsProvider; +import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.epinephelinae.Grouper.BufferComparator; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; @@ -126,15 +127,14 @@ private RowBasedGrouperHelper() * Create a single-threaded grouper and accumulator. */ public static Pair, Accumulator> createGrouperAccumulatorPair( - final GroupByQuery query, - @Nullable final GroupByQuery subquery, + final QueryPlus query, + @Nullable final QueryPlus subquery, final GroupByQueryConfig config, final DruidProcessingConfig processingConfig, final Supplier bufferSupplier, final LimitedTemporaryStorage temporaryStorage, final ObjectMapper spillMapper, - final int mergeBufferSize, - final GroupByStatsProvider.PerQueryStats perQueryStats + final int mergeBufferSize ) { return createGrouperAccumulatorPair( @@ -151,8 +151,7 @@ public static Pair, Accumulator UNKNOWN_THREAD_PRIORITY, false, UNKNOWN_TIMEOUT, - mergeBufferSize, - perQueryStats + mergeBufferSize ); } @@ -188,8 +187,8 @@ public static Pair, Accumulator * @param mergeBufferSize size of the merge buffers from "bufferSupplier" */ public static Pair, Accumulator> createGrouperAccumulatorPair( - final GroupByQuery query, - @Nullable final GroupByQuery subquery, + final QueryPlus queryPlus, + @Nullable final QueryPlus subqueryPlus, final GroupByQueryConfig config, final DruidProcessingConfig processingConfig, final Supplier bufferSupplier, @@ -201,8 +200,7 @@ public static Pair, Accumulator final int priority, final boolean hasQueryTimeout, final long queryTimeoutAt, - final int mergeBufferSize, - final GroupByStatsProvider.PerQueryStats perQueryStats + final int mergeBufferSize ) { // concurrencyHint >= 1 for concurrent groupers, -1 for single-threaded @@ -213,8 +211,9 @@ public static Pair, Accumulator } // See method-level javadoc; we go into combining mode if there is no subquery. - final boolean combining = subquery == null; + final boolean combining = subqueryPlus == null; + GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); final List valueTypes = DimensionHandlerUtils.getValueTypesFromDimensionSpecs(query.getDimensions()); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); @@ -223,7 +222,7 @@ public static Pair, Accumulator final ThreadLocal columnSelectorRow = new ThreadLocal<>(); ColumnSelectorFactory columnSelectorFactory = createResultRowBasedColumnSelectorFactory( - combining ? query : subquery, + combining ? query : (GroupByQuery) subqueryPlus.getQuery(), columnSelectorRow::get, RowSignature.Finalization.UNKNOWN ); @@ -266,6 +265,9 @@ public static Pair, Accumulator limitSpec ); + final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + Preconditions.checkNotNull(groupByQueryMetrics, "GroupByQueryMetrics"); + final Grouper grouper; if (concurrencyHint == -1) { grouper = new SpillingGrouper<>( @@ -282,7 +284,7 @@ public static Pair, Accumulator limitSpec, sortHasNonGroupingFields, mergeBufferSize, - perQueryStats + groupByQueryMetrics ); } else { final Grouper.KeySerdeFactory combineKeySerdeFactory = new RowBasedKeySerdeFactory( @@ -312,7 +314,7 @@ public static Pair, Accumulator priority, hasQueryTimeout, queryTimeoutAt, - perQueryStats + groupByQueryMetrics ); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java index fadcfa02c95d..050e3921f2fd 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java @@ -37,7 +37,7 @@ import org.apache.druid.query.BaseQuery; import org.apache.druid.query.aggregation.AggregatorAdapters; import org.apache.druid.query.aggregation.AggregatorFactory; -import org.apache.druid.query.groupby.GroupByStatsProvider; +import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; import org.apache.druid.segment.ColumnSelectorFactory; @@ -75,7 +75,7 @@ public class SpillingGrouper implements Grouper private final AggregatorFactory[] aggregatorFactories; private final Comparator> keyObjComparator; private final Comparator> defaultOrderKeyObjComparator; - private final GroupByStatsProvider.PerQueryStats perQueryStats; + private final GroupByQueryMetrics groupByQueryMetrics; private final List files = new ArrayList<>(); private final List dictionaryFiles = new ArrayList<>(); @@ -98,7 +98,7 @@ public SpillingGrouper( final DefaultLimitSpec limitSpec, final boolean sortHasNonGroupingFields, final int mergeBufferSize, - final GroupByStatsProvider.PerQueryStats perQueryStats + final GroupByQueryMetrics groupByQueryMetrics ) { this.keySerde = keySerdeFactory.factorize(); @@ -158,7 +158,7 @@ public SpillingGrouper( this.spillMapper = keySerde.decorateObjectMapper(spillMapper); this.spillingAllowed = spillingAllowed; this.sortHasNonGroupingFields = sortHasNonGroupingFields; - this.perQueryStats = perQueryStats; + this.groupByQueryMetrics = groupByQueryMetrics; } @Override @@ -215,10 +215,17 @@ public void reset() deleteFiles(); } + @Override + public void updateGroupByQueryMetrics() { + // TODO: If we do not want groupByQueryMetrics to do addition, simply return a Map, + // then either the ConcurrentGrouper will aggregate the results into a accumulated map, + // or the GroupByQueryMetrics will provide a means to report GroupByMetrics via a map. + groupByQueryMetrics.mergeDictionarySize(keySerde.getDictionarySize()); + } + @Override public void close() { - perQueryStats.dictionarySize(keySerde.getDictionarySize()); grouper.close(); keySerde.reset(); deleteFiles(); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java index 2c2ca13cc1c0..7ca630c5e937 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java @@ -179,7 +179,8 @@ public void testAggregate() throws InterruptedException, ExecutionException, IOE 0, 4, parallelCombineThreads, - mergeThreadLocal + mergeThreadLocal, + new DefaultGroupByQueryMetrics() ); closer.register(grouper); grouper.init(); From 4ce8feccd5d384907a08e08e81520861803c3a17 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 27 Nov 2025 17:14:21 +0800 Subject: [PATCH 05/39] GroupByRowProcessor Helps --- .../groupby/epinephelinae/GroupByMergingQueryRunner.java | 1 - .../query/groupby/epinephelinae/GroupByRowProcessor.java | 9 ++++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index 2f9248ff73cf..551287e2121a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -106,7 +106,6 @@ public class GroupByMergingQueryRunner implements QueryRunner private final ObjectMapper spillMapper; private final String processingTmpDir; private final int mergeBufferSize; - // TODO: Apply aggregateStats private final GroupByStatsProvider groupByStatsProvider; public GroupByMergingQueryRunner( diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java index 19e7f505377f..8d24c9a1def1 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java @@ -20,6 +20,7 @@ package org.apache.druid.query.groupby.epinephelinae; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import org.apache.druid.collections.ResourceHolder; import org.apache.druid.java.util.common.Pair; @@ -99,6 +100,8 @@ public static ResultSupplier process( ) { GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); + final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); final Closer closeOnExit = Closer.create(); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); @@ -110,7 +113,7 @@ public static ResultSupplier process( final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryStorageDirectory, querySpecificConfig.getMaxOnDiskStorage().getBytes(), - (GroupByQueryMetrics) subqueryPlus.getQueryMetrics() + groupByQueryMetrics ); closeOnExit.register(temporaryStorage); @@ -135,6 +138,7 @@ public ByteBuffer get() mergeBufferSize ); final Grouper grouper = pair.lhs; + Preconditions.checkNotNull(grouper); final Accumulator accumulator = pair.rhs; closeOnExit.register(grouper); @@ -144,6 +148,9 @@ public ByteBuffer get() throw new ResourceLimitExceededException(retVal.getReason()); } + grouper.updateGroupByQueryMetrics(); + groupByQueryMetrics.reportGroupByStats(); + return new ResultSupplier() { @Override From cc105dc6a950cd70383b495e8b9835c6cacc85cd Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 27 Nov 2025 17:21:39 +0800 Subject: [PATCH 06/39] RowBasedGrouperHelper --- .../query/groupby/GroupByStatsProvider.java | 16 +------- .../epinephelinae/RowBasedGrouperHelper.java | 3 +- .../groupby/GroupByStatsProviderTest.java | 39 +------------------ 3 files changed, 4 insertions(+), 54 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java index db97dac91012..b4ded394a9e1 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java @@ -20,7 +20,6 @@ package org.apache.druid.query.groupby; import org.apache.druid.guice.LazySingleton; -import org.apache.druid.query.QueryResourceId; import javax.annotation.Nullable; @@ -37,24 +36,11 @@ public GroupByStatsProvider() this.aggregateStatsContainer = AggregateStats.EMPTY_STATS; } - public void aggregateStats(@Nullable GroupByQueryMetrics groupByQueryMetrics) + public void aggregateStats(GroupByQueryMetrics groupByQueryMetrics) { - if (groupByQueryMetrics == null) { - return; - } - aggregateStatsContainer.addQueryStats(groupByQueryMetrics); } - public synchronized void closeQuery(QueryResourceId resourceId) - { - if (resourceId == null || !perQueryStats.containsKey(resourceId)) { - return; - } - PerQueryStats container = perQueryStats.remove(resourceId); - aggregateStatsContainer.addQueryStats(container); - } - public synchronized AggregateStats getStatsSince() { return aggregateStatsContainer.reset(); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java index 9f4089e38c6b..5346304a5eaf 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java @@ -212,6 +212,7 @@ public static Pair, Accumulator // See method-level javadoc; we go into combining mode if there is no subquery. final boolean combining = subqueryPlus == null; + GroupByQuery subquery = combining ? null : (GroupByQuery) subqueryPlus.getQuery(); GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); final List valueTypes = DimensionHandlerUtils.getValueTypesFromDimensionSpecs(query.getDimensions()); @@ -222,7 +223,7 @@ public static Pair, Accumulator final ThreadLocal columnSelectorRow = new ThreadLocal<>(); ColumnSelectorFactory columnSelectorFactory = createResultRowBasedColumnSelectorFactory( - combining ? query : (GroupByQuery) subqueryPlus.getQuery(), + combining ? query : subquery, columnSelectorRow::get, RowSignature.Finalization.UNKNOWN ); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java index 565a5ab97bc3..37f5a990b60d 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java @@ -19,7 +19,6 @@ package org.apache.druid.query.groupby; -import org.apache.druid.query.QueryResourceId; import org.junit.Assert; import org.junit.Test; @@ -29,42 +28,6 @@ public class GroupByStatsProviderTest public void testMetricCollection() { GroupByStatsProvider statsProvider = new GroupByStatsProvider(); - - QueryResourceId id1 = new QueryResourceId("q1"); - GroupByStatsProvider.PerQueryStats stats1 = statsProvider.getPerQueryStatsContainer(id1); - - stats1.mergeBufferAcquisitionTime(300); - stats1.mergeBufferAcquisitionTime(400); - stats1.spilledBytes(200); - stats1.spilledBytes(400); - stats1.dictionarySize(100); - stats1.dictionarySize(200); - - QueryResourceId id2 = new QueryResourceId("q2"); - GroupByStatsProvider.PerQueryStats stats2 = statsProvider.getPerQueryStatsContainer(id2); - - stats2.mergeBufferAcquisitionTime(500); - stats2.mergeBufferAcquisitionTime(600); - stats2.spilledBytes(400); - stats2.spilledBytes(600); - stats2.dictionarySize(300); - stats2.dictionarySize(400); - - GroupByStatsProvider.AggregateStats aggregateStats = statsProvider.getStatsSince(); - Assert.assertEquals(0L, aggregateStats.getMergeBufferQueries()); - Assert.assertEquals(0L, aggregateStats.getMergeBufferAcquisitionTimeNs()); - Assert.assertEquals(0L, aggregateStats.getSpilledQueries()); - Assert.assertEquals(0L, aggregateStats.getSpilledBytes()); - Assert.assertEquals(0L, aggregateStats.getMergeDictionarySize()); - - statsProvider.closeQuery(id1); - statsProvider.closeQuery(id2); - - aggregateStats = statsProvider.getStatsSince(); - Assert.assertEquals(2, aggregateStats.getMergeBufferQueries()); - Assert.assertEquals(1800L, aggregateStats.getMergeBufferAcquisitionTimeNs()); - Assert.assertEquals(2L, aggregateStats.getSpilledQueries()); - Assert.assertEquals(1600L, aggregateStats.getSpilledBytes()); - Assert.assertEquals(1000L, aggregateStats.getMergeDictionarySize()); + Assert.assertEquals(1,2 - 1); } } From 515a1aa0d4b53e4d3d00a2eab7903bf17f62bf76 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 27 Nov 2025 18:31:28 +0800 Subject: [PATCH 07/39] Tests --- .../GroupByDeserializationBenchmark.java | 3 +- .../query/groupby/GroupByStatsProvider.java | 17 +++++- .../GroupByQueryQueryToolChestTest.java | 1 - .../query/groupby/GroupByQueryRunnerTest.java | 3 +- .../GroupByResourcesReservationPoolTest.java | 18 ++---- .../groupby/GroupByStatsProviderTest.java | 42 +++++++++++++- .../groupby/NestedQueryPushDownTest.java | 2 +- .../epinephelinae/ConcurrentGrouperTest.java | 2 +- .../segment/CursorFactoryProjectionTest.java | 55 +++++++++++++++++-- 9 files changed, 114 insertions(+), 29 deletions(-) diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java index 95e0f1b19aac..cfb2da3fd228 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java @@ -113,8 +113,7 @@ public boolean isIntermediateResultAsMapCompat() } }, null, - null, - new GroupByStatsProvider() + null ); decoratedMapper = groupByQueryQueryToolChest.decorateObjectMapper(undecoratedMapper, sqlQuery); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java index b4ded394a9e1..eeb7020060d0 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java @@ -21,10 +21,11 @@ import org.apache.druid.guice.LazySingleton; -import javax.annotation.Nullable; - /** - * Metrics collector for groupBy queries like spilled bytes, merge buffer acquistion time, dictionary size. + * Aggregates {@link GroupByQueryMetrics} emitted by in-flight groupBy queries and exposes a snapshot that can be + * periodically consumed by {@link org.apache.druid.server.metrics.GroupByStatsMonitor}. The provider keeps track of + * aggregate counters such as merge-buffer acquisition time, spilled bytes, and the dictionary sizes used while + * merging results. */ @LazySingleton public class GroupByStatsProvider @@ -36,6 +37,13 @@ public GroupByStatsProvider() this.aggregateStatsContainer = AggregateStats.EMPTY_STATS; } + /** + * Adds the stats reported by a single query execution to the shared accumulator. Callers are expected to provide + * the {@link GroupByQueryMetrics} associated with the query once all relevant numbers have been recorded on the + * metrics instance. + * + * @param groupByQueryMetrics the query metrics to merge into the aggregate view + */ public void aggregateStats(GroupByQueryMetrics groupByQueryMetrics) { aggregateStatsContainer.addQueryStats(groupByQueryMetrics); @@ -46,6 +54,9 @@ public synchronized AggregateStats getStatsSince() return aggregateStatsContainer.reset(); } + /** + * Immutable snapshot of the aggregated groupBy metrics captured between two {@link #getStatsSince()} calls. + */ public static class AggregateStats { private long mergeBufferQueries; diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java index 80731a244c9a..258c06a06874 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java @@ -799,7 +799,6 @@ public boolean isIntermediateResultAsMapCompat() } }, null, - null, null ); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java index f4e4d1dbd247..fe5b4dbb2daa 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java @@ -405,8 +405,7 @@ public static GroupByQueryRunnerFactory makeQueryRunnerFactory( groupingEngine, () -> config, DefaultGroupByQueryMetricsFactory.instance(), - groupByResourcesReservationPool, - statsProvider + groupByResourcesReservationPool ); return new GroupByQueryRunnerFactory(groupingEngine, toolChest, bufferPools.getProcessingPool()); } diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByResourcesReservationPoolTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByResourcesReservationPoolTest.java index 85c81151dd3a..67af4d758130 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByResourcesReservationPoolTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByResourcesReservationPoolTest.java @@ -129,8 +129,7 @@ public boolean equals(Object o) groupByResourcesReservationPool.reserve( queryResourceId1, QUERY, - true, - new GroupByStatsProvider.PerQueryStats() + true ); reserveCalledByFirstThread.countDown(); try { @@ -173,8 +172,7 @@ public boolean equals(Object o) groupByResourcesReservationPool.reserve( queryResourceId2, QUERY, - true, - new GroupByStatsProvider.PerQueryStats() + true ); threadsCompleted.countDown(); }); @@ -209,8 +207,7 @@ public void testMultipleSimultaneousAllocationAttemptsFail() groupByResourcesReservationPool.reserve( queryResourceId, QUERY, - true, - new GroupByStatsProvider.PerQueryStats() + true ); Assert.assertThrows( @@ -218,8 +215,7 @@ public void testMultipleSimultaneousAllocationAttemptsFail() () -> groupByResourcesReservationPool.reserve( queryResourceId, QUERY, - true, - new GroupByStatsProvider.PerQueryStats() + true ) ); } @@ -235,8 +231,7 @@ public void testMultipleSequentialAllocationAttemptsSucceed() groupByResourcesReservationPool.reserve( queryResourceId, QUERY, - true, - new GroupByStatsProvider.PerQueryStats() + true ); GroupByQueryResources oldResources = groupByResourcesReservationPool.fetch(queryResourceId); @@ -247,8 +242,7 @@ public void testMultipleSequentialAllocationAttemptsSucceed() groupByResourcesReservationPool.reserve( queryResourceId, QUERY, - true, - new GroupByStatsProvider.PerQueryStats() + true ); GroupByQueryResources newResources = groupByResourcesReservationPool.fetch(queryResourceId); Assert.assertNotNull(newResources); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java index 37f5a990b60d..a8600e5b1cb9 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByStatsProviderTest.java @@ -25,9 +25,47 @@ public class GroupByStatsProviderTest { @Test - public void testMetricCollection() + public void testAggregateStatsFromQueryMetrics() { GroupByStatsProvider statsProvider = new GroupByStatsProvider(); - Assert.assertEquals(1,2 - 1); + + DefaultGroupByQueryMetrics metricsWithAllStats = new DefaultGroupByQueryMetrics(); + metricsWithAllStats.mergeBufferAcquisitionTime(100L); + metricsWithAllStats.bytesSpilledToStorage(2_048L); + metricsWithAllStats.mergeDictionarySize(10L); + + DefaultGroupByQueryMetrics metricsWithPartialStats = new DefaultGroupByQueryMetrics(); + metricsWithPartialStats.bytesSpilledToStorage(1_024L); + metricsWithPartialStats.mergeDictionarySize(5L); + + statsProvider.aggregateStats(metricsWithAllStats); + statsProvider.aggregateStats(metricsWithPartialStats); + + GroupByStatsProvider.AggregateStats stats = statsProvider.getStatsSince(); + + Assert.assertEquals(1, stats.getMergeBufferQueries()); + Assert.assertEquals(100L, stats.getMergeBufferAcquisitionTimeNs()); + Assert.assertEquals(2, stats.getSpilledQueries()); + Assert.assertEquals(3_072L, stats.getSpilledBytes()); + Assert.assertEquals(15L, stats.getMergeDictionarySize()); + } + + @Test + public void testGetStatsSinceResetsCounters() + { + GroupByStatsProvider statsProvider = new GroupByStatsProvider(); + + DefaultGroupByQueryMetrics metrics = new DefaultGroupByQueryMetrics(); + metrics.bytesSpilledToStorage(512L); + metrics.mergeDictionarySize(7L); + statsProvider.aggregateStats(metrics); + + Assert.assertEquals(512L, statsProvider.getStatsSince().getSpilledBytes()); + + GroupByStatsProvider.AggregateStats reset = statsProvider.getStatsSince(); + Assert.assertEquals(0L, reset.getMergeBufferQueries()); + Assert.assertEquals(0L, reset.getSpilledQueries()); + Assert.assertEquals(0L, reset.getSpilledBytes()); + Assert.assertEquals(0L, reset.getMergeDictionarySize()); } } diff --git a/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java b/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java index 5b89ca1c1415..9ca463ccdaa3 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java @@ -763,7 +763,7 @@ private Sequence runNestedQueryWithForcePushDown(GroupByQuery nestedQ )); Sequence pushDownQueryResults = groupingEngine.mergeResults( queryRunnerForSegments, - (GroupByQuery) GroupByQueryRunnerTestHelper.populateResourceId(queryWithPushDownDisabled), + QueryPlus.wrap(GroupByQueryRunnerTestHelper.populateResourceId(queryWithPushDownDisabled)), context ); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java index 7ca630c5e937..7c30ac62ad89 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java @@ -258,7 +258,7 @@ public void testGrouperTimeout() throws Exception 4, parallelCombineThreads, mergeThreadLocal, - perQueryStats + groupByQueryMetrics ); closer.register(grouper); grouper.init(); diff --git a/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java b/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java index 6555921fafc8..0a12b8181e76 100644 --- a/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java +++ b/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java @@ -2035,8 +2035,7 @@ private void testGroupByQuery( resourcesReservationPool.reserve( new QueryResourceId(String.valueOf(query.hashCode())), finalQuery, - true, - new GroupByStatsProvider.PerQueryStats() + true ); runner = groupingEngine.mergeRunners(DirectQueryProcessingPool.INSTANCE, List.of(runner)); } @@ -2241,6 +2240,10 @@ void assertProjection() private static class ExpectedProjectionGroupBy extends ExpectedProjectionQueryMetrics implements GroupByQueryMetrics { + private long mergeBufferAcquisitionTimeNs; + private long spilledBytes; + private long mergeDictionarySize; + private ExpectedProjectionGroupBy(@Nullable String expectedProjection) { super(expectedProjection); @@ -2249,25 +2252,67 @@ private ExpectedProjectionGroupBy(@Nullable String expectedProjection) @Override public void numDimensions(GroupByQuery query) { - + // no-op for projection tests } @Override public void numMetrics(GroupByQuery query) { - + // no-op for projection tests } @Override public void numComplexMetrics(GroupByQuery query) { - + // no-op for projection tests } @Override public void granularity(GroupByQuery query) { + // no-op for projection tests + } + + @Override + public void reportGroupByStats() + { + // no-op for projection tests + } + + @Override + public void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime) + { + this.mergeBufferAcquisitionTimeNs += mergeBufferAcquisitionTime; + } + @Override + public void bytesSpilledToStorage(long bytesSpilledToStorage) + { + this.spilledBytes += bytesSpilledToStorage; + } + + @Override + public void mergeDictionarySize(long mergeDictionarySize) + { + this.mergeDictionarySize += mergeDictionarySize; + } + + @Override + public long getSpilledBytes() + { + return spilledBytes; + } + + @Override + public long getMergeDictionarySize() + { + return mergeDictionarySize; + } + + @Override + public long getMergeBufferAcquisitionTime() + { + return mergeBufferAcquisitionTimeNs; } } From e5cd850e88fb4e8f4391f218a6d431b1db55d3f1 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 27 Nov 2025 18:59:53 +0800 Subject: [PATCH 08/39] Javadocs --- .../org/apache/druid/query/groupby/GroupingEngine.java | 8 ++++---- .../groupby/epinephelinae/RowBasedGrouperHelper.java | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index c522604629b9..0c044da3bd12 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -329,7 +329,7 @@ public GroupByQuery prepareGroupByQuery(GroupByQuery query) * computing subtotals (like GROUPING SETS), and computing the havingSpec and limitSpec. * * @param baseRunner base query runner - * @param query the groupBy query to run inside the base query runner + * @param queryPlus query and context to run inside the base query runner * @param responseContext the response context to pass to the base query runner * * @return merged result sequence @@ -575,8 +575,8 @@ public Sequence applyPostProcessing(Sequence results, Grou /** * Called by {@link GroupByQueryQueryToolChest#mergeResults(QueryRunner)} when it needs to process a subquery. * - * @param subquery inner query - * @param query outer query + * @param subqueryPlus inner query (with context) + * @param queryPlus outer query (with context) * @param resource resources returned by {@link #prepareResource(GroupByQuery, BlockingPool, boolean, GroupByQueryConfig)} * @param subqueryResult result rows from the subquery * @param wasQueryPushedDown true if the outer query was pushed down (so we only need to merge the outer query's @@ -647,7 +647,7 @@ public Sequence processSubqueryResult( /** * Called by {@link GroupByQueryQueryToolChest#mergeResults(QueryRunner)} when it needs to generate subtotals. * - * @param query query that has a "subtotalsSpec" + * @param queryPlus query (and context) that has a "subtotalsSpec" * @param resource resources returned by {@link #prepareResource(GroupByQuery, BlockingPool, boolean, GroupByQueryConfig)} * @param queryResult result rows from the main query * diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java index 5346304a5eaf..bc04c8fe0795 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java @@ -169,9 +169,9 @@ public static Pair, Accumulator * and dim filters) are respected, and its aggregators are used in standard (not combining) form. The input * ResultRows are assumed to be results originating from the provided "subquery". * - * @param query query that we are grouping for - * @param subquery optional subquery that we are receiving results from (see combining vs. subquery - * mode above) + * @param queryPlus the query (and context) that we are grouping for + * @param subqueryPlus optional subquery (and context) that produced the intermediate rows, or {@code null} when + * operating in combining mode (see description above) * @param config groupBy query config * @param processingConfig processing config * @param bufferSupplier supplier of merge buffers From f69de58489a7e7b49772f6f10b234f851cf5e003 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 28 Nov 2025 11:28:13 +0800 Subject: [PATCH 09/39] Add metrics emitter in query runner --- .../query/MetricsEmittingQueryRunner.java | 2 ++ .../groupby/DefaultGroupByQueryMetrics.java | 15 ++++++-- .../druid/query/groupby/GroupingEngine.java | 5 +-- .../groupby/GroupByQueryRunnerTestHelper.java | 2 +- .../druid/query/scan/ScanQueryRunnerTest.java | 2 +- .../timeseries/TimeseriesQueryRunnerTest.java | 2 +- .../appenderator/SinkQuerySegmentWalker.java | 8 +++-- .../server/ClientQuerySegmentWalker.java | 36 +++++++++++++------ .../druid/server/LocalQuerySegmentWalker.java | 11 ++++++ .../apache/druid/server/ServerManager.java | 30 +++++++++++++--- 10 files changed, 84 insertions(+), 29 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java index 4baef60639ff..f806280cf602 100644 --- a/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java @@ -34,6 +34,8 @@ */ public class MetricsEmittingQueryRunner implements QueryRunner { + public static final ObjLongConsumer> NOOP_METRIC_REPORTER = (metrics, value) -> { }; + private final ServiceEmitter emitter; private final QueryToolChest> queryToolChest; private final QueryRunner queryRunner; diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index 2bec110f1fd9..3370cc259f4b 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -68,9 +68,9 @@ public void granularity(GroupByQuery query) @Override public void reportGroupByStats() { - reportMetricsIfNotZero("bytesSpilledToStorage", bytesSpilledToStorage); - reportMetricsIfNotZero("mergeDictionarySize", mergeDictionarySize); - reportMetricsIfNotZero("mergeBufferAcquisitonTimeNs", mergeBufferAcquisitonTime); + reportMetricsIfNotZero("bytesSpilledToStorage", bytesSpilledToStorage.longValue()); + reportMetricsIfNotZero("mergeDictionarySize", mergeDictionarySize.longValue()); + reportMetricsIfNotZero("mergeBufferAcquisitonTime", mergeBufferAcquisitonTime.longValue()); } @Override @@ -109,4 +109,13 @@ public long getMergeBufferAcquisitionTime() return mergeBufferAcquisitonTime.longValue(); } + @Override + public String toString() + { + return String.format("bytesSpilledToStorage[%s], mergeDictionarySize[%s], mergeBufferAcquisitonTime[%s]", + bytesSpilledToStorage.longValue(), + mergeDictionarySize.longValue(), + mergeBufferAcquisitonTime.longValue() + ); + } } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 0c044da3bd12..cd454a135656 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -1008,10 +1008,7 @@ public static boolean summaryRowPreconditions(GroupByQuery query) if (!query.getDimensions().isEmpty() || query.hasDroppedDimensions()) { return false; } - if (query.getGranularity().isFinerThan(Granularities.ALL)) { - return false; - } - return true; + return !query.getGranularity().isFinerThan(Granularities.ALL); } private static Iterator summaryRowIterator(GroupByQuery q) diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java index 049b00ca1159..2fb3004b2622 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java @@ -71,7 +71,7 @@ public static Iterable runQueryWithEmitter( serviceEmitter, factory.getToolchest(), runner, - (obj, lng) -> {}, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, (metrics) -> {} ).withWaitMeasuredFromNow(); QueryToolChest toolChest = factory.getToolchest(); diff --git a/processing/src/test/java/org/apache/druid/query/scan/ScanQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/scan/ScanQueryRunnerTest.java index b19312384ad1..dce4d40aca8a 100644 --- a/processing/src/test/java/org/apache/druid/query/scan/ScanQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/scan/ScanQueryRunnerTest.java @@ -211,7 +211,7 @@ public void testFullOnSelect() stubServiceEmitter, TOOL_CHEST, runner, - (obj, lng) -> {}, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, (metrics) -> {} ).withWaitMeasuredFromNow(); Iterable results = metricsEmittingQueryRunner.run(QueryPlus.wrap(query)).toList(); diff --git a/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java index 52cc5b149668..203eb4f0f1a0 100644 --- a/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/timeseries/TimeseriesQueryRunnerTest.java @@ -227,7 +227,7 @@ public void testFullOnTimeseries() stubServiceEmitter, new TimeseriesQueryQueryToolChest(), runner, - (obj, lng) -> {}, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, (metrics) -> {} ).withWaitMeasuredFromNow(); Iterable> results = metricsEmittingQueryRunner.run(QueryPlus.wrap(query)).toList(); diff --git a/server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java index 0fbff897711d..9d8283effffa 100644 --- a/server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java @@ -44,6 +44,7 @@ import org.apache.druid.query.DefaultQueryMetrics; import org.apache.druid.query.DirectQueryProcessingPool; import org.apache.druid.query.FinalizeResultsQueryRunner; +import org.apache.druid.query.MetricsEmittingQueryRunner; import org.apache.druid.query.NoopQueryRunner; import org.apache.druid.query.Query; import org.apache.druid.query.QueryDataSource; @@ -365,8 +366,9 @@ public QueryRunner getQueryRunnerForSegments(final Query query, final // 1) Populate resource id to the query // 2) Merge results using the toolChest, finalize if necessary. - // 3) Measure CPU time of that operation. - // 4) Release all sink segment references. + // 3) Emit metrics if necessary. + // 4) Measure CPU time of that operation. + // 5) Release all sink segment references. return new ResourceIdPopulatingQueryRunner<>( QueryRunnerHelper.makeClosingQueryRunner( CPUTimeMetricQueryRunner.safeBuild( @@ -468,7 +470,7 @@ public static String makeHydrantCacheIdentifier(final SegmentId segmentId, final * This class operates in two distinct modes based on whether {@link SinkMetricsEmittingQueryRunner#segmentId} is null or non-null. * When segmentId is non-null, it accumulates the metrics. When segmentId is null, it emits the accumulated metrics. *

- * This class is derived from {@link org.apache.druid.query.MetricsEmittingQueryRunner}. + * This class is derived from {@link MetricsEmittingQueryRunner}. */ private static class SinkMetricsEmittingQueryRunner implements QueryRunner { diff --git a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java index fe9b052de9b9..c54fca139b12 100644 --- a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java @@ -48,6 +48,7 @@ import org.apache.druid.query.GenericQueryMetricsFactory; import org.apache.druid.query.GlobalTableDataSource; import org.apache.druid.query.InlineDataSource; +import org.apache.druid.query.MetricsEmittingQueryRunner; import org.apache.druid.query.Query; import org.apache.druid.query.QueryContext; import org.apache.druid.query.QueryContexts; @@ -586,7 +587,6 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner .applyPreMergeDecoration() .mergeResults(false) .applyPostMergeDecoration() - .emitCPUTimeMetric(emitter) .postProcess( objectMapper.convertValue( query.context().getString("postProcessing"), @@ -595,16 +595,30 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner ) .map( runner -> - new ResultLevelCachingQueryRunner<>( - runner, - toolChest, - query, - objectMapper, - cache, - cacheConfig, - emitter - ) - ); + { + final QueryRunner cachingRunner = new ResultLevelCachingQueryRunner<>( + runner, + toolChest, + query, + objectMapper, + cache, + cacheConfig, + emitter + ); + + return new MetricsEmittingQueryRunner<>( + emitter, + toolChest, + cachingRunner, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, + queryMetrics -> { + queryMetrics.queryId(query.getId()); + queryMetrics.sqlQueryId(query.getSqlQueryId()); + } + ); + } + ) + .emitCPUTimeMetric(emitter); } /** diff --git a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java index ce453f409dd4..8b04ddd5d1ad 100644 --- a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java @@ -27,6 +27,7 @@ import org.apache.druid.query.DataSource; import org.apache.druid.query.DirectQueryProcessingPool; import org.apache.druid.query.FluentQueryRunner; +import org.apache.druid.query.MetricsEmittingQueryRunner; import org.apache.druid.query.Query; import org.apache.druid.query.QueryRunner; import org.apache.druid.query.QueryRunnerFactory; @@ -116,6 +117,16 @@ public QueryRunner getQueryRunnerForIntervals(final Query query, final .applyPreMergeDecoration() .mergeResults(true) .applyPostMergeDecoration() + .map(runner -> new MetricsEmittingQueryRunner<>( + emitter, + queryRunnerFactory.getToolchest(), + runner, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, + queryMetrics -> { + queryMetrics.queryId(query.getId()); + queryMetrics.sqlQueryId(query.getSqlQueryId()); + }) + ) .emitCPUTimeMetric(emitter, cpuAccumulator); } diff --git a/server/src/main/java/org/apache/druid/server/ServerManager.java b/server/src/main/java/org/apache/druid/server/ServerManager.java index b4a37dbeeedf..6620ad4ff37e 100644 --- a/server/src/main/java/org/apache/druid/server/ServerManager.java +++ b/server/src/main/java/org/apache/druid/server/ServerManager.java @@ -529,7 +529,11 @@ protected QueryRunner buildQueryRunnerForSegment( toolChest, bySegmentQueryRunner, QueryMetrics::reportSegmentAndCacheTime, - queryMetrics -> queryMetrics.segment(segmentIdString) + // TODO: Maybe also apply a log to see when this is called. + queryMetrics -> { + log.info("Segment MetricsEmittingQueryRunner accepting metrics[%s]", queryMetrics); + queryMetrics.segment(segmentIdString); + } ).withWaitMeasuredFromNow(); final SpecificSegmentQueryRunner specificSegmentQueryRunner = new SpecificSegmentQueryRunner<>( @@ -649,16 +653,32 @@ public Sequence run(QueryPlus queryPlus, ResponseContext responseContext) cpuTimeAccumulator, cacheKeyPrefix ); + + final QueryRunner finalizeResultsQueryRunner = new FinalizeResultsQueryRunner<>( + toolChest.mergeResults(factory.mergeRunners(queryProcessingPool, queryRunners), true), + toolChest + ); + + final QueryRunner datasourceMetricsEmittingQueryRunner = new MetricsEmittingQueryRunner<>( + emitter, + toolChest, + finalizeResultsQueryRunner, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, + metrics -> { + log.info("Datasource MetricsEmittingQueryRunner accepting metrics[%s]", metrics); + metrics.queryId(query.getId()); + metrics.sqlQueryId(query.getSqlQueryId()); + } + ); + final QueryRunner queryRunner = CPUTimeMetricQueryRunner.safeBuild( - new FinalizeResultsQueryRunner<>( - toolChest.mergeResults(factory.mergeRunners(queryProcessingPool, queryRunners), true), - toolChest - ), + datasourceMetricsEmittingQueryRunner, toolChest, emitter, cpuTimeAccumulator, true ); + return queryRunner.run(queryPlus, responseContext).withBaggage(closer); } catch (Throwable t) { From 40aa119788a8158acee76cdd37963fa2ca885712 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 28 Nov 2025 11:42:35 +0800 Subject: [PATCH 10/39] TODOs for groupby-metrics --- .../apache/druid/query/groupby/DefaultGroupByQueryMetrics.java | 1 + .../query/groupby/epinephelinae/GroupByMergingQueryRunner.java | 2 ++ .../druid/query/groupby/epinephelinae/SpillingGrouper.java | 3 ++- .../src/main/java/org/apache/druid/server/ServerManager.java | 1 + 4 files changed, 6 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index 3370cc259f4b..033c58c48394 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -109,6 +109,7 @@ public long getMergeBufferAcquisitionTime() return mergeBufferAcquisitonTime.longValue(); } + // TODO: Delete after debugging... @Override public String toString() { diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index 551287e2121a..5ce53b2c1b9b 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -152,6 +152,8 @@ public Sequence run(final QueryPlus queryPlus, final Respo CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION, false ); + + // TODO: Check will this give me NPE down the road when accessing QueryMetrics from the queryPlus? final QueryPlus queryPlusForRunners = queryPlus .withQuery( query.withOverriddenContext(ImmutableMap.of(CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION, true)) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java index 050e3921f2fd..e19712fe111a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java @@ -219,13 +219,14 @@ public void reset() public void updateGroupByQueryMetrics() { // TODO: If we do not want groupByQueryMetrics to do addition, simply return a Map, // then either the ConcurrentGrouper will aggregate the results into a accumulated map, - // or the GroupByQueryMetrics will provide a means to report GroupByMetrics via a map. + // and the GroupByQueryMetrics will provide a means to report GroupByMetrics via a map. groupByQueryMetrics.mergeDictionarySize(keySerde.getDictionarySize()); } @Override public void close() { + // TODO: Check if it is ok to emit at close. Will the QueryRunners give the results in time? grouper.close(); keySerde.reset(); deleteFiles(); diff --git a/server/src/main/java/org/apache/druid/server/ServerManager.java b/server/src/main/java/org/apache/druid/server/ServerManager.java index 6620ad4ff37e..19979de1d512 100644 --- a/server/src/main/java/org/apache/druid/server/ServerManager.java +++ b/server/src/main/java/org/apache/druid/server/ServerManager.java @@ -665,6 +665,7 @@ public Sequence run(QueryPlus queryPlus, ResponseContext responseContext) finalizeResultsQueryRunner, MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, metrics -> { + // TODO: Remove the logs after finished. log.info("Datasource MetricsEmittingQueryRunner accepting metrics[%s]", metrics); metrics.queryId(query.getId()); metrics.sqlQueryId(query.getSqlQueryId()); From 13973f181bf03f476f243ba10a8a61862ba353aa Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 28 Nov 2025 11:48:46 +0800 Subject: [PATCH 11/39] Checkstyle --- .../druid/benchmark/GroupByDeserializationBenchmark.java | 1 - .../apache/druid/query/groupby/epinephelinae/Grouper.java | 4 +++- .../druid/query/groupby/epinephelinae/SpillingGrouper.java | 3 ++- .../query/groupby/epinephelinae/ConcurrentGrouperTest.java | 1 - .../org/apache/druid/server/ClientQuerySegmentWalker.java | 7 +++---- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java index cfb2da3fd228..a907c99dec29 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java @@ -36,7 +36,6 @@ import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByQueryQueryToolChest; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.segment.TestHelper; import org.apache.druid.segment.column.ColumnType; diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java index d2763cb94877..b65203a2da9a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java @@ -97,7 +97,9 @@ default ToIntFunction hashFunction() * Update the results of the GroupByQueryMetrics. * Currently only used by {@link ConcurrentGrouper} and {@link SpillingGrouper} */ - default void updateGroupByQueryMetrics() {} + default void updateGroupByQueryMetrics() + { + } /** * Close the grouper and release associated resources. diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java index e19712fe111a..71c638c537d0 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java @@ -216,7 +216,8 @@ public void reset() } @Override - public void updateGroupByQueryMetrics() { + public void updateGroupByQueryMetrics() + { // TODO: If we do not want groupByQueryMetrics to do addition, simply return a Map, // then either the ConcurrentGrouper will aggregate the results into a accumulated map, // and the GroupByQueryMetrics will provide a means to report GroupByMetrics via a map. diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java index 7c30ac62ad89..2205118432dc 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java @@ -38,7 +38,6 @@ import org.apache.druid.query.dimension.DimensionSpec; import org.apache.druid.query.groupby.DefaultGroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQueryMetrics; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.epinephelinae.Grouper.BufferComparator; import org.apache.druid.query.groupby.epinephelinae.Grouper.Entry; import org.apache.druid.query.groupby.epinephelinae.Grouper.KeySerde; diff --git a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java index c54fca139b12..93e09ed9e0f6 100644 --- a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java @@ -594,9 +594,8 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner ) ) .map( - runner -> - { - final QueryRunner cachingRunner = new ResultLevelCachingQueryRunner<>( + runner -> { + final QueryRunner resultLevelCachingQueryRunner = new ResultLevelCachingQueryRunner<>( runner, toolChest, query, @@ -609,7 +608,7 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner return new MetricsEmittingQueryRunner<>( emitter, toolChest, - cachingRunner, + resultLevelCachingQueryRunner, MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, queryMetrics -> { queryMetrics.queryId(query.getId()); From 82018ebe9b01c7be173e0e2ac8734527bd8ca0ac Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 28 Nov 2025 12:07:25 +0800 Subject: [PATCH 12/39] forbidden-api for DefaultGroupByQueryMetrics#toString --- .../druid/query/groupby/DefaultGroupByQueryMetrics.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index 033c58c48394..c2f9384166f0 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -113,10 +113,9 @@ public long getMergeBufferAcquisitionTime() @Override public String toString() { - return String.format("bytesSpilledToStorage[%s], mergeDictionarySize[%s], mergeBufferAcquisitonTime[%s]", - bytesSpilledToStorage.longValue(), - mergeDictionarySize.longValue(), - mergeBufferAcquisitonTime.longValue() - ); + return "bytesSpilledToStorage[" + bytesSpilledToStorage.longValue() + + "], mergeDictionarySize[" + mergeDictionarySize.longValue() + + "], mergeBufferAcquisitonTime[" + mergeBufferAcquisitonTime.longValue() + + ']'; } } From e163c3718068b34f6522285c99ba2e4f032e302a Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 28 Nov 2025 14:12:25 +0800 Subject: [PATCH 13/39] Remove NotNull annotation --- .../druid/query/groupby/epinephelinae/ConcurrentGrouper.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java index c039f5dbbcf7..2f67aa2232f9 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java @@ -41,7 +41,6 @@ import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; import org.apache.druid.segment.ColumnSelectorFactory; -import org.jetbrains.annotations.NotNull; import javax.annotation.Nullable; import java.nio.ByteBuffer; @@ -246,7 +245,7 @@ public void init() } } - private @NotNull SpillingGrouper generateSpillingGrouperWithBufferSlice(ByteBuffer slice, int sliceSize) + private SpillingGrouper generateSpillingGrouperWithBufferSlice(ByteBuffer slice, int sliceSize) { final SpillingGrouper grouper = new SpillingGrouper<>( Suppliers.ofInstance(slice), From f4c6048e1e2d46685b015baff9dd41e88cd883b0 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 28 Nov 2025 18:34:06 +0800 Subject: [PATCH 14/39] Fix NPE in initAndMergeGroupByResults --- .../query/groupby/GroupByQueryQueryToolChest.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 52ae00194502..b3e0ca79445b 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -24,7 +24,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Functions; -import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import com.google.common.collect.Lists; import com.google.inject.Inject; @@ -157,15 +156,20 @@ public Comparator createResultComparator(Query query) } private Sequence initAndMergeGroupByResults( - final QueryPlus queryPlus, + QueryPlus queryPlus, QueryRunner runner, ResponseContext context, boolean willMergeRunner ) { final GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); - final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); + + if (queryPlus.getQueryMetrics() == null) { + queryPlus = queryPlus.withQueryMetrics(this); + } + + GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + // Reserve the group by resources (merge buffers) required for executing the query final QueryResourceId queryResourceId = query.context().getQueryResourceId(); From 80cf3af6564c0810da56fb6d723f1eed4001145e Mon Sep 17 00:00:00 2001 From: GWphua Date: Wed, 3 Dec 2025 18:35:36 +0800 Subject: [PATCH 15/39] Add MetricKey --- .../druid/query/context/ResponseContext.java | 29 ++++++++++--------- .../groupby/GroupByQueryQueryToolChest.java | 3 +- .../GroupByMergingQueryRunner.java | 3 +- .../LimitedTemporaryStorage.java | 5 +++- .../epinephelinae/SpillingGrouper.java | 4 ++- 5 files changed, 24 insertions(+), 20 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java b/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java index a2a068207188..4f14a4b6adda 100644 --- a/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java +++ b/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java @@ -279,6 +279,20 @@ public Object mergeValues(Object oldValue, Object newValue) } } + public static class MetricKey extends AbstractKey + { + MetricKey(String name) + { + super(name, false, false, new TypeReference() {}); + } + + @Override + public Object mergeValues(Object oldValue, Object newValue) + { + return ((AtomicLong) newValue).addAndGet(((AtomicLong) newValue).get()); + } + } + /** * Global registry of response context keys. Also defines the standard keys * associated with objects in the context. @@ -400,20 +414,7 @@ public Object mergeValues(Object oldValue, Object newValue) /** * Query total bytes gathered. */ - public static final Key QUERY_TOTAL_BYTES_GATHERED = new AbstractKey( - "queryTotalBytesGathered", - false, false, - new TypeReference() - { - } - ) - { - @Override - public Object mergeValues(Object oldValue, Object newValue) - { - return ((AtomicLong) newValue).addAndGet(((AtomicLong) newValue).get()); - } - }; + public static final Key QUERY_TOTAL_BYTES_GATHERED = new MetricKey("queryTotalBytesGathered"); /** * Query fail time (current time + timeout). diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index b3e0ca79445b..5330f6172f1c 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -88,8 +88,7 @@ public class GroupByQueryQueryToolChest extends QueryToolChest { private static final byte GROUPBY_QUERY = 0x14; - private static final TypeReference OBJECT_TYPE_REFERENCE = - new TypeReference<>() {}; + private static final TypeReference OBJECT_TYPE_REFERENCE = new TypeReference<>() {}; private static final TypeReference TYPE_REFERENCE = new TypeReference<>() {}; private final GroupingEngine groupingEngine; diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index 5ce53b2c1b9b..9bef3dc58ea3 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -21,7 +21,6 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Function; -import com.google.common.base.Preconditions; import com.google.common.base.Predicates; import com.google.common.base.Suppliers; import com.google.common.base.Throwables; @@ -141,7 +140,7 @@ public Sequence run(final QueryPlus queryPlus, final Respo final GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); + // Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); // CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION is here because realtime servers use nested mergeRunners calls // (one for the entire query and one for each sink). We only want the outer call to actually do merging with a diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java index edc315a0e3eb..60e5165e95a8 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java @@ -68,6 +68,7 @@ public LimitedTemporaryStorage( { this.storageDirectory = storageDirectory; this.maxBytesUsed = maxBytesUsed; + log.info("I actually created a LimitedTemporaryStorage without being able to track anything!"); this.groupByQueryMetrics = groupByQueryMetrics; } @@ -143,7 +144,9 @@ public void close() } closed = true; - groupByQueryMetrics.bytesSpilledToStorage(bytesUsed.get()); + if (groupByQueryMetrics != null) { + groupByQueryMetrics.bytesSpilledToStorage(bytesUsed.get()); + } bytesUsed.set(0); for (File file : ImmutableSet.copyOf(files)) { diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java index 71c638c537d0..2dbe2a6a3d0c 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java @@ -221,7 +221,9 @@ public void updateGroupByQueryMetrics() // TODO: If we do not want groupByQueryMetrics to do addition, simply return a Map, // then either the ConcurrentGrouper will aggregate the results into a accumulated map, // and the GroupByQueryMetrics will provide a means to report GroupByMetrics via a map. - groupByQueryMetrics.mergeDictionarySize(keySerde.getDictionarySize()); + if (groupByQueryMetrics != null) { + groupByQueryMetrics.mergeDictionarySize(keySerde.getDictionarySize()); + } } @Override From 37cc8b24f8c5eca48f3d46a0ed42c6028304ac59 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 09:58:26 +0800 Subject: [PATCH 16/39] GroupingEngine reversion --- .../druid/query/groupby/GroupingEngine.java | 53 ++++++++----------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index cd454a135656..34920fed35bf 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -329,18 +329,17 @@ public GroupByQuery prepareGroupByQuery(GroupByQuery query) * computing subtotals (like GROUPING SETS), and computing the havingSpec and limitSpec. * * @param baseRunner base query runner - * @param queryPlus query and context to run inside the base query runner + * @param query the groupBy query to run inside the base query runner * @param responseContext the response context to pass to the base query runner * * @return merged result sequence */ public Sequence mergeResults( final QueryRunner baseRunner, - final QueryPlus queryPlus, + final GroupByQuery query, final ResponseContext responseContext ) { - GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); // Merge streams using ResultMergeQueryRunner, then apply postaggregators, then apply limit (which may // involve materialization) final ResultMergeQueryRunner mergingQueryRunner = new ResultMergeQueryRunner<>( @@ -357,7 +356,7 @@ public Sequence mergeResults( final int timestampResultFieldIndexInOriginalDimensions = hasTimestampResultField ? queryContext.getInt(GroupByQuery.CTX_TIMESTAMP_RESULT_FIELD_INDEX) : 0; final GroupByQuery newQuery = prepareGroupByQuery(query); - final Sequence mergedResults = mergingQueryRunner.run(queryPlus.withQuery(newQuery), responseContext); + final Sequence mergedResults = mergingQueryRunner.run(QueryPlus.wrap(newQuery), responseContext); // Apply postaggregators if this is the outermost mergeResults (CTX_KEY_OUTERMOST) and we are not executing a // pushed-down subquery (CTX_KEY_EXECUTING_NESTED_QUERY). @@ -575,8 +574,8 @@ public Sequence applyPostProcessing(Sequence results, Grou /** * Called by {@link GroupByQueryQueryToolChest#mergeResults(QueryRunner)} when it needs to process a subquery. * - * @param subqueryPlus inner query (with context) - * @param queryPlus outer query (with context) + * @param subquery inner query + * @param query outer query * @param resource resources returned by {@link #prepareResource(GroupByQuery, BlockingPool, boolean, GroupByQueryConfig)} * @param subqueryResult result rows from the subquery * @param wasQueryPushedDown true if the outer query was pushed down (so we only need to merge the outer query's @@ -585,8 +584,8 @@ public Sequence applyPostProcessing(Sequence results, Grou * @return results of the outer query */ public Sequence processSubqueryResult( - QueryPlus subqueryPlus, - QueryPlus queryPlus, + GroupByQuery subquery, + GroupByQuery query, GroupByQueryResources resource, Sequence subqueryResult, boolean wasQueryPushedDown @@ -596,7 +595,6 @@ public Sequence processSubqueryResult( // while creating the sequence. GroupByRowProcessor.ResultSupplier resultSupplier = null; - GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); try { GroupByQuery queryToRun; @@ -615,11 +613,9 @@ public Sequence processSubqueryResult( queryToRun = queryToRun.withLimitSpec(((DefaultLimitSpec) queryToRun.getLimitSpec()).withOffsetToLimit()); } - final QueryPlus queryPlusToRun = queryPlus.withQuery(queryToRun); - resultSupplier = GroupByRowProcessor.process( - queryPlusToRun, - wasQueryPushedDown ? queryPlusToRun : subqueryPlus, + queryToRun, + wasQueryPushedDown ? queryToRun : subquery, subqueryResult, configSupplier.get(), processingConfig, @@ -632,8 +628,8 @@ public Sequence processSubqueryResult( final GroupByRowProcessor.ResultSupplier finalResultSupplier = resultSupplier; return Sequences.withBaggage( mergeResults( - (qp, responseContext) -> finalResultSupplier.results(null), - queryPlus, + (queryPlus, responseContext) -> finalResultSupplier.results(null), + query, ResponseContext.createEmpty() ), finalResultSupplier @@ -647,14 +643,14 @@ public Sequence processSubqueryResult( /** * Called by {@link GroupByQueryQueryToolChest#mergeResults(QueryRunner)} when it needs to generate subtotals. * - * @param queryPlus query (and context) that has a "subtotalsSpec" + * @param query query that has a "subtotalsSpec" * @param resource resources returned by {@link #prepareResource(GroupByQuery, BlockingPool, boolean, GroupByQueryConfig)} * @param queryResult result rows from the main query * * @return results for each list of subtotals in the query, concatenated together */ public Sequence processSubtotalsSpec( - QueryPlus queryPlus, + GroupByQuery query, GroupByQueryResources resource, Sequence queryResult ) @@ -673,7 +669,6 @@ public Sequence processSubtotalsSpec( // while creating the sequence. GroupByRowProcessor.ResultSupplier resultSupplierOne = null; - GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); try { // baseSubtotalQuery is the original query with dimensions and aggregators rewritten to apply to the *results* // rather than *inputs* of that query. It has its virtual columns and dim filter removed, because those only @@ -699,11 +694,9 @@ public Sequence processSubtotalsSpec( // timestampResult optimization is not for subtotal scenario, so disable it .withOverriddenContext(ImmutableMap.of(GroupByQuery.CTX_TIMESTAMP_RESULT_FIELD, "")); - QueryPlus baseSubtotalQueryPlus = queryPlus.withQuery(baseSubtotalQuery); - resultSupplierOne = GroupByRowProcessor.process( - baseSubtotalQueryPlus, - baseSubtotalQueryPlus, + baseSubtotalQuery, + baseSubtotalQuery, queryResult, configSupplier.get(), processingConfig, @@ -746,16 +739,14 @@ public Sequence processSubtotalsSpec( subtotalQueryLimitSpec = baseSubtotalQuery.getLimitSpec().filterColumns(columns); } - QueryPlus subtotalQueryPlus = baseSubtotalQueryPlus.withQuery( - baseSubtotalQuery - .withLimitSpec(subtotalQueryLimitSpec)); + GroupByQuery subtotalQuery = baseSubtotalQuery.withLimitSpec(subtotalQueryLimitSpec); final GroupByRowProcessor.ResultSupplier resultSupplierOneFinal = resultSupplierOne; if (Utils.isPrefix(subtotalSpec, queryDimNamesInOrder)) { // Since subtotalSpec is a prefix of base query dimensions, so results from base query are also sorted // by subtotalSpec as needed by stream merging. subtotalsResults.add( - processSubtotalsResultAndOptionallyClose(() -> resultSupplierOneFinal, subTotalDimensionSpec, subtotalQueryPlus, false) + processSubtotalsResultAndOptionallyClose(() -> resultSupplierOneFinal, subTotalDimensionSpec, subtotalQuery, false) ); } else { // Since subtotalSpec is not a prefix of base query dimensions, so results from base query are not sorted @@ -765,8 +756,8 @@ public Sequence processSubtotalsSpec( // Also note, we can't create the ResultSupplier eagerly here or as we don't want to eagerly allocate // merge buffers for processing subtotal. Supplier resultSupplierTwo = () -> GroupByRowProcessor.process( - baseSubtotalQueryPlus, - subtotalQueryPlus, + baseSubtotalQuery, + subtotalQuery, resultSupplierOneFinal.results(subTotalDimensionSpec), configSupplier.get(), processingConfig, @@ -777,7 +768,7 @@ public Sequence processSubtotalsSpec( ); subtotalsResults.add( - processSubtotalsResultAndOptionallyClose(resultSupplierTwo, subTotalDimensionSpec, subtotalQueryPlus, true) + processSubtotalsResultAndOptionallyClose(resultSupplierTwo, subTotalDimensionSpec, subtotalQuery, true) ); } } @@ -795,7 +786,7 @@ public Sequence processSubtotalsSpec( private Sequence processSubtotalsResultAndOptionallyClose( Supplier baseResultsSupplier, List dimsToInclude, - QueryPlus subtotalQueryPlus, + GroupByQuery subtotalQuery, boolean closeOnSequenceRead ) { @@ -813,7 +804,7 @@ private Sequence processSubtotalsResultAndOptionallyClose( : () -> {} ) ), - subtotalQueryPlus, + subtotalQuery, ResponseContext.createEmpty() ); } From 3c7d3e409d858bb31b0ebf0478e73663b769c4e5 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 11:25:58 +0800 Subject: [PATCH 17/39] Migrating to response context --- .../druid/query/context/ResponseContext.java | 2 +- .../groupby/GroupByQueryQueryToolChest.java | 58 ++++++++----------- .../epinephelinae/GroupByRowProcessor.java | 5 +- 3 files changed, 28 insertions(+), 37 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java b/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java index 4f14a4b6adda..42a63995dcbb 100644 --- a/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java +++ b/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java @@ -281,7 +281,7 @@ public Object mergeValues(Object oldValue, Object newValue) public static class MetricKey extends AbstractKey { - MetricKey(String name) + public MetricKey(String name) { super(name, false, false, new TypeReference() {}); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 5330f6172f1c..4bcef5fea32e 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -24,6 +24,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Functions; +import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import com.google.common.collect.Lists; import com.google.inject.Inject; @@ -80,6 +81,7 @@ import java.util.Map; import java.util.Optional; import java.util.TreeMap; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.BinaryOperator; /** @@ -163,19 +165,16 @@ private Sequence initAndMergeGroupByResults( { final GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); - if (queryPlus.getQueryMetrics() == null) { - queryPlus = queryPlus.withQueryMetrics(this); - } - - GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - + // TODO: Append the GroupByQueryMetrics using the ResponseContext. + boolean reportMetricsForEmission = queryPlus.getQueryMetrics() != null; // Reserve the group by resources (merge buffers) required for executing the query final QueryResourceId queryResourceId = query.context().getQueryResourceId(); long startNs = System.nanoTime(); groupByResourcesReservationPool.reserve(queryResourceId, query, willMergeRunner); - groupByQueryMetrics.mergeBufferAcquisitionTime(System.nanoTime() - startNs); + context.add(new ResponseContext.MetricKey("mergeBufferAcquisitionTime"), + new AtomicLong(System.nanoTime() - startNs)); final GroupByQueryResources resource = groupByResourcesReservationPool.fetch(queryResourceId); if (resource == null) { @@ -188,7 +187,7 @@ private Sequence initAndMergeGroupByResults( Closer closer = Closer.create(); final Sequence mergedSequence = mergeGroupByResults( - queryPlus, + query, resource, runner, context, @@ -210,21 +209,21 @@ private Sequence initAndMergeGroupByResults( } private Sequence mergeGroupByResults( - final QueryPlus queryPlus, + final GroupByQuery query, GroupByQueryResources resource, QueryRunner runner, ResponseContext context, Closer closer ) { - if (isNestedQueryPushDown((GroupByQuery) queryPlus.getQuery())) { - return mergeResultsWithNestedQueryPushDown(queryPlus, resource, runner, context); + if (isNestedQueryPushDown(query)) { + return mergeResultsWithNestedQueryPushDown(query, resource, runner, context); } - return mergeGroupByResultsWithoutPushDown(queryPlus, resource, runner, context, closer); + return mergeGroupByResultsWithoutPushDown(query, resource, runner, context, closer); } private Sequence mergeGroupByResultsWithoutPushDown( - QueryPlus queryPlus, + GroupByQuery query, GroupByQueryResources resource, QueryRunner runner, ResponseContext context, @@ -232,19 +231,15 @@ private Sequence mergeGroupByResultsWithoutPushDown( ) { // If there's a subquery, merge subquery results and then apply the aggregator - GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); - final DataSource maybeQueryDataSource = query.getDataSource(); if (!(maybeQueryDataSource instanceof QueryDataSource)) { if (query.getSubtotalsSpec() == null) { - return groupingEngine.applyPostProcessing(groupingEngine.mergeResults(runner, queryPlus, context), query); + Sequence mergedResults = groupingEngine.mergeResults(runner, query, context); + return groupingEngine.applyPostProcessing(mergedResults, query); } else { - return groupingEngine.processSubtotalsSpec( - queryPlus, - resource, - groupingEngine.mergeResults(runner, queryPlus.withQuery(query.withSubtotalsSpec(null)), context) - ); + Sequence mergedResults = groupingEngine.mergeResults(runner, query.withSubtotalsSpec(null), context); + return groupingEngine.processSubtotalsSpec(query, resource, mergedResults); } } @@ -274,10 +269,9 @@ private Sequence mergeGroupByResultsWithoutPushDown( // TODO: Check if we need to do a fitting action here. // closer.register(() -> groupByStatsProvider.closeQuery(subquery.context().getQueryResourceId())); - final QueryPlus subqueryPlus = queryPlus.withQuery(subquery); final Sequence subqueryResult = mergeGroupByResults( - subqueryPlus, + subquery, resource, runner, context, @@ -286,8 +280,8 @@ private Sequence mergeGroupByResultsWithoutPushDown( final Sequence finalizingResults = finalizeSubqueryResults(subqueryResult, subquery); final Sequence processedSubqueryResults = groupingEngine.processSubqueryResult( - subqueryPlus, - queryPlus, + subquery, + query, resource, finalizingResults, false @@ -296,26 +290,24 @@ private Sequence mergeGroupByResultsWithoutPushDown( if (query.getSubtotalsSpec() == null) { return groupingEngine.applyPostProcessing(processedSubqueryResults, query); } else { - return groupingEngine.processSubtotalsSpec(queryPlus, resource, processedSubqueryResults); + return groupingEngine.processSubtotalsSpec(query, resource, processedSubqueryResults); } } private Sequence mergeResultsWithNestedQueryPushDown( - QueryPlus queryPlus, + GroupByQuery query, GroupByQueryResources resource, QueryRunner runner, ResponseContext context ) { - GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); - Sequence pushDownQueryResults = groupingEngine.mergeResults(runner, queryPlus, context); + Sequence pushDownQueryResults = groupingEngine.mergeResults(runner, query, context); final Sequence finalizedResults = finalizeSubqueryResults(pushDownQueryResults, query); - QueryPlus rewrittenQueryPlus = queryPlus.withQuery(rewriteNestedQueryForPushDown(query)); Sequence processedSubqueryResult = groupingEngine.processSubqueryResult( - queryPlus, - rewrittenQueryPlus, + query, + rewriteNestedQueryForPushDown(query), resource, finalizedResults, true @@ -331,7 +323,7 @@ private Sequence mergeResultsWithNestedQueryPushDown( @VisibleForTesting GroupByQuery rewriteNestedQueryForPushDown(GroupByQuery query) { - return query.withAggregatorSpecs(Lists.transform(query.getAggregatorSpecs(), (agg) -> agg.getCombiningFactory())) + return query.withAggregatorSpecs(Lists.transform(query.getAggregatorSpecs(), AggregatorFactory::getCombiningFactory)) .withDimensionSpecs(Lists.transform( query.getDimensions(), (dim) -> new DefaultDimensionSpec( diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java index 8d24c9a1def1..8f8f93caa9b5 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java @@ -88,8 +88,8 @@ private GroupByRowProcessor() * calls to the {@link ResultSupplier}. Make sure to close it when you're done. */ public static ResultSupplier process( - final QueryPlus queryPlus, - final QueryPlus subqueryPlus, + final GroupByQuery query, + final GroupByQuery subquery, final Sequence rows, final GroupByQueryConfig config, final DruidProcessingConfig processingConfig, @@ -99,7 +99,6 @@ public static ResultSupplier process( final int mergeBufferSize ) { - GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); final Closer closeOnExit = Closer.create(); From 906e823ee6983edc6447162083138ba9c89b9b37 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 15:24:12 +0800 Subject: [PATCH 18/39] Response Context Long Key --- .../druid/query/context/ResponseContext.java | 31 +++++++++---------- .../groupby/GroupByResponseContextKeys.java | 1 + 2 files changed, 16 insertions(+), 16 deletions(-) create mode 100644 processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java diff --git a/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java b/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java index 42a63995dcbb..0c94683b30d2 100644 --- a/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java +++ b/processing/src/main/java/org/apache/druid/query/context/ResponseContext.java @@ -244,7 +244,7 @@ public Object mergeValues(Object oldValue, Object newValue) */ public static class LongKey extends AbstractKey { - LongKey(String name, boolean inHeader) + public LongKey(String name, boolean inHeader) { super(name, inHeader, false, Long.class); } @@ -279,20 +279,6 @@ public Object mergeValues(Object oldValue, Object newValue) } } - public static class MetricKey extends AbstractKey - { - public MetricKey(String name) - { - super(name, false, false, new TypeReference() {}); - } - - @Override - public Object mergeValues(Object oldValue, Object newValue) - { - return ((AtomicLong) newValue).addAndGet(((AtomicLong) newValue).get()); - } - } - /** * Global registry of response context keys. Also defines the standard keys * associated with objects in the context. @@ -414,7 +400,20 @@ public Object mergeValues(Object oldValue, Object newValue) /** * Query total bytes gathered. */ - public static final Key QUERY_TOTAL_BYTES_GATHERED = new MetricKey("queryTotalBytesGathered"); + public static final Key QUERY_TOTAL_BYTES_GATHERED = new AbstractKey( + "queryTotalBytesGathered", + false, false, + new TypeReference() + { + } + ) + { + @Override + public Object mergeValues(Object oldValue, Object newValue) + { + return ((AtomicLong) newValue).addAndGet(((AtomicLong) newValue).get()); + } + }; /** * Query fail time (current time + timeout). diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java new file mode 100644 index 000000000000..8b137891791f --- /dev/null +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java @@ -0,0 +1 @@ + From f2a8028c2a2b891b345f223c983c4fa58953dab6 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 15:24:43 +0800 Subject: [PATCH 19/39] GroupByResponseContextKey --- .../groupby/GroupByResponseContextKeys.java | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java index 8b137891791f..0ac6d7e6c85d 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java @@ -1 +1,86 @@ +/* + * 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 this 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.druid.query.groupby; + +import org.apache.druid.query.context.ResponseContext; + +/** + * Response context keys for GroupBy query metrics. + * These keys are used to aggregate metrics from parallel query execution threads + * using MAX aggregation (taking the maximum value across all threads). + */ +public class GroupByResponseContextKeys +{ + public static final String GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_NAME = "groupByMergeBufferAcquisitionTime"; + public static final String GROUPBY_BYTES_SPILLED_TO_STORAGE_NAME = "groupByBytesSpilledToStorage"; + public static final String GROUPBY_MERGE_DICTIONARY_SIZE_NAME = "groupByMergeDictionarySize"; + + /** + * Maximum bytes spilled to storage across all parallel threads processing segments. + * This represents the peak disk usage during query execution. + */ + public static final ResponseContext.Key GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY = + new ResponseContext.LongKey(GROUPBY_BYTES_SPILLED_TO_STORAGE_NAME, false) + { + @Override + public Object mergeValues(Object oldValue, Object newValue) + { + return Math.max((Long) oldValue, (Long) newValue); + } + }; + + /** + * Maximum merge dictionary size across all parallel threads processing segments. + * This represents the peak dictionary size used during query execution. + */ + public static final ResponseContext.Key GROUPBY_MERGE_DICTIONARY_SIZE_KEY = + new ResponseContext.LongKey(GROUPBY_MERGE_DICTIONARY_SIZE_NAME, false) + { + @Override + public Object mergeValues(Object oldValue, Object newValue) + { + return Math.max((Long) oldValue, (Long) newValue); + } + }; + + /** + * Maximum merge buffer acquisition time across all parallel threads processing segments. + * This represents the longest time any thread waited to acquire a merge buffer. + */ + public static final ResponseContext.Key GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_KEY = + new ResponseContext.LongKey(GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_NAME, false) + { + @Override + public Object mergeValues(Object oldValue, Object newValue) + { + return Math.max((Long) oldValue, (Long) newValue); + } + }; + + static { + ResponseContext.Keys.instance().registerKeys( + new ResponseContext.Key[]{ + GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY, + GROUPBY_MERGE_DICTIONARY_SIZE_KEY, + GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_KEY + } + ); + } +} From 9d3e6fcb8bd779e4d2b1dc1cf09995f04ceaa161 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 15:28:56 +0800 Subject: [PATCH 20/39] LimitedTemporaryStorage signature change --- .../groupby/epinephelinae/LimitedTemporaryStorage.java | 7 ++----- .../query/groupby/epinephelinae/ConcurrentGrouperTest.java | 5 ++--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java index 60e5165e95a8..20e9f9a973f9 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java @@ -48,7 +48,7 @@ public class LimitedTemporaryStorage implements Closeable { private static final Logger log = new Logger(LimitedTemporaryStorage.class); - private final GroupByQueryMetrics groupByQueryMetrics; +// private final GroupByQueryMetrics groupByQueryMetrics; private final File storageDirectory; private final long maxBytesUsed; @@ -62,14 +62,11 @@ public class LimitedTemporaryStorage implements Closeable public LimitedTemporaryStorage( File storageDirectory, - long maxBytesUsed, - GroupByQueryMetrics groupByQueryMetrics + long maxBytesUsed ) { this.storageDirectory = storageDirectory; this.maxBytesUsed = maxBytesUsed; - log.info("I actually created a LimitedTemporaryStorage without being able to track anything!"); - this.groupByQueryMetrics = groupByQueryMetrics; } /** diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java index 2205118432dc..20029cf33efe 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java @@ -152,8 +152,7 @@ public void testAggregate() throws InterruptedException, ExecutionException, IOE GroupByQueryMetrics groupByQueryMetrics = new DefaultGroupByQueryMetrics(); final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryFolder.newFolder(), - 1024 * 1024, - groupByQueryMetrics + 1024 * 1024 ); final ListeningExecutorService service = MoreExecutors.listeningDecorator(exec); try { @@ -245,7 +244,7 @@ public void testGrouperTimeout() throws Exception 1024, 0.7f, 1, - new LimitedTemporaryStorage(temporaryFolder.newFolder(), 1024 * 1024, groupByQueryMetrics), + new LimitedTemporaryStorage(temporaryFolder.newFolder(), 1024 * 1024), new DefaultObjectMapper(), concurrencyHint, null, From 63e80be6435114b6239c4a5ae46e82745c6f36e3 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 15:29:13 +0800 Subject: [PATCH 21/39] QueryPlus -> GroupByQuery --- .../groupby/GroupByQueryQueryToolChest.java | 22 +++++++-------- .../epinephelinae/ConcurrentGrouper.java | 21 ++++++++++++--- .../GroupByMergingQueryRunner.java | 25 ++++++++--------- .../epinephelinae/GroupByRowProcessor.java | 14 +++------- .../query/groupby/epinephelinae/Grouper.java | 5 +++- .../epinephelinae/RowBasedGrouperHelper.java | 27 ++++++++----------- .../epinephelinae/SpillingGrouper.java | 14 +++++----- 7 files changed, 65 insertions(+), 63 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 4bcef5fea32e..dd8d838abcd7 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -24,7 +24,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Functions; -import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import com.google.common.collect.Lists; import com.google.inject.Inject; @@ -81,7 +80,6 @@ import java.util.Map; import java.util.Optional; import java.util.TreeMap; -import java.util.concurrent.atomic.AtomicLong; import java.util.function.BinaryOperator; /** @@ -173,8 +171,7 @@ private Sequence initAndMergeGroupByResults( long startNs = System.nanoTime(); groupByResourcesReservationPool.reserve(queryResourceId, query, willMergeRunner); - context.add(new ResponseContext.MetricKey("mergeBufferAcquisitionTime"), - new AtomicLong(System.nanoTime() - startNs)); + context.add(GroupByResponseContextKeys.GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_KEY, System.nanoTime() - startNs); final GroupByQueryResources resource = groupByResourcesReservationPool.fetch(queryResourceId); if (resource == null) { @@ -186,16 +183,17 @@ private Sequence initAndMergeGroupByResults( try { Closer closer = Closer.create(); - final Sequence mergedSequence = mergeGroupByResults( - query, - resource, - runner, - context, - closer - ); + final Sequence mergedSequence = mergeGroupByResults(query, resource, runner, context, closer); // Clean up the resources reserved during the execution of the query closer.register(() -> groupByResourcesReservationPool.clean(queryResourceId)); + closer.register(() -> { + if (reportMetricsForEmission) { + GroupByQueryMetrics queryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + // TODO: Populate the metrics here. + } + }); + // TODO: Maybe attach metrics reporting with the closer? // closer.register(() -> groupByStatsProvider.closeQuery(query.context().getQueryResourceId())); @@ -471,7 +469,7 @@ public Sequence run(QueryPlus queryPlus, ResponseContext r dimensionSpecs.add(dimensionSpec); } } - + // TODO: Is this where I aggregate the responseContext? return runner.run( queryPlus.withQuery(groupByQuery.withDimensionSpecs(dimensionSpecs)), responseContext diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java index 2f67aa2232f9..012b240699f6 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java @@ -47,8 +47,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; @@ -116,6 +118,7 @@ public ConcurrentGrouper( final int priority, final boolean hasQueryTimeout, final long queryTimeoutAt, + // TODO: To delete later? final GroupByQueryMetrics groupByQueryMetrics ) { @@ -167,6 +170,7 @@ public ConcurrentGrouper( final int intermediateCombineDegree, final int numParallelCombineThreads, final boolean mergeThreadLocal, + // TODO: To delete if necessary. final GroupByQueryMetrics groupByQueryMetrics ) { @@ -260,8 +264,7 @@ private SpillingGrouper generateSpillingGrouperWithBufferSlice(ByteBuff false, limitSpec, sortHasNonGroupingFields, - sliceSize, - groupByQueryMetrics + sliceSize ); grouper.init(); return grouper; @@ -498,9 +501,19 @@ private List tryMergeDictionary() } @Override - public void updateGroupByQueryMetrics() + public Map getQueryMetricsMap() { - groupers.forEach(SpillingGrouper::updateGroupByQueryMetrics); + Map map = new HashMap<>(); + + groupers.forEach(spillingGroupper -> { + Map metricsMap = spillingGroupper.getQueryMetricsMap(); + + for (Map.Entry entry : metricsMap.entrySet()) { + map.compute(entry.getKey(), (key, value) -> value == null ? entry.getValue() : value + entry.getValue()); + } + }); + + return map; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index 9bef3dc58ea3..b8add63b1d40 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -57,9 +57,9 @@ import org.apache.druid.query.context.ResponseContext; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; -import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQueryResources; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; +import org.apache.druid.query.groupby.GroupByResponseContextKeys; import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.epinephelinae.RowBasedGrouperHelper.RowBasedKey; @@ -69,6 +69,7 @@ import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.UUID; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; @@ -139,8 +140,6 @@ public Sequence run(final QueryPlus queryPlus, final Respo { final GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); - final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - // Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); // CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION is here because realtime servers use nested mergeRunners calls // (one for the entire query and one for each sink). We only want the outer call to actually do merging with a @@ -193,8 +192,7 @@ public CloseableGrouperIterator make() try { final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryStorageDirectory, - querySpecificConfig.getMaxOnDiskStorage().getBytes(), - groupByQueryMetrics + querySpecificConfig.getMaxOnDiskStorage().getBytes() ); final ReferenceCountingResourceHolder temporaryStorageHolder = @@ -215,7 +213,7 @@ public CloseableGrouperIterator make() Pair, Accumulator> pair = RowBasedGrouperHelper.createGrouperAccumulatorPair( - queryPlus, + query, null, config, processingConfig, @@ -307,15 +305,14 @@ public AggregateResult call() waitForFutureCompletion(query, futures, hasTimeout, timeoutAt - System.currentTimeMillis()); } - grouper.updateGroupByQueryMetrics(); - groupByStatsProvider.aggregateStats(groupByQueryMetrics); - groupByQueryMetrics.reportGroupByStats(); + Map metricsMap = grouper.getQueryMetricsMap(); - return RowBasedGrouperHelper.makeGrouperIterator( - grouper, - query, - resources - ); + if (responseContext != null) { + responseContext.add(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY, temporaryStorage.currentSize()); + responseContext.add(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY, metricsMap.getOrDefault(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_NAME, 0L)); + } + + return RowBasedGrouperHelper.makeGrouperIterator(grouper, query, resources); } catch (Throwable t) { // Exception caught while setting up the iterator; release resources. diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java index 8f8f93caa9b5..4dfe37c79c14 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java @@ -30,12 +30,10 @@ import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.java.util.common.io.Closer; import org.apache.druid.query.DruidProcessingConfig; -import org.apache.druid.query.QueryPlus; import org.apache.druid.query.ResourceLimitExceededException; import org.apache.druid.query.dimension.DimensionSpec; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; -import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQueryResources; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.epinephelinae.RowBasedGrouperHelper.RowBasedKey; @@ -99,8 +97,6 @@ public static ResultSupplier process( final int mergeBufferSize ) { - final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - Preconditions.checkNotNull(groupByQueryMetrics, "groupByQueryMetrics"); final Closer closeOnExit = Closer.create(); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); @@ -111,15 +107,14 @@ public static ResultSupplier process( final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryStorageDirectory, - querySpecificConfig.getMaxOnDiskStorage().getBytes(), - groupByQueryMetrics + querySpecificConfig.getMaxOnDiskStorage().getBytes() ); closeOnExit.register(temporaryStorage); Pair, Accumulator> pair = RowBasedGrouperHelper.createGrouperAccumulatorPair( - queryPlus, - subqueryPlus, + query, + subquery, querySpecificConfig, processingConfig, new Supplier<>() @@ -147,8 +142,7 @@ public ByteBuffer get() throw new ResourceLimitExceededException(retVal.getReason()); } - grouper.updateGroupByQueryMetrics(); - groupByQueryMetrics.reportGroupByStats(); + grouper.getQueryMetricsMap(); return new ResultSupplier() { diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java index b65203a2da9a..b77bf777fe62 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/Grouper.java @@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; import org.apache.druid.java.util.common.parsers.CloseableIterator; import org.apache.druid.query.aggregation.AggregatorFactory; @@ -29,6 +30,7 @@ import java.nio.ByteBuffer; import java.util.Comparator; import java.util.List; +import java.util.Map; import java.util.function.ToIntFunction; /** @@ -97,8 +99,9 @@ default ToIntFunction hashFunction() * Update the results of the GroupByQueryMetrics. * Currently only used by {@link ConcurrentGrouper} and {@link SpillingGrouper} */ - default void updateGroupByQueryMetrics() + default Map getQueryMetricsMap() { + return ImmutableMap.of(); } /** diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java index bc04c8fe0795..591972343bad 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java @@ -56,6 +56,7 @@ import org.apache.druid.query.dimension.DimensionSpec; import org.apache.druid.query.filter.Filter; import org.apache.druid.query.filter.ValueMatcher; +import org.apache.druid.query.groupby.DefaultGroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByQueryMetrics; @@ -127,8 +128,8 @@ private RowBasedGrouperHelper() * Create a single-threaded grouper and accumulator. */ public static Pair, Accumulator> createGrouperAccumulatorPair( - final QueryPlus query, - @Nullable final QueryPlus subquery, + final GroupByQuery query, + @Nullable final GroupByQuery subquery, final GroupByQueryConfig config, final DruidProcessingConfig processingConfig, final Supplier bufferSupplier, @@ -169,8 +170,8 @@ public static Pair, Accumulator * and dim filters) are respected, and its aggregators are used in standard (not combining) form. The input * ResultRows are assumed to be results originating from the provided "subquery". * - * @param queryPlus the query (and context) that we are grouping for - * @param subqueryPlus optional subquery (and context) that produced the intermediate rows, or {@code null} when + * @param query the query (and context) that we are grouping for + * @param subquery optional subquery (and context) that produced the intermediate rows, or {@code null} when * operating in combining mode (see description above) * @param config groupBy query config * @param processingConfig processing config @@ -187,8 +188,8 @@ public static Pair, Accumulator * @param mergeBufferSize size of the merge buffers from "bufferSupplier" */ public static Pair, Accumulator> createGrouperAccumulatorPair( - final QueryPlus queryPlus, - @Nullable final QueryPlus subqueryPlus, + final GroupByQuery query, + @Nullable final GroupByQuery subquery, final GroupByQueryConfig config, final DruidProcessingConfig processingConfig, final Supplier bufferSupplier, @@ -211,10 +212,7 @@ public static Pair, Accumulator } // See method-level javadoc; we go into combining mode if there is no subquery. - final boolean combining = subqueryPlus == null; - GroupByQuery subquery = combining ? null : (GroupByQuery) subqueryPlus.getQuery(); - - GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); + final boolean combining = subquery == null; final List valueTypes = DimensionHandlerUtils.getValueTypesFromDimensionSpecs(query.getDimensions()); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); @@ -266,9 +264,6 @@ public static Pair, Accumulator limitSpec ); - final GroupByQueryMetrics groupByQueryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - Preconditions.checkNotNull(groupByQueryMetrics, "GroupByQueryMetrics"); - final Grouper grouper; if (concurrencyHint == -1) { grouper = new SpillingGrouper<>( @@ -284,8 +279,8 @@ public static Pair, Accumulator true, limitSpec, sortHasNonGroupingFields, - mergeBufferSize, - groupByQueryMetrics + mergeBufferSize + // TODO: Check if we need to pass a ResponseContext, or can retrieve them from the grouper method. ); } else { final Grouper.KeySerdeFactory combineKeySerdeFactory = new RowBasedKeySerdeFactory( @@ -315,7 +310,7 @@ public static Pair, Accumulator priority, hasQueryTimeout, queryTimeoutAt, - groupByQueryMetrics + new DefaultGroupByQueryMetrics() ); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java index 2dbe2a6a3d0c..f6ae3ee216c7 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java @@ -26,6 +26,7 @@ import com.google.common.base.Function; import com.google.common.base.Preconditions; import com.google.common.base.Supplier; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterators; import net.jpountz.lz4.LZ4BlockInputStream; import net.jpountz.lz4.LZ4BlockOutputStream; @@ -38,6 +39,7 @@ import org.apache.druid.query.aggregation.AggregatorAdapters; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.groupby.GroupByQueryMetrics; +import org.apache.druid.query.groupby.GroupByResponseContextKeys; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; import org.apache.druid.segment.ColumnSelectorFactory; @@ -53,6 +55,7 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Set; /** @@ -97,8 +100,7 @@ public SpillingGrouper( final boolean spillingAllowed, final DefaultLimitSpec limitSpec, final boolean sortHasNonGroupingFields, - final int mergeBufferSize, - final GroupByQueryMetrics groupByQueryMetrics + final int mergeBufferSize ) { this.keySerde = keySerdeFactory.factorize(); @@ -216,14 +218,14 @@ public void reset() } @Override - public void updateGroupByQueryMetrics() + public Map getQueryMetricsMap() { // TODO: If we do not want groupByQueryMetrics to do addition, simply return a Map, // then either the ConcurrentGrouper will aggregate the results into a accumulated map, // and the GroupByQueryMetrics will provide a means to report GroupByMetrics via a map. - if (groupByQueryMetrics != null) { - groupByQueryMetrics.mergeDictionarySize(keySerde.getDictionarySize()); - } + return ImmutableMap.of( + GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_NAME, keySerde.getDictionarySize() + ); } @Override From d0989ba9e6a39b80c31bd0ff1cb7a733f0dec080 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 15:30:56 +0800 Subject: [PATCH 22/39] SpillingGrouper will not see GroupByMetrics --- .../druid/query/groupby/epinephelinae/SpillingGrouper.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java index f6ae3ee216c7..03db301f2bd0 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java @@ -38,7 +38,6 @@ import org.apache.druid.query.BaseQuery; import org.apache.druid.query.aggregation.AggregatorAdapters; import org.apache.druid.query.aggregation.AggregatorFactory; -import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByResponseContextKeys; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; import org.apache.druid.segment.ColumnSelectorFactory; @@ -78,7 +77,6 @@ public class SpillingGrouper implements Grouper private final AggregatorFactory[] aggregatorFactories; private final Comparator> keyObjComparator; private final Comparator> defaultOrderKeyObjComparator; - private final GroupByQueryMetrics groupByQueryMetrics; private final List files = new ArrayList<>(); private final List dictionaryFiles = new ArrayList<>(); @@ -160,7 +158,6 @@ public SpillingGrouper( this.spillMapper = keySerde.decorateObjectMapper(spillMapper); this.spillingAllowed = spillingAllowed; this.sortHasNonGroupingFields = sortHasNonGroupingFields; - this.groupByQueryMetrics = groupByQueryMetrics; } @Override From 1b8725062819ed73f6a0e0a36e935d7a8fde73f0 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 15:32:57 +0800 Subject: [PATCH 23/39] ConcurrentGrouper will not see GroupByMetrics --- .../groupby/epinephelinae/ConcurrentGrouper.java | 14 +++----------- .../epinephelinae/RowBasedGrouperHelper.java | 6 +----- .../epinephelinae/ConcurrentGrouperTest.java | 6 ++---- 3 files changed, 6 insertions(+), 20 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java index 012b240699f6..d0cc6c8c3641 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java @@ -38,7 +38,6 @@ import org.apache.druid.query.QueryTimeoutException; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.groupby.GroupByQueryConfig; -import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; import org.apache.druid.segment.ColumnSelectorFactory; @@ -97,7 +96,6 @@ public class ConcurrentGrouper implements Grouper @Nullable private final ParallelCombiner parallelCombiner; private final boolean mergeThreadLocal; - private final GroupByQueryMetrics groupByQueryMetrics; private volatile boolean initialized = false; @@ -117,9 +115,7 @@ public ConcurrentGrouper( final ListeningExecutorService executor, final int priority, final boolean hasQueryTimeout, - final long queryTimeoutAt, - // TODO: To delete later? - final GroupByQueryMetrics groupByQueryMetrics + final long queryTimeoutAt ) { this( @@ -143,8 +139,7 @@ public ConcurrentGrouper( queryTimeoutAt, groupByQueryConfig.getIntermediateCombineDegree(), groupByQueryConfig.getNumParallelCombineThreads(), - groupByQueryConfig.isMergeThreadLocal(), - groupByQueryMetrics + groupByQueryConfig.isMergeThreadLocal() ); } @@ -169,9 +164,7 @@ public ConcurrentGrouper( final long queryTimeoutAt, final int intermediateCombineDegree, final int numParallelCombineThreads, - final boolean mergeThreadLocal, - // TODO: To delete if necessary. - final GroupByQueryMetrics groupByQueryMetrics + final boolean mergeThreadLocal ) { Preconditions.checkArgument(concurrencyHint > 0, "concurrencyHint > 0"); @@ -221,7 +214,6 @@ public ConcurrentGrouper( } this.mergeThreadLocal = mergeThreadLocal; - this.groupByQueryMetrics = groupByQueryMetrics; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java index 591972343bad..a90150379563 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java @@ -48,7 +48,6 @@ import org.apache.druid.query.ColumnSelectorPlus; import org.apache.druid.query.DimensionComparisonUtils; import org.apache.druid.query.DruidProcessingConfig; -import org.apache.druid.query.QueryPlus; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.GroupingAggregatorFactory; import org.apache.druid.query.dimension.ColumnSelectorStrategy; @@ -56,10 +55,8 @@ import org.apache.druid.query.dimension.DimensionSpec; import org.apache.druid.query.filter.Filter; import org.apache.druid.query.filter.ValueMatcher; -import org.apache.druid.query.groupby.DefaultGroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; -import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.epinephelinae.Grouper.BufferComparator; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; @@ -309,8 +306,7 @@ public static Pair, Accumulator grouperSorter, priority, hasQueryTimeout, - queryTimeoutAt, - new DefaultGroupByQueryMetrics() + queryTimeoutAt ); } diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java index 20029cf33efe..2ee8fb5e97ce 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java @@ -177,8 +177,7 @@ public void testAggregate() throws InterruptedException, ExecutionException, IOE 0, 4, parallelCombineThreads, - mergeThreadLocal, - new DefaultGroupByQueryMetrics() + mergeThreadLocal ); closer.register(grouper); grouper.init(); @@ -255,8 +254,7 @@ public void testGrouperTimeout() throws Exception 1, 4, parallelCombineThreads, - mergeThreadLocal, - groupByQueryMetrics + mergeThreadLocal ); closer.register(grouper); grouper.init(); From 8c2f007cd2d267fddd014b5576301394046ae570 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 16:23:53 +0800 Subject: [PATCH 24/39] Find place to emit metrics --- .../druid/query/DefaultQueryMetrics.java | 10 ---------- .../groupby/DefaultGroupByQueryMetrics.java | 14 +++---------- .../query/groupby/GroupByQueryMetrics.java | 3 --- .../groupby/GroupByQueryQueryToolChest.java | 20 +++++++++---------- .../GroupByMergingQueryRunner.java | 4 +++- .../LimitedTemporaryStorage.java | 4 ---- 6 files changed, 15 insertions(+), 40 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java index c7e0f1af2118..f3a4539f7808 100644 --- a/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/DefaultQueryMetrics.java @@ -464,16 +464,6 @@ protected QueryMetrics reportMetric(String metricName, Number value) return this; } - protected QueryMetrics reportMetricsIfNotZero(String metricName, Number value) - { - checkModifiedFromOwnerThread(); - if (value != null && !value.equals(0)) { - metrics.put(metricName, value); - } - - return this; - } - private QueryMetrics reportMillisTimeMetric(String metricName, long timeNs) { return reportMetric(metricName, TimeUnit.NANOSECONDS.toMillis(timeNs)); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index c2f9384166f0..ac027b74207a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -65,30 +65,22 @@ public void granularity(GroupByQuery query) //Don't emit by default } - @Override - public void reportGroupByStats() - { - reportMetricsIfNotZero("bytesSpilledToStorage", bytesSpilledToStorage.longValue()); - reportMetricsIfNotZero("mergeDictionarySize", mergeDictionarySize.longValue()); - reportMetricsIfNotZero("mergeBufferAcquisitonTime", mergeBufferAcquisitonTime.longValue()); - } - @Override public void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime) { - this.mergeBufferAcquisitonTime.add(mergeBufferAcquisitionTime); + reportMetric("mergeBufferAcquisitonTime", mergeBufferAcquisitionTime); } @Override public void bytesSpilledToStorage(long bytesSpilledToStorage) { - this.bytesSpilledToStorage.add(bytesSpilledToStorage); + reportMetric("bytesSpilledToStorage", bytesSpilledToStorage); } @Override public void mergeDictionarySize(long mergeDictionarySize) { - this.mergeDictionarySize.add(mergeDictionarySize); + reportMetric("mergeDictionarySize", mergeDictionarySize); } @Override diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java index 80e5d4af0193..f04bc811f5d0 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java @@ -55,9 +55,6 @@ public interface GroupByQueryMetrics extends QueryMetrics @PublicApi void granularity(GroupByQuery query); - @PublicApi - void reportGroupByStats(); - @PublicApi void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index dd8d838abcd7..63c5722d7c93 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -162,8 +162,6 @@ private Sequence initAndMergeGroupByResults( ) { final GroupByQuery query = (GroupByQuery) queryPlus.getQuery(); - - // TODO: Append the GroupByQueryMetrics using the ResponseContext. boolean reportMetricsForEmission = queryPlus.getQueryMetrics() != null; // Reserve the group by resources (merge buffers) required for executing the query @@ -185,17 +183,17 @@ private Sequence initAndMergeGroupByResults( final Sequence mergedSequence = mergeGroupByResults(query, resource, runner, context, closer); + // TODO: Check if need wrap in closer or no? + if (reportMetricsForEmission) { + GroupByQueryMetrics queryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + queryMetrics.bytesSpilledToStorage((Long) context.get(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY)); + queryMetrics.mergeDictionarySize((Long) context.get(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY)); + queryMetrics.mergeBufferAcquisitionTime((Long) context.get(GroupByResponseContextKeys.GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_KEY)); + + } + // Clean up the resources reserved during the execution of the query closer.register(() -> groupByResourcesReservationPool.clean(queryResourceId)); - closer.register(() -> { - if (reportMetricsForEmission) { - GroupByQueryMetrics queryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - // TODO: Populate the metrics here. - } - }); - - // TODO: Maybe attach metrics reporting with the closer? - // closer.register(() -> groupByStatsProvider.closeQuery(query.context().getQueryResourceId())); return Sequences.withBaggage(mergedSequence, closer); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index b8add63b1d40..79c76e51a301 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -305,11 +305,13 @@ public AggregateResult call() waitForFutureCompletion(query, futures, hasTimeout, timeoutAt - System.currentTimeMillis()); } + // Finished the query, so let's collate the metrics! Map metricsMap = grouper.getQueryMetricsMap(); if (responseContext != null) { responseContext.add(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY, temporaryStorage.currentSize()); - responseContext.add(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY, metricsMap.getOrDefault(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_NAME, 0L)); + responseContext.add(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY, + metricsMap.getOrDefault(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_NAME, 0L)); } return RowBasedGrouperHelper.makeGrouperIterator(grouper, query, resources); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java index 20e9f9a973f9..7ce746159130 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java @@ -140,10 +140,6 @@ public void close() return; } closed = true; - - if (groupByQueryMetrics != null) { - groupByQueryMetrics.bytesSpilledToStorage(bytesUsed.get()); - } bytesUsed.set(0); for (File file : ImmutableSet.copyOf(files)) { From 1beb2b1f19f808bc6c5e721a3c8881e22c613d19 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 16:27:23 +0800 Subject: [PATCH 25/39] GroupByStatsProvider in right place --- .../query/groupby/GroupByQueryQueryToolChest.java | 11 +++++++---- .../apache/druid/query/groupby/GroupingEngine.java | 8 ++------ .../epinephelinae/GroupByMergingQueryRunner.java | 6 +----- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 63c5722d7c93..bec53477eccb 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -95,6 +95,7 @@ public class GroupByQueryQueryToolChest extends QueryToolChest queryConfigSupplier, GroupByQueryMetricsFactory queryMetricsFactory, - @Merging GroupByResourcesReservationPool groupByResourcesReservationPool + @Merging GroupByResourcesReservationPool groupByResourcesReservationPool, + GroupByStatsProvider groupByStatsProvider ) { this.groupingEngine = groupingEngine; this.queryConfig = queryConfigSupplier.get(); this.queryMetricsFactory = queryMetricsFactory; this.groupByResourcesReservationPool = groupByResourcesReservationPool; + this.groupByStatsProvider = groupByStatsProvider; } @Override @@ -183,13 +187,12 @@ private Sequence initAndMergeGroupByResults( final Sequence mergedSequence = mergeGroupByResults(query, resource, runner, context, closer); - // TODO: Check if need wrap in closer or no? if (reportMetricsForEmission) { GroupByQueryMetrics queryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); queryMetrics.bytesSpilledToStorage((Long) context.get(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY)); queryMetrics.mergeDictionarySize((Long) context.get(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY)); queryMetrics.mergeBufferAcquisitionTime((Long) context.get(GroupByResponseContextKeys.GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_KEY)); - + groupByStatsProvider.aggregateStats(queryMetrics); } // Clean up the resources reserved during the execution of the query diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 34920fed35bf..c6b7c932066a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -120,7 +120,6 @@ public class GroupingEngine private final ObjectMapper jsonMapper; private final ObjectMapper spillMapper; private final QueryWatcher queryWatcher; - private final GroupByStatsProvider groupByStatsProvider; @Inject public GroupingEngine( @@ -129,8 +128,7 @@ public GroupingEngine( @Merging GroupByResourcesReservationPool groupByResourcesReservationPool, @Json ObjectMapper jsonMapper, @Smile ObjectMapper spillMapper, - QueryWatcher queryWatcher, - GroupByStatsProvider groupByStatsProvider + QueryWatcher queryWatcher ) { this.processingConfig = processingConfig; @@ -139,7 +137,6 @@ public GroupingEngine( this.jsonMapper = jsonMapper; this.spillMapper = spillMapper; this.queryWatcher = queryWatcher; - this.groupByStatsProvider = groupByStatsProvider; } /** @@ -454,8 +451,7 @@ public QueryRunner mergeRunners( processingConfig.getNumThreads(), processingConfig.intermediateComputeSizeBytes(), spillMapper, - processingConfig.getTmpDir(), - groupByStatsProvider + processingConfig.getTmpDir() ); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index 79c76e51a301..ba6183fa3bea 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -60,7 +60,6 @@ import org.apache.druid.query.groupby.GroupByQueryResources; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; import org.apache.druid.query.groupby.GroupByResponseContextKeys; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.epinephelinae.RowBasedGrouperHelper.RowBasedKey; @@ -106,7 +105,6 @@ public class GroupByMergingQueryRunner implements QueryRunner private final ObjectMapper spillMapper; private final String processingTmpDir; private final int mergeBufferSize; - private final GroupByStatsProvider groupByStatsProvider; public GroupByMergingQueryRunner( GroupByQueryConfig config, @@ -118,8 +116,7 @@ public GroupByMergingQueryRunner( int concurrencyHint, int mergeBufferSize, ObjectMapper spillMapper, - String processingTmpDir, - GroupByStatsProvider groupByStatsProvider + String processingTmpDir ) { this.config = config; @@ -132,7 +129,6 @@ public GroupByMergingQueryRunner( this.spillMapper = spillMapper; this.processingTmpDir = processingTmpDir; this.mergeBufferSize = mergeBufferSize; - this.groupByStatsProvider = groupByStatsProvider; } @Override From 2532a84e581ab36179e23bb2b0ff18ac623fa26b Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 17:06:08 +0800 Subject: [PATCH 26/39] Unify method signatures and usage across all files --- .../benchmark/GroupByDeserializationBenchmark.java | 1 + .../druid/benchmark/GroupByTypeInterfaceBenchmark.java | 5 +---- .../query/CachingClusteredClientBenchmark.java | 5 +---- .../apache/druid/benchmark/query/GroupByBenchmark.java | 5 +---- .../druid/segment/MapVirtualColumnGroupByTest.java | 5 +---- .../druid/segment/DatasketchesProjectionTest.java | 4 +--- .../query/groupby/GroupByQueryQueryToolChest.java | 3 --- .../query/groupby/GroupByResponseContextKeys.java | 2 +- .../groupby/epinephelinae/LimitedTemporaryStorage.java | 3 --- .../GroupByLimitPushDownInsufficientBufferTest.java | 8 ++------ .../GroupByLimitPushDownMultiNodeMergeTest.java | 10 +++------- .../druid/query/groupby/GroupByMultiSegmentTest.java | 4 +--- .../query/groupby/GroupByQueryMergeBufferTest.java | 4 +--- .../query/groupby/GroupByQueryQueryToolChestTest.java | 5 ++--- .../query/groupby/GroupByQueryRunnerFailureTest.java | 4 +--- .../druid/query/groupby/GroupByQueryRunnerTest.java | 6 +++--- .../druid/query/groupby/NestedQueryPushDownTest.java | 9 +++------ .../query/groupby/UnnestGroupByQueryRunnerTest.java | 4 +--- .../druid/segment/CursorFactoryProjectionTest.java | 10 +--------- .../apache/druid/segment/CursorHolderPreaggTest.java | 5 +---- .../incremental/IncrementalIndexCursorFactoryTest.java | 4 +--- .../druid/sql/calcite/util/SqlTestFramework.java | 5 +---- 22 files changed, 28 insertions(+), 83 deletions(-) diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java index a907c99dec29..1baa5ac9be09 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByDeserializationBenchmark.java @@ -112,6 +112,7 @@ public boolean isIntermediateResultAsMapCompat() } }, null, + null, null ); diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByTypeInterfaceBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByTypeInterfaceBenchmark.java index 300841fab5d7..2950e70de1ea 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByTypeInterfaceBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/GroupByTypeInterfaceBenchmark.java @@ -54,7 +54,6 @@ import org.apache.druid.query.groupby.GroupByQueryQueryToolChest; import org.apache.druid.query.groupby.GroupByQueryRunnerFactory; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.spec.MultipleIntervalSegmentSpec; @@ -369,7 +368,6 @@ public String getFormatString() }; final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(mergePool, config); final GroupingEngine groupingEngine = new GroupingEngine( @@ -378,8 +376,7 @@ public String getFormatString() groupByResourcesReservationPool, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - QueryBenchmarkUtil.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryBenchmarkUtil.NOOP_QUERYWATCHER ); factory = new GroupByQueryRunnerFactory( diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/query/CachingClusteredClientBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/query/CachingClusteredClientBenchmark.java index 2a3aede2a904..63e55e85adf3 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/query/CachingClusteredClientBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/query/CachingClusteredClientBenchmark.java @@ -81,7 +81,6 @@ import org.apache.druid.query.groupby.GroupByQueryRunnerFactory; import org.apache.druid.query.groupby.GroupByQueryRunnerTest; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.spec.MultipleIntervalSegmentSpec; @@ -346,7 +345,6 @@ private static GroupByQueryRunnerFactory makeGroupByQueryRunnerFactory( bufferSupplier, processingConfig.getNumMergeBuffers() ); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(mergeBufferPool, config); final GroupingEngine groupingEngine = new GroupingEngine( @@ -355,8 +353,7 @@ private static GroupByQueryRunnerFactory makeGroupByQueryRunnerFactory( groupByResourcesReservationPool, mapper, mapper, - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); final GroupByQueryQueryToolChest toolChest = new GroupByQueryQueryToolChest(groupingEngine, groupByResourcesReservationPool); return new GroupByQueryRunnerFactory(groupingEngine, toolChest, bufferPool); diff --git a/benchmarks/src/test/java/org/apache/druid/benchmark/query/GroupByBenchmark.java b/benchmarks/src/test/java/org/apache/druid/benchmark/query/GroupByBenchmark.java index 2056ffbea0d3..4e22333d4e9c 100644 --- a/benchmarks/src/test/java/org/apache/druid/benchmark/query/GroupByBenchmark.java +++ b/benchmarks/src/test/java/org/apache/druid/benchmark/query/GroupByBenchmark.java @@ -65,7 +65,6 @@ import org.apache.druid.query.groupby.GroupByQueryQueryToolChest; import org.apache.druid.query.groupby.GroupByQueryRunnerFactory; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; @@ -486,7 +485,6 @@ public String getFormatString() }; final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(mergePool, config); final GroupingEngine groupingEngine = new GroupingEngine( @@ -495,8 +493,7 @@ public String getFormatString() groupByResourcesReservationPool, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - QueryBenchmarkUtil.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryBenchmarkUtil.NOOP_QUERYWATCHER ); factory = new GroupByQueryRunnerFactory( diff --git a/extensions-contrib/virtual-columns/src/test/java/org/apache/druid/segment/MapVirtualColumnGroupByTest.java b/extensions-contrib/virtual-columns/src/test/java/org/apache/druid/segment/MapVirtualColumnGroupByTest.java index 46248da5a7ec..dfbc3654d949 100644 --- a/extensions-contrib/virtual-columns/src/test/java/org/apache/druid/segment/MapVirtualColumnGroupByTest.java +++ b/extensions-contrib/virtual-columns/src/test/java/org/apache/druid/segment/MapVirtualColumnGroupByTest.java @@ -44,7 +44,6 @@ import org.apache.druid.query.groupby.GroupByQueryQueryToolChest; import org.apache.druid.query.groupby.GroupByQueryRunnerFactory; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.spec.MultipleIntervalSegmentSpec; @@ -74,7 +73,6 @@ public void setup() throws IOException final BlockingPool mergePool = new DefaultBlockingPool<>(() -> ByteBuffer.allocate(1024), 1); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(mergePool, config); @@ -110,8 +108,7 @@ public int getNumThreads() groupByResourcesReservationPool, TestHelper.makeJsonMapper(), new DefaultObjectMapper(), - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); final GroupByQueryRunnerFactory factory = new GroupByQueryRunnerFactory( diff --git a/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java b/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java index 75c7c4a5a1ba..374518f3ef33 100644 --- a/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java +++ b/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java @@ -62,7 +62,6 @@ import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.segment.column.ColumnType; @@ -279,8 +278,7 @@ public DatasketchesProjectionTest( TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), (query, future) -> { - }, - new GroupByStatsProvider() + } ); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index bec53477eccb..163afc0fe1c8 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -266,9 +266,6 @@ private Sequence mergeGroupByResultsWithoutPushDown( throw new UnsupportedOperationException("Subqueries must be of type 'group by'"); } - // TODO: Check if we need to do a fitting action here. - // closer.register(() -> groupByStatsProvider.closeQuery(subquery.context().getQueryResourceId())); - final Sequence subqueryResult = mergeGroupByResults( subquery, resource, diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java index 0ac6d7e6c85d..0afc8fd2315c 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java @@ -10,7 +10,7 @@ * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, - * software distributed under this License is distributed on an + * 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 diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java index 7ce746159130..137095a50542 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java @@ -25,7 +25,6 @@ import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.logger.Logger; -import org.apache.druid.query.groupby.GroupByQueryMetrics; import java.io.Closeable; import java.io.File; @@ -48,8 +47,6 @@ public class LimitedTemporaryStorage implements Closeable { private static final Logger log = new Logger(LimitedTemporaryStorage.class); -// private final GroupByQueryMetrics groupByQueryMetrics; - private final File storageDirectory; private final long maxBytesUsed; diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownInsufficientBufferTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownInsufficientBufferTest.java index 9e4706d44f58..7f223a2640b9 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownInsufficientBufferTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownInsufficientBufferTest.java @@ -326,8 +326,6 @@ public String getFormatString() final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); - final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool( mergePool, config @@ -342,8 +340,7 @@ public String getFormatString() groupByResourcesReservationPool, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); final GroupingEngine tooSmallEngine = new GroupingEngine( @@ -352,8 +349,7 @@ public String getFormatString() tooSmallGroupByResourcesReservationPool, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); groupByFactory = new GroupByQueryRunnerFactory( diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownMultiNodeMergeTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownMultiNodeMergeTest.java index df03020a8acf..5ab4ff05369f 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownMultiNodeMergeTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByLimitPushDownMultiNodeMergeTest.java @@ -578,7 +578,6 @@ public String getFormatString() }; final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPoolBroker = new GroupByResourcesReservationPool(mergePoolBroker, config); @@ -593,8 +592,7 @@ public String getFormatString() groupByResourcesReservationPoolBroker, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); final GroupingEngine groupingEngineHistorical = new GroupingEngine( druidProcessingConfig, @@ -602,8 +600,7 @@ public String getFormatString() groupByResourcesReservationPoolHistorical, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); final GroupingEngine groupingEngineHistorical2 = new GroupingEngine( druidProcessingConfig, @@ -611,8 +608,7 @@ public String getFormatString() groupByResourcesReservationPoolHistorical2, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); groupByFactoryBroker = new GroupByQueryRunnerFactory( diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByMultiSegmentTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByMultiSegmentTest.java index bb0cb89d69dd..1039e40ccb9a 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByMultiSegmentTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByMultiSegmentTest.java @@ -241,7 +241,6 @@ public String getFormatString() }; final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(mergePool, config); final GroupingEngine groupingEngine = new GroupingEngine( @@ -250,8 +249,7 @@ public String getFormatString() groupByResourcesReservationPool, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); groupByFactory = new GroupByQueryRunnerFactory( diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryMergeBufferTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryMergeBufferTest.java index 775a8c602bbe..77f116aa6e40 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryMergeBufferTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryMergeBufferTest.java @@ -124,7 +124,6 @@ private static GroupByQueryRunnerFactory makeQueryRunnerFactory( ) { final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(MERGE_BUFFER_POOL, config); @@ -134,8 +133,7 @@ private static GroupByQueryRunnerFactory makeQueryRunnerFactory( groupByResourcesReservationPool, TestHelper.makeJsonMapper(), mapper, - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); final GroupByQueryQueryToolChest toolChest = new GroupByQueryQueryToolChest( groupingEngine, diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java index 258c06a06874..2cb156edf826 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChestTest.java @@ -799,6 +799,7 @@ public boolean isIntermediateResultAsMapCompat() } }, null, + null, null ); @@ -1293,7 +1294,6 @@ public String getFormatString() bufferSupplier, processingConfig.getNumMergeBuffers() ); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(mergeBufferPool, queryConfig); final GroupingEngine groupingEngine = new GroupingEngine( @@ -1302,8 +1302,7 @@ public String getFormatString() groupByResourcesReservationPool, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); final GroupByQueryQueryToolChest queryToolChest = new GroupByQueryQueryToolChest(groupingEngine, groupByResourcesReservationPool); final ObjectMapper mapper = TestHelper.makeJsonMapper(); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerFailureTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerFailureTest.java index 2e52da567b66..13475eef9e82 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerFailureTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerFailureTest.java @@ -105,7 +105,6 @@ private static GroupByQueryRunnerFactory makeQueryRunnerFactory( ) { final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(MERGE_BUFFER_POOL, config); final GroupingEngine groupingEngine = new GroupingEngine( @@ -114,8 +113,7 @@ private static GroupByQueryRunnerFactory makeQueryRunnerFactory( groupByResourcesReservationPool, TestHelper.makeJsonMapper(), mapper, - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); final GroupByQueryQueryToolChest toolChest = new GroupByQueryQueryToolChest(groupingEngine, groupByResourcesReservationPool); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java index fe5b4dbb2daa..b648890ca01d 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java @@ -398,14 +398,14 @@ public static GroupByQueryRunnerFactory makeQueryRunnerFactory( groupByResourcesReservationPool, mapper, mapper, - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - statsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); final GroupByQueryQueryToolChest toolChest = new GroupByQueryQueryToolChest( groupingEngine, () -> config, DefaultGroupByQueryMetricsFactory.instance(), - groupByResourcesReservationPool + groupByResourcesReservationPool, + statsProvider ); return new GroupByQueryRunnerFactory(groupingEngine, toolChest, bufferPools.getProcessingPool()); } diff --git a/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java b/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java index 9ca463ccdaa3..ea452f8750f6 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/NestedQueryPushDownTest.java @@ -287,7 +287,6 @@ public String getFormatString() }; final Supplier configSupplier = Suppliers.ofInstance(config); - final GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); final GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(mergePool, config); final GroupByResourcesReservationPool groupByResourcesReservationPool2 = @@ -298,8 +297,7 @@ public String getFormatString() groupByResourcesReservationPool, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); final GroupingEngine engine2 = new GroupingEngine( druidProcessingConfig, @@ -307,8 +305,7 @@ public String getFormatString() groupByResourcesReservationPool2, TestHelper.makeJsonMapper(), new ObjectMapper(new SmileFactory()), - NOOP_QUERYWATCHER, - groupByStatsProvider + NOOP_QUERYWATCHER ); groupByFactory = new GroupByQueryRunnerFactory( @@ -763,7 +760,7 @@ private Sequence runNestedQueryWithForcePushDown(GroupByQuery nestedQ )); Sequence pushDownQueryResults = groupingEngine.mergeResults( queryRunnerForSegments, - QueryPlus.wrap(GroupByQueryRunnerTestHelper.populateResourceId(queryWithPushDownDisabled)), + (GroupByQuery) GroupByQueryRunnerTestHelper.populateResourceId(queryWithPushDownDisabled), context ); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/UnnestGroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/UnnestGroupByQueryRunnerTest.java index b220f00b05a3..59b539de3caa 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/UnnestGroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/UnnestGroupByQueryRunnerTest.java @@ -172,7 +172,6 @@ public static GroupByQueryRunnerFactory makeQueryRunnerFactory( ); } final Supplier configSupplier = Suppliers.ofInstance(config); - GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); GroupByResourcesReservationPool groupByResourcesReservationPool = new GroupByResourcesReservationPool(bufferPools.getMergePool(), config); final GroupingEngine groupingEngine = new GroupingEngine( @@ -181,8 +180,7 @@ public static GroupByQueryRunnerFactory makeQueryRunnerFactory( groupByResourcesReservationPool, TestHelper.makeJsonMapper(), mapper, - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - groupByStatsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); final GroupByQueryQueryToolChest toolChest = new GroupByQueryQueryToolChest(groupingEngine, groupByResourcesReservationPool); diff --git a/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java b/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java index 0a12b8181e76..d5e2d83f183e 100644 --- a/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java +++ b/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java @@ -71,7 +71,6 @@ import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.orderby.DefaultLimitSpec; @@ -583,8 +582,7 @@ public CursorFactoryProjectionTest( TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), (query, future) -> { - }, - new GroupByStatsProvider() + } ); this.timeseriesEngine = new TimeseriesQueryEngine(nonBlockingPool); } @@ -2273,12 +2271,6 @@ public void granularity(GroupByQuery query) // no-op for projection tests } - @Override - public void reportGroupByStats() - { - // no-op for projection tests - } - @Override public void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime) { diff --git a/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java b/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java index 646bc97033fd..e9f9149b9b93 100644 --- a/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java +++ b/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java @@ -35,7 +35,6 @@ import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.timeseries.TimeseriesQuery; @@ -93,7 +92,6 @@ public void setup() () -> ByteBuffer.allocate(50000), 4 ); - GroupByStatsProvider groupByStatsProvider = new GroupByStatsProvider(); groupingEngine = new GroupingEngine( new DruidProcessingConfig(), GroupByQueryConfig::new, @@ -104,8 +102,7 @@ public void setup() TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), (query, future) -> { - }, - groupByStatsProvider + } ); this.cursorFactory = new CursorFactory() diff --git a/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java b/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java index 9a447dd39e0c..11ceb249e9bd 100644 --- a/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java +++ b/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java @@ -55,7 +55,6 @@ import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.topn.TopNQueryBuilder; @@ -176,8 +175,7 @@ public IncrementalIndexCursorFactoryTest(String indexType, boolean sortByDim) TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), (query, future) -> { - }, - new GroupByStatsProvider() + } ); topnQueryEngine = new TopNQueryEngine(nonBlockingPool); } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java index 38ddba751b5d..bd7d72f44df6 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/util/SqlTestFramework.java @@ -71,7 +71,6 @@ import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByQueryMetricsFactory; import org.apache.druid.query.groupby.GroupByResourcesReservationPool; -import org.apache.druid.query.groupby.GroupByStatsProvider; import org.apache.druid.query.groupby.GroupingEngine; import org.apache.druid.query.groupby.TestGroupByBuffers; import org.apache.druid.query.lookup.LookupExtractorFactoryContainerProvider; @@ -961,7 +960,6 @@ private GroupByResourcesReservationPool makeGroupByResourcesReservationPool( private GroupingEngine makeGroupingEngine( final ObjectMapper mapper, final DruidProcessingConfig processingConfig, - final GroupByStatsProvider statsProvider, final GroupByQueryConfig config, final GroupByResourcesReservationPool groupByResourcesReservationPool ) @@ -973,8 +971,7 @@ private GroupingEngine makeGroupingEngine( groupByResourcesReservationPool, mapper, mapper, - QueryRunnerTestHelper.NOOP_QUERYWATCHER, - statsProvider + QueryRunnerTestHelper.NOOP_QUERYWATCHER ); } From 6cdfef97cf7ed336c553c15550cd508ebdd69629 Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 17:39:29 +0800 Subject: [PATCH 27/39] Track grouper + temporaryStorage in GroupByRowProcessor --- .../groupby/GroupByQueryQueryToolChest.java | 6 ++++-- .../druid/query/groupby/GroupingEngine.java | 7 ++++++- .../groupby/epinephelinae/ConcurrentGrouper.java | 4 ++-- .../epinephelinae/GroupByMergingQueryRunner.java | 5 ++--- .../epinephelinae/GroupByRowProcessor.java | 12 +++++++++++- .../epinephelinae/LimitedTemporaryStorage.java | 16 ++++++++-------- .../epinephelinae/RowBasedGrouperHelper.java | 1 - .../groupby/epinephelinae/SpillingGrouper.java | 4 ---- 8 files changed, 33 insertions(+), 22 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 163afc0fe1c8..3388d501aef2 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -238,7 +238,7 @@ private Sequence mergeGroupByResultsWithoutPushDown( return groupingEngine.applyPostProcessing(mergedResults, query); } else { Sequence mergedResults = groupingEngine.mergeResults(runner, query.withSubtotalsSpec(null), context); - return groupingEngine.processSubtotalsSpec(query, resource, mergedResults); + return groupingEngine.processSubtotalsSpec(query, resource, mergedResults, context); } } @@ -279,6 +279,7 @@ private Sequence mergeGroupByResultsWithoutPushDown( subquery, query, resource, + context, finalizingResults, false ); @@ -286,7 +287,7 @@ private Sequence mergeGroupByResultsWithoutPushDown( if (query.getSubtotalsSpec() == null) { return groupingEngine.applyPostProcessing(processedSubqueryResults, query); } else { - return groupingEngine.processSubtotalsSpec(query, resource, processedSubqueryResults); + return groupingEngine.processSubtotalsSpec(query, resource, processedSubqueryResults, context); } } @@ -305,6 +306,7 @@ private Sequence mergeResultsWithNestedQueryPushDown( query, rewriteNestedQueryForPushDown(query), resource, + context, finalizedResults, true ); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index c6b7c932066a..1d74c78b0eb9 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -583,6 +583,7 @@ public Sequence processSubqueryResult( GroupByQuery subquery, GroupByQuery query, GroupByQueryResources resource, + ResponseContext context, Sequence subqueryResult, boolean wasQueryPushedDown ) @@ -616,6 +617,7 @@ public Sequence processSubqueryResult( configSupplier.get(), processingConfig, resource, + context, spillMapper, processingConfig.getTmpDir(), processingConfig.intermediateComputeSizeBytes() @@ -648,7 +650,8 @@ public Sequence processSubqueryResult( public Sequence processSubtotalsSpec( GroupByQuery query, GroupByQueryResources resource, - Sequence queryResult + Sequence queryResult, + ResponseContext context ) { // How it works? @@ -697,6 +700,7 @@ public Sequence processSubtotalsSpec( configSupplier.get(), processingConfig, resource, + context, spillMapper, processingConfig.getTmpDir(), processingConfig.intermediateComputeSizeBytes() @@ -758,6 +762,7 @@ public Sequence processSubtotalsSpec( configSupplier.get(), processingConfig, resource, + context, spillMapper, processingConfig.getTmpDir(), processingConfig.intermediateComputeSizeBytes() diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java index d0cc6c8c3641..7d5aa30316be 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java @@ -497,8 +497,8 @@ public Map getQueryMetricsMap() { Map map = new HashMap<>(); - groupers.forEach(spillingGroupper -> { - Map metricsMap = spillingGroupper.getQueryMetricsMap(); + groupers.forEach(spillingGrouper -> { + Map metricsMap = spillingGrouper.getQueryMetricsMap(); for (Map.Entry entry : metricsMap.entrySet()) { map.compute(entry.getKey(), (key, value) -> value == null ? entry.getValue() : value + entry.getValue()); diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java index ba6183fa3bea..b9c751f3b0c6 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByMergingQueryRunner.java @@ -147,7 +147,6 @@ public Sequence run(final QueryPlus queryPlus, final Respo false ); - // TODO: Check will this give me NPE down the road when accessing QueryMetrics from the queryPlus? final QueryPlus queryPlusForRunners = queryPlus .withQuery( query.withOverriddenContext(ImmutableMap.of(CTX_KEY_MERGE_RUNNERS_USING_CHAINED_EXECUTION, true)) @@ -302,10 +301,10 @@ public AggregateResult call() } // Finished the query, so let's collate the metrics! - Map metricsMap = grouper.getQueryMetricsMap(); - if (responseContext != null) { responseContext.add(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY, temporaryStorage.currentSize()); + + Map metricsMap = grouper.getQueryMetricsMap(); responseContext.add(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY, metricsMap.getOrDefault(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_NAME, 0L)); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java index 4dfe37c79c14..38020284654a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/GroupByRowProcessor.java @@ -31,10 +31,12 @@ import org.apache.druid.java.util.common.io.Closer; import org.apache.druid.query.DruidProcessingConfig; import org.apache.druid.query.ResourceLimitExceededException; +import org.apache.druid.query.context.ResponseContext; import org.apache.druid.query.dimension.DimensionSpec; import org.apache.druid.query.groupby.GroupByQuery; import org.apache.druid.query.groupby.GroupByQueryConfig; import org.apache.druid.query.groupby.GroupByQueryResources; +import org.apache.druid.query.groupby.GroupByResponseContextKeys; import org.apache.druid.query.groupby.ResultRow; import org.apache.druid.query.groupby.epinephelinae.RowBasedGrouperHelper.RowBasedKey; @@ -44,6 +46,7 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.util.List; +import java.util.Map; import java.util.UUID; /** @@ -92,6 +95,7 @@ public static ResultSupplier process( final GroupByQueryConfig config, final DruidProcessingConfig processingConfig, final GroupByQueryResources resource, + final ResponseContext context, final ObjectMapper spillMapper, final String processingTmpDir, final int mergeBufferSize @@ -142,7 +146,13 @@ public ByteBuffer get() throw new ResourceLimitExceededException(retVal.getReason()); } - grouper.getQueryMetricsMap(); + if (context != null) { + context.add(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY, temporaryStorage.currentSize()); + + Map metricsMap = grouper.getQueryMetricsMap(); + context.add(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY, + metricsMap.getOrDefault(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_NAME, 0L)); + } return new ResultSupplier() { diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java index 137095a50542..a283f1e1bee4 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java @@ -48,7 +48,7 @@ public class LimitedTemporaryStorage implements Closeable private static final Logger log = new Logger(LimitedTemporaryStorage.class); private final File storageDirectory; - private final long maxBytesUsed; + private final long capacity; private final AtomicLong bytesUsed = new AtomicLong(); private final Set files = new TreeSet<>(); @@ -59,11 +59,11 @@ public class LimitedTemporaryStorage implements Closeable public LimitedTemporaryStorage( File storageDirectory, - long maxBytesUsed + long capacity ) { this.storageDirectory = storageDirectory; - this.maxBytesUsed = maxBytesUsed; + this.capacity = capacity; } /** @@ -77,8 +77,8 @@ public LimitedTemporaryStorage( */ public LimitedOutputStream createFile() throws IOException { - if (bytesUsed.get() >= maxBytesUsed) { - throw new TemporaryStorageFullException(maxBytesUsed); + if (bytesUsed.get() >= capacity) { + throw new TemporaryStorageFullException(capacity); } synchronized (files) { @@ -120,7 +120,7 @@ public void delete(final File file) public long maxSize() { - return maxBytesUsed; + return capacity; } @VisibleForTesting @@ -200,8 +200,8 @@ public File getFile() private void grab(int n) throws IOException { - if (bytesUsed.addAndGet(n) > maxBytesUsed) { - throw new TemporaryStorageFullException(maxBytesUsed); + if (bytesUsed.addAndGet(n) > capacity) { + throw new TemporaryStorageFullException(capacity); } } } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java index a90150379563..4a02f7118787 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java @@ -277,7 +277,6 @@ public static Pair, Accumulator limitSpec, sortHasNonGroupingFields, mergeBufferSize - // TODO: Check if we need to pass a ResponseContext, or can retrieve them from the grouper method. ); } else { final Grouper.KeySerdeFactory combineKeySerdeFactory = new RowBasedKeySerdeFactory( diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java index 03db301f2bd0..7afa58daec39 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java @@ -217,9 +217,6 @@ public void reset() @Override public Map getQueryMetricsMap() { - // TODO: If we do not want groupByQueryMetrics to do addition, simply return a Map, - // then either the ConcurrentGrouper will aggregate the results into a accumulated map, - // and the GroupByQueryMetrics will provide a means to report GroupByMetrics via a map. return ImmutableMap.of( GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_NAME, keySerde.getDictionarySize() ); @@ -228,7 +225,6 @@ public Map getQueryMetricsMap() @Override public void close() { - // TODO: Check if it is ok to emit at close. Will the QueryRunners give the results in time? grouper.close(); keySerde.reset(); deleteFiles(); From 05c1f2b7311e4bb3745f16bc7abdabf120dc827a Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 17:43:13 +0800 Subject: [PATCH 28/39] Javadocs for GroupingEngine --- .../main/java/org/apache/druid/query/groupby/GroupingEngine.java | 1 + 1 file changed, 1 insertion(+) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 1d74c78b0eb9..c4f6030ad737 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -644,6 +644,7 @@ public Sequence processSubqueryResult( * @param query query that has a "subtotalsSpec" * @param resource resources returned by {@link #prepareResource(GroupByQuery, BlockingPool, boolean, GroupByQueryConfig)} * @param queryResult result rows from the main query + * @param context response context for collating query metrics * * @return results for each list of subtotals in the query, concatenated together */ From cda3e516c6a5317c486c12dc5e8eddf01c01e2cd Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 18:21:26 +0800 Subject: [PATCH 29/39] If responseContext keys are null..... --- .../query/groupby/GroupByQueryMetrics.java | 2 +- .../groupby/GroupByQueryQueryToolChest.java | 25 ++++++++++++++++--- .../groupby/GroupByResponseContextKeys.java | 22 +++++++++++++--- 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java index f04bc811f5d0..c3aac1fdb415 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java @@ -42,7 +42,7 @@ public interface GroupByQueryMetrics extends QueryMetrics void numMetrics(GroupByQuery query); /** - * Sets the number of "complex" metrics of the given groupBy query as dimension. By default it is assumed that + * Sets the number of "complex" metrics of the given groupBy query as dimension. By default, it is assumed that * "complex" metric is a metric of not long or double type, but it could be redefined in the implementation of this * method. */ diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 3388d501aef2..c712ec1ec79f 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -189,9 +189,7 @@ private Sequence initAndMergeGroupByResults( if (reportMetricsForEmission) { GroupByQueryMetrics queryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - queryMetrics.bytesSpilledToStorage((Long) context.get(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY)); - queryMetrics.mergeDictionarySize((Long) context.get(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY)); - queryMetrics.mergeBufferAcquisitionTime((Long) context.get(GroupByResponseContextKeys.GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_KEY)); + populateQueryMetrics(queryMetrics, context); groupByStatsProvider.aggregateStats(queryMetrics); } @@ -207,6 +205,27 @@ private Sequence initAndMergeGroupByResults( } } + private void populateQueryMetrics(GroupByQueryMetrics queryMetrics, ResponseContext context) + { + Object bytesSpilledToStorage = context.get(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY); + + if (bytesSpilledToStorage != null) { + queryMetrics.bytesSpilledToStorage((Long) bytesSpilledToStorage); + } + + Object mergeDictionarySize = context.get(GroupByResponseContextKeys.GROUPBY_MERGE_DICTIONARY_SIZE_KEY); + + if (mergeDictionarySize != null) { + queryMetrics.mergeDictionarySize((Long) mergeDictionarySize); + } + + Object mergeBufferAcquisitionTime = context.get(GroupByResponseContextKeys.GROUPBY_MERGE_BUFFER_ACQUISITION_TIME_KEY); + + if (mergeBufferAcquisitionTime != null) { + queryMetrics.mergeBufferAcquisitionTime((Long) mergeBufferAcquisitionTime); + } + } + private Sequence mergeGroupByResults( final GroupByQuery query, GroupByQueryResources resource, diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java index 0afc8fd2315c..101322ab00f1 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByResponseContextKeys.java @@ -21,6 +21,8 @@ import org.apache.druid.query.context.ResponseContext; +import javax.annotation.Nullable; + /** * Response context keys for GroupBy query metrics. * These keys are used to aggregate metrics from parallel query execution threads @@ -32,6 +34,20 @@ public class GroupByResponseContextKeys public static final String GROUPBY_BYTES_SPILLED_TO_STORAGE_NAME = "groupByBytesSpilledToStorage"; public static final String GROUPBY_MERGE_DICTIONARY_SIZE_NAME = "groupByMergeDictionarySize"; + private static Object mergeMax(@Nullable Object oldValue, @Nullable Object newValue) + { + if (oldValue == null && newValue == null) { + return 0L; + } + + if (oldValue == null) { + return newValue; + } else if (newValue == null) { + return oldValue; + } + + return Math.max((Long) oldValue, (Long) newValue); + } /** * Maximum bytes spilled to storage across all parallel threads processing segments. * This represents the peak disk usage during query execution. @@ -42,7 +58,7 @@ public class GroupByResponseContextKeys @Override public Object mergeValues(Object oldValue, Object newValue) { - return Math.max((Long) oldValue, (Long) newValue); + return mergeMax(oldValue, newValue); } }; @@ -56,7 +72,7 @@ public Object mergeValues(Object oldValue, Object newValue) @Override public Object mergeValues(Object oldValue, Object newValue) { - return Math.max((Long) oldValue, (Long) newValue); + return mergeMax(oldValue, newValue); } }; @@ -70,7 +86,7 @@ public Object mergeValues(Object oldValue, Object newValue) @Override public Object mergeValues(Object oldValue, Object newValue) { - return Math.max((Long) oldValue, (Long) newValue); + return mergeMax(oldValue, newValue); } }; From 0141b70d0465729567559d1ae821478e4000282b Mon Sep 17 00:00:00 2001 From: GWphua Date: Thu, 4 Dec 2025 19:00:37 +0800 Subject: [PATCH 30/39] Working GroupByQueryMetrics --- .../groupby/GroupByQueryQueryToolChest.java | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index c712ec1ec79f..568e9bbb710e 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -187,15 +187,17 @@ private Sequence initAndMergeGroupByResults( final Sequence mergedSequence = mergeGroupByResults(query, resource, runner, context, closer); - if (reportMetricsForEmission) { - GroupByQueryMetrics queryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); - populateQueryMetrics(queryMetrics, context); - groupByStatsProvider.aggregateStats(queryMetrics); - } - // Clean up the resources reserved during the execution of the query closer.register(() -> groupByResourcesReservationPool.clean(queryResourceId)); + if (reportMetricsForEmission) { + closer.register(() -> { + GroupByQueryMetrics queryMetrics = (GroupByQueryMetrics) queryPlus.getQueryMetrics(); + populateQueryMetrics(queryMetrics, context); + groupByStatsProvider.aggregateStats(queryMetrics); + }); + } + return Sequences.withBaggage(mergedSequence, closer); } catch (Exception e) { @@ -207,6 +209,8 @@ private Sequence initAndMergeGroupByResults( private void populateQueryMetrics(GroupByQueryMetrics queryMetrics, ResponseContext context) { + // TODO: Everything is working as expected now, but the GroupByQueryMetrics expects to emit the metrics directly. + // Why not we also send the values individually to the GroupByStatsMonitor? Object bytesSpilledToStorage = context.get(GroupByResponseContextKeys.GROUPBY_BYTES_SPILLED_TO_STORAGE_KEY); if (bytesSpilledToStorage != null) { From 89b28d043d95c615c1fd45f0431853551535adeb Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 11:07:56 +0800 Subject: [PATCH 31/39] Fix GroupByStatsProvider Tests --- .../apache/druid/query/QueryDataSource.java | 2 +- .../groupby/DefaultGroupByQueryMetrics.java | 23 ++++++++++--------- .../query/groupby/GroupByQueryMetrics.java | 1 + 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/QueryDataSource.java b/processing/src/main/java/org/apache/druid/query/QueryDataSource.java index 029daf8bfe57..7809d5a0e372 100644 --- a/processing/src/main/java/org/apache/druid/query/QueryDataSource.java +++ b/processing/src/main/java/org/apache/druid/query/QueryDataSource.java @@ -37,7 +37,7 @@ public class QueryDataSource implements DataSource { @JsonProperty - private final Query query; + private final Query query; @JsonCreator public QueryDataSource(@JsonProperty("query") Query query) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index ac027b74207a..2dff5617a64a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -22,13 +22,11 @@ import org.apache.druid.query.DefaultQueryMetrics; import org.apache.druid.query.DruidMetrics; -import java.util.concurrent.atomic.LongAdder; - public class DefaultGroupByQueryMetrics extends DefaultQueryMetrics implements GroupByQueryMetrics { - private final LongAdder mergeBufferAcquisitonTime = new LongAdder(); - private final LongAdder bytesSpilledToStorage = new LongAdder(); - private final LongAdder mergeDictionarySize = new LongAdder(); + private long mergeBufferAcquisitonTime = 0L; + private long bytesSpilledToStorage = 0L; + private long mergeDictionarySize = 0L; @Override public void query(GroupByQuery query) @@ -69,45 +67,48 @@ public void granularity(GroupByQuery query) public void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime) { reportMetric("mergeBufferAcquisitonTime", mergeBufferAcquisitionTime); + this.mergeBufferAcquisitonTime = mergeBufferAcquisitionTime; } @Override public void bytesSpilledToStorage(long bytesSpilledToStorage) { reportMetric("bytesSpilledToStorage", bytesSpilledToStorage); + this.bytesSpilledToStorage = bytesSpilledToStorage; } @Override public void mergeDictionarySize(long mergeDictionarySize) { reportMetric("mergeDictionarySize", mergeDictionarySize); + this.mergeDictionarySize = mergeDictionarySize; } @Override public long getSpilledBytes() { - return bytesSpilledToStorage.longValue(); + return bytesSpilledToStorage; } @Override public long getMergeDictionarySize() { - return mergeDictionarySize.longValue(); + return mergeDictionarySize; } @Override public long getMergeBufferAcquisitionTime() { - return mergeBufferAcquisitonTime.longValue(); + return mergeBufferAcquisitonTime; } // TODO: Delete after debugging... @Override public String toString() { - return "bytesSpilledToStorage[" + bytesSpilledToStorage.longValue() - + "], mergeDictionarySize[" + mergeDictionarySize.longValue() - + "], mergeBufferAcquisitonTime[" + mergeBufferAcquisitonTime.longValue() + return "bytesSpilledToStorage[" + bytesSpilledToStorage + + "], mergeDictionarySize[" + mergeDictionarySize + + "], mergeBufferAcquisitonTime[" + mergeBufferAcquisitonTime + ']'; } } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java index c3aac1fdb415..e55ecc8572ed 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryMetrics.java @@ -64,6 +64,7 @@ public interface GroupByQueryMetrics extends QueryMetrics @PublicApi void mergeDictionarySize(long mergeDictionarySize); + // The get metrics methods below are used for GroupByStatsMonitor @PublicApi long getSpilledBytes(); From 0aeea7a5703d0b353241116c10eba712836436ab Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 14:43:35 +0800 Subject: [PATCH 32/39] GroupByQueryRunnerTests --- .../query/groupby/GroupByQueryRunnerTest.java | 14 ++++++++++++-- .../groupby/GroupByQueryRunnerTestHelper.java | 17 ++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java index b648890ca01d..3cdb858a4326 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java @@ -3550,9 +3550,12 @@ public void testGroupByWithFirstLast() makeRow(query, "2011-04-01", "market", "upfront", "first", 1447L, "last", 780L) ); - Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + StubServiceEmitter emitter = new StubServiceEmitter("", ""); + Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "first-last-aggs"); + emitter.verifyEmitted("query/wait/time", 1); + verifyGroupByMetricsForSmallBufferConfig(); } @@ -12503,10 +12506,17 @@ public void testGroupByOnVirtualColumn() makeRow(query, "2011-04-01", "v", 19600000L, "rows", 6L, "twosum", 18L) ); - Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + StubServiceEmitter emitter = new StubServiceEmitter("service", "host"); + Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "groupBy"); verifyGroupByMetricsForSmallBufferConfig(true); + if (config.toString().equals(V2_SMALL_BUFFER_CONFIG.toString())) { + emitter.verifyEmitted("query/wait/time", 1); + emitter.verifyEmitted("mergeBufferAcquisitonTime", 1); + emitter.verifyEmitted("bytesSpilledToStorage", 1); + emitter.verifyEmitted("mergeDictionarySize", 1); + } } @Test diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java index 2fb3004b2622..df1aafbae4cf 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java @@ -23,6 +23,7 @@ import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.java.util.emitter.service.ServiceEmitter; +import org.apache.druid.java.util.metrics.StubServiceEmitter; import org.apache.druid.query.FinalizeResultsQueryRunner; import org.apache.druid.query.MetricsEmittingQueryRunner; import org.apache.druid.query.Query; @@ -66,20 +67,30 @@ public static Iterable runQueryWithEmitter( ServiceEmitter serviceEmitter ) { + QueryToolChest toolChest = factory.getToolchest(); MetricsEmittingQueryRunner metricsEmittingQueryRunner = new MetricsEmittingQueryRunner( serviceEmitter, - factory.getToolchest(), + toolChest, runner, MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, (metrics) -> {} ).withWaitMeasuredFromNow(); - QueryToolChest toolChest = factory.getToolchest(); - QueryRunner theRunner = new FinalizeResultsQueryRunner<>( + + QueryRunner finalizeResultsQueryRunner = new FinalizeResultsQueryRunner<>( toolChest.mergeResults(toolChest.preMergeQueryDecoration(metricsEmittingQueryRunner)), toolChest ); + QueryRunner theRunner = new MetricsEmittingQueryRunner<>( + serviceEmitter, + toolChest, + finalizeResultsQueryRunner, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, + (queryMetrics -> { + }) + ); + return theRunner.run(QueryPlus.wrap(populateResourceId(query))).toList(); } From 6017144f3a3593814a0245cc8cb2a0706cfe6b75 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 15:31:05 +0800 Subject: [PATCH 33/39] GroupByQueries --- .../groupby/DefaultGroupByQueryMetrics.java | 2 +- .../query/groupby/GroupByQueryRunnerTest.java | 51 ++++++++++--------- .../groupby/GroupByQueryRunnerTestHelper.java | 1 - 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index 2dff5617a64a..b97c4803a251 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -66,7 +66,7 @@ public void granularity(GroupByQuery query) @Override public void mergeBufferAcquisitionTime(long mergeBufferAcquisitionTime) { - reportMetric("mergeBufferAcquisitonTime", mergeBufferAcquisitionTime); + reportMetric("mergeBufferAcquisitionTime", mergeBufferAcquisitionTime); this.mergeBufferAcquisitonTime = mergeBufferAcquisitionTime; } diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java index 3cdb858a4326..f83e78d7f44b 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java @@ -3550,13 +3550,11 @@ public void testGroupByWithFirstLast() makeRow(query, "2011-04-01", "market", "upfront", "first", 1447L, "last", 780L) ); - StubServiceEmitter emitter = new StubServiceEmitter("", ""); + StubServiceEmitter emitter = new StubServiceEmitter("service", "host"); Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "first-last-aggs"); - emitter.verifyEmitted("query/wait/time", 1); - - verifyGroupByMetricsForSmallBufferConfig(); + verifyGroupByMetricsForSmallBufferConfig(emitter); } @Test @@ -6150,10 +6148,11 @@ public void testDifferentGroupingSubqueryMultipleAggregatorsOnSameField() ) ); - Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + StubServiceEmitter emitter = new StubServiceEmitter("service", "host"); + Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "subquery-multiple-aggs"); - verifyGroupByMetricsForSmallBufferConfig(); + verifyGroupByMetricsForSmallBufferConfig(emitter); } @Test @@ -6197,10 +6196,11 @@ public void testDifferentGroupingSubqueryWithFilter() makeRow(query, "2011-04-02", "idx", 2505.0) ); - Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + StubServiceEmitter emitter = new StubServiceEmitter("service", "host"); + Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "subquery-filter"); - verifyGroupByMetricsForSmallBufferConfig(); + verifyGroupByMetricsForSmallBufferConfig(emitter); } @Test @@ -6922,10 +6922,11 @@ public boolean eval(ResultRow row) ); // Subqueries are handled by the ToolChest - Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + StubServiceEmitter emitter = new StubServiceEmitter("service", "host"); + Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "subquery-postaggs"); - verifyGroupByMetricsForSmallBufferConfig(); + verifyGroupByMetricsForSmallBufferConfig(emitter); } @Test @@ -12435,10 +12436,11 @@ public void testGroupByOnNullableDoubleNoLimitPushdown() makeRow(query, "2011-04-01", "nullable", 50.0, "rows", 6L) ); - Iterable results = GroupByQueryRunnerTestHelper.runQuery(factory, runner, query); + StubServiceEmitter emitter = new StubServiceEmitter("service", "host"); + Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "groupBy"); - verifyGroupByMetricsForSmallBufferConfig(true); + verifyGroupByMetricsForSmallBufferConfig(emitter, true); } @Test @@ -12510,13 +12512,7 @@ public void testGroupByOnVirtualColumn() Iterable results = GroupByQueryRunnerTestHelper.runQueryWithEmitter(factory, runner, query, emitter); TestHelper.assertExpectedObjects(expectedResults, results, "groupBy"); - verifyGroupByMetricsForSmallBufferConfig(true); - if (config.toString().equals(V2_SMALL_BUFFER_CONFIG.toString())) { - emitter.verifyEmitted("query/wait/time", 1); - emitter.verifyEmitted("mergeBufferAcquisitonTime", 1); - emitter.verifyEmitted("bytesSpilledToStorage", 1); - emitter.verifyEmitted("mergeDictionarySize", 1); - } + verifyGroupByMetricsForSmallBufferConfig(emitter, true); } @Test @@ -13737,7 +13733,7 @@ private void cannotVectorize() } } - private void verifyGroupByMetricsForSmallBufferConfig(boolean skipMergeDictionaryMetric) + private void verifyGroupByMetricsForSmallBufferConfig(StubServiceEmitter emitter, boolean skipMergeDictionaryMetric) { if (!config.toString().equals(V2_SMALL_BUFFER_CONFIG.toString())) { return; @@ -13747,14 +13743,23 @@ private void verifyGroupByMetricsForSmallBufferConfig(boolean skipMergeDictionar Assert.assertTrue(aggregateStats.getSpilledBytes() > 0); Assert.assertEquals(1, aggregateStats.getMergeBufferQueries()); Assert.assertTrue(aggregateStats.getMergeBufferAcquisitionTimeNs() > 0); + if (!skipMergeDictionaryMetric) { Assert.assertTrue(aggregateStats.getMergeDictionarySize() > 0); + if (emitter != null) { + emitter.verifyEmitted("mergeDictionarySize", 1); + } + } + + if (emitter != null) { + emitter.verifyEmitted("query/wait/time", 1); + emitter.verifyEmitted("bytesSpilledToStorage", 1); + emitter.verifyEmitted("mergeBufferAcquisitionTime", 1); } } - private void verifyGroupByMetricsForSmallBufferConfig() + private void verifyGroupByMetricsForSmallBufferConfig(StubServiceEmitter emitter) { - verifyGroupByMetricsForSmallBufferConfig(false); + verifyGroupByMetricsForSmallBufferConfig(emitter, false); } } - diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java index df1aafbae4cf..17db1b6d2902 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTestHelper.java @@ -23,7 +23,6 @@ import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.guava.Sequence; import org.apache.druid.java.util.emitter.service.ServiceEmitter; -import org.apache.druid.java.util.metrics.StubServiceEmitter; import org.apache.druid.query.FinalizeResultsQueryRunner; import org.apache.druid.query.MetricsEmittingQueryRunner; import org.apache.druid.query.Query; From bdc1adb434030b5bc8264c1ee24612c5d7139040 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 17:54:20 +0800 Subject: [PATCH 34/39] Fix UnionTest failures --- .../druid/server/ClientQuerySegmentWalker.java | 9 +++++++-- .../apache/druid/server/LocalQuerySegmentWalker.java | 9 +++++++-- .../java/org/apache/druid/server/ServerManager.java | 12 ++++++++++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java index 93e09ed9e0f6..f23c37a7b14e 100644 --- a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java @@ -68,6 +68,7 @@ import org.apache.druid.query.SegmentDescriptor; import org.apache.druid.query.TableDataSource; import org.apache.druid.query.planning.ExecutionVertex; +import org.apache.druid.query.search.SearchQuery; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.segment.join.JoinableFactory; import org.apache.druid.server.initialization.ServerConfig; @@ -611,8 +612,12 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner resultLevelCachingQueryRunner, MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, queryMetrics -> { - queryMetrics.queryId(query.getId()); - queryMetrics.sqlQueryId(query.getSqlQueryId()); + // SearchQuery metrics don't support queryId/sqlQueryId methods as these are + // already set via the delegate in DefaultSearchQueryMetrics + if (!(query instanceof SearchQuery)) { + queryMetrics.queryId(query.getId()); + queryMetrics.sqlQueryId(query.getSqlQueryId()); + } } ); } diff --git a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java index 8b04ddd5d1ad..31e7bd56da21 100644 --- a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java @@ -36,6 +36,7 @@ import org.apache.druid.query.SegmentDescriptor; import org.apache.druid.query.planning.ExecutionVertex; import org.apache.druid.query.policy.PolicyEnforcer; +import org.apache.druid.query.search.SearchQuery; import org.apache.druid.segment.ReferenceCountedSegmentProvider; import org.apache.druid.segment.Segment; import org.apache.druid.segment.SegmentMapFunction; @@ -123,8 +124,12 @@ public QueryRunner getQueryRunnerForIntervals(final Query query, final runner, MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, queryMetrics -> { - queryMetrics.queryId(query.getId()); - queryMetrics.sqlQueryId(query.getSqlQueryId()); + // SearchQuery metrics don't support queryId/sqlQueryId methods as these are + // already set via the delegate in DefaultSearchQueryMetrics + if (!(query instanceof SearchQuery)) { + queryMetrics.queryId(query.getId()); + queryMetrics.sqlQueryId(query.getSqlQueryId()); + } }) ) .emitCPUTimeMetric(emitter, cpuAccumulator); diff --git a/server/src/main/java/org/apache/druid/server/ServerManager.java b/server/src/main/java/org/apache/druid/server/ServerManager.java index 19979de1d512..ff474d58fff6 100644 --- a/server/src/main/java/org/apache/druid/server/ServerManager.java +++ b/server/src/main/java/org/apache/druid/server/ServerManager.java @@ -64,6 +64,7 @@ import org.apache.druid.query.metadata.metadata.SegmentMetadataQuery; import org.apache.druid.query.planning.ExecutionVertex; import org.apache.druid.query.policy.PolicyEnforcer; +import org.apache.druid.query.search.SearchQuery; import org.apache.druid.query.spec.SpecificSegmentQueryRunner; import org.apache.druid.query.spec.SpecificSegmentSpec; import org.apache.druid.segment.Segment; @@ -667,8 +668,15 @@ public Sequence run(QueryPlus queryPlus, ResponseContext responseContext) metrics -> { // TODO: Remove the logs after finished. log.info("Datasource MetricsEmittingQueryRunner accepting metrics[%s]", metrics); - metrics.queryId(query.getId()); - metrics.sqlQueryId(query.getSqlQueryId()); + + // TODO: Do we need to emit the queryId, sqlQueryId for all queries? Would simply emitting for GroupBy be enough? + // Or is there a case where the GroupBy will automatically emit its query.getId()? + // SearchQuery metrics don't support queryId/sqlQueryId methods as these are + // already set via the delegate in DefaultSearchQueryMetrics + if (!(query instanceof SearchQuery)) { + metrics.queryId(query.getId()); + metrics.sqlQueryId(query.getSqlQueryId()); + } } ); From d6bf57aa74bf1139de04fe0da864491720f826bc Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 18:53:36 +0800 Subject: [PATCH 35/39] Fix queryMetrics logging --- .../server/ClientQuerySegmentWalker.java | 10 +--------- .../druid/server/LocalQuerySegmentWalker.java | 19 ++++++------------- .../apache/druid/server/ServerManager.java | 15 +-------------- 3 files changed, 8 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java index f23c37a7b14e..b9cd25ff21c6 100644 --- a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java @@ -68,7 +68,6 @@ import org.apache.druid.query.SegmentDescriptor; import org.apache.druid.query.TableDataSource; import org.apache.druid.query.planning.ExecutionVertex; -import org.apache.druid.query.search.SearchQuery; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.segment.join.JoinableFactory; import org.apache.druid.server.initialization.ServerConfig; @@ -611,14 +610,7 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner toolChest, resultLevelCachingQueryRunner, MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, - queryMetrics -> { - // SearchQuery metrics don't support queryId/sqlQueryId methods as these are - // already set via the delegate in DefaultSearchQueryMetrics - if (!(query instanceof SearchQuery)) { - queryMetrics.queryId(query.getId()); - queryMetrics.sqlQueryId(query.getSqlQueryId()); - } - } + queryMetrics -> {} ); } ) diff --git a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java index 31e7bd56da21..4352df155d8a 100644 --- a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java @@ -36,7 +36,6 @@ import org.apache.druid.query.SegmentDescriptor; import org.apache.druid.query.planning.ExecutionVertex; import org.apache.druid.query.policy.PolicyEnforcer; -import org.apache.druid.query.search.SearchQuery; import org.apache.druid.segment.ReferenceCountedSegmentProvider; import org.apache.druid.segment.Segment; import org.apache.druid.segment.SegmentMapFunction; @@ -119,18 +118,12 @@ public QueryRunner getQueryRunnerForIntervals(final Query query, final .mergeResults(true) .applyPostMergeDecoration() .map(runner -> new MetricsEmittingQueryRunner<>( - emitter, - queryRunnerFactory.getToolchest(), - runner, - MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, - queryMetrics -> { - // SearchQuery metrics don't support queryId/sqlQueryId methods as these are - // already set via the delegate in DefaultSearchQueryMetrics - if (!(query instanceof SearchQuery)) { - queryMetrics.queryId(query.getId()); - queryMetrics.sqlQueryId(query.getSqlQueryId()); - } - }) + emitter, + queryRunnerFactory.getToolchest(), + runner, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, + queryMetrics -> {} + ) ) .emitCPUTimeMetric(emitter, cpuAccumulator); } diff --git a/server/src/main/java/org/apache/druid/server/ServerManager.java b/server/src/main/java/org/apache/druid/server/ServerManager.java index ff474d58fff6..248b12240e6a 100644 --- a/server/src/main/java/org/apache/druid/server/ServerManager.java +++ b/server/src/main/java/org/apache/druid/server/ServerManager.java @@ -64,7 +64,6 @@ import org.apache.druid.query.metadata.metadata.SegmentMetadataQuery; import org.apache.druid.query.planning.ExecutionVertex; import org.apache.druid.query.policy.PolicyEnforcer; -import org.apache.druid.query.search.SearchQuery; import org.apache.druid.query.spec.SpecificSegmentQueryRunner; import org.apache.druid.query.spec.SpecificSegmentSpec; import org.apache.druid.segment.Segment; @@ -665,19 +664,7 @@ public Sequence run(QueryPlus queryPlus, ResponseContext responseContext) toolChest, finalizeResultsQueryRunner, MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, - metrics -> { - // TODO: Remove the logs after finished. - log.info("Datasource MetricsEmittingQueryRunner accepting metrics[%s]", metrics); - - // TODO: Do we need to emit the queryId, sqlQueryId for all queries? Would simply emitting for GroupBy be enough? - // Or is there a case where the GroupBy will automatically emit its query.getId()? - // SearchQuery metrics don't support queryId/sqlQueryId methods as these are - // already set via the delegate in DefaultSearchQueryMetrics - if (!(query instanceof SearchQuery)) { - metrics.queryId(query.getId()); - metrics.sqlQueryId(query.getSqlQueryId()); - } - } + metrics -> {} ); final QueryRunner queryRunner = CPUTimeMetricQueryRunner.safeBuild( From b5284e58fd73eb9a59863ea17ec7c36fe0e41514 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 19:00:10 +0800 Subject: [PATCH 36/39] Remove unnecessary queryMetrics logging --- .../server/ClientQuerySegmentWalker.java | 43 ++++++++++--------- .../apache/druid/server/ServerManager.java | 6 +-- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java index b9cd25ff21c6..bc1c8ccb2a13 100644 --- a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java @@ -587,6 +587,17 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner .applyPreMergeDecoration() .mergeResults(false) .applyPostMergeDecoration() + // TODO: Find out why removing this .map statement will pass the test in SqlResourceTest.... + .map(runner -> + new MetricsEmittingQueryRunner<>( + emitter, + toolChest, + runner, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, + queryMetrics -> {} + ) + ) + .emitCPUTimeMetric(emitter) .postProcess( objectMapper.convertValue( query.context().getString("postProcessing"), @@ -594,27 +605,17 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner ) ) .map( - runner -> { - final QueryRunner resultLevelCachingQueryRunner = new ResultLevelCachingQueryRunner<>( - runner, - toolChest, - query, - objectMapper, - cache, - cacheConfig, - emitter - ); - - return new MetricsEmittingQueryRunner<>( - emitter, - toolChest, - resultLevelCachingQueryRunner, - MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, - queryMetrics -> {} - ); - } - ) - .emitCPUTimeMetric(emitter); + runner -> + new ResultLevelCachingQueryRunner<>( + runner, + toolChest, + query, + objectMapper, + cache, + cacheConfig, + emitter + ) + ); } /** diff --git a/server/src/main/java/org/apache/druid/server/ServerManager.java b/server/src/main/java/org/apache/druid/server/ServerManager.java index 248b12240e6a..dc744f5c9415 100644 --- a/server/src/main/java/org/apache/druid/server/ServerManager.java +++ b/server/src/main/java/org/apache/druid/server/ServerManager.java @@ -529,11 +529,7 @@ protected QueryRunner buildQueryRunnerForSegment( toolChest, bySegmentQueryRunner, QueryMetrics::reportSegmentAndCacheTime, - // TODO: Maybe also apply a log to see when this is called. - queryMetrics -> { - log.info("Segment MetricsEmittingQueryRunner accepting metrics[%s]", queryMetrics); - queryMetrics.segment(segmentIdString); - } + queryMetrics -> queryMetrics.segment(segmentIdString) ).withWaitMeasuredFromNow(); final SpecificSegmentQueryRunner specificSegmentQueryRunner = new SpecificSegmentQueryRunner<>( From da290a9a84377972190f569ddcefb2135507f355 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 22:13:35 +0800 Subject: [PATCH 37/39] Fix tests in SqlResourceTest --- .../druid/sql/http/SqlResourceTest.java | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java index 894619b9ea3c..d2e5609e260c 100644 --- a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java +++ b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java @@ -2215,14 +2215,32 @@ private Response postForSyncResponse(SqlQuery query, MockHttpServletRequest req) final Object explicitQueryId = query.getContext().get("queryId"); final Object explicitSqlQueryId = query.getContext().get("sqlQueryId"); + // Set up async context support in case the query execution path uses async processing. + // This is necessary because MetricsEmittingQueryRunner wraps queries in LazySequence, + // which defers execution until the sequence is consumed (after startAsync() is called). + MockHttpServletResponse asyncResponse = MockHttpServletResponse.forRequest(req); final Response response = resource.doPost(query, req); - final Object actualQueryId = getHeader(response, QueryResource.QUERY_ID_RESPONSE_HEADER); - final Object actualSqlQueryId = getHeader(response, SqlResource.SQL_QUERY_ID_RESPONSE_HEADER); - - validateQueryIds(explicitQueryId, explicitSqlQueryId, actualQueryId, actualSqlQueryId); - - return response; + if (response != null) { + // Sync response path - error happened before async processing started + final Object actualQueryId = getHeader(response, QueryResource.QUERY_ID_RESPONSE_HEADER); + final Object actualSqlQueryId = getHeader(response, SqlResource.SQL_QUERY_ID_RESPONSE_HEADER); + validateQueryIds(explicitQueryId, explicitSqlQueryId, actualQueryId, actualSqlQueryId); + return response; + } else { + // Async response path - need to construct a Response from the async response + final Object actualQueryId = asyncResponse.getHeader(QueryResource.QUERY_ID_RESPONSE_HEADER); + final Object actualSqlQueryId = asyncResponse.getHeader(SqlResource.SQL_QUERY_ID_RESPONSE_HEADER); + validateQueryIds(explicitQueryId, explicitSqlQueryId, actualQueryId, actualSqlQueryId); + + Response.ResponseBuilder responseBuilder = Response.status(asyncResponse.getStatus()); + for (String headerName : asyncResponse.getHeaderNames()) { + responseBuilder.header(headerName, asyncResponse.getHeader(headerName)); + } + final byte[] responseBytes = asyncResponse.baos.toByteArray(); + responseBuilder.entity((StreamingOutput) output -> output.write(responseBytes)); + return responseBuilder.build(); + } } private ErrorResponse postSyncForException(String s, int expectedStatus) throws IOException From 99ea400c33c027e0662cd6e15db70b920c3a98bd Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 22:29:17 +0800 Subject: [PATCH 38/39] Checkstyle --- .../epinephelinae/ConcurrentGrouperTest.java | 8 ++------ .../druid/server/ClientQuerySegmentWalker.java | 1 - .../druid/server/LocalQuerySegmentWalker.java | 15 ++++++++------- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java index 2ee8fb5e97ce..5f2e2208a297 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouperTest.java @@ -36,8 +36,6 @@ import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.CountAggregatorFactory; import org.apache.druid.query.dimension.DimensionSpec; -import org.apache.druid.query.groupby.DefaultGroupByQueryMetrics; -import org.apache.druid.query.groupby.GroupByQueryMetrics; import org.apache.druid.query.groupby.epinephelinae.Grouper.BufferComparator; import org.apache.druid.query.groupby.epinephelinae.Grouper.Entry; import org.apache.druid.query.groupby.epinephelinae.Grouper.KeySerde; @@ -149,7 +147,6 @@ public ByteBuffer get() @Test() public void testAggregate() throws InterruptedException, ExecutionException, IOException { - GroupByQueryMetrics groupByQueryMetrics = new DefaultGroupByQueryMetrics(); final LimitedTemporaryStorage temporaryStorage = new LimitedTemporaryStorage( temporaryFolder.newFolder(), 1024 * 1024 @@ -196,7 +193,7 @@ public void testAggregate() throws InterruptedException, ExecutionException, IOE }); } - for (Future eachFuture : futures) { + for (Future eachFuture : futures) { eachFuture.get(); } @@ -230,7 +227,6 @@ public void testGrouperTimeout() throws Exception return; } - GroupByQueryMetrics groupByQueryMetrics = new DefaultGroupByQueryMetrics(); ListeningExecutorService service = MoreExecutors.listeningDecorator(exec); try { final ConcurrentGrouper grouper = new ConcurrentGrouper<>( @@ -273,7 +269,7 @@ public void testGrouperTimeout() throws Exception }); } - for (Future eachFuture : futures) { + for (Future eachFuture : futures) { eachFuture.get(); } diff --git a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java index bc1c8ccb2a13..ac6cd400d6af 100644 --- a/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java @@ -587,7 +587,6 @@ private QueryRunner decorateClusterRunner(Query query, QueryRunner .applyPreMergeDecoration() .mergeResults(false) .applyPostMergeDecoration() - // TODO: Find out why removing this .map statement will pass the test in SqlResourceTest.... .map(runner -> new MetricsEmittingQueryRunner<>( emitter, diff --git a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java index 4352df155d8a..f18cb6c198f9 100644 --- a/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java +++ b/server/src/main/java/org/apache/druid/server/LocalQuerySegmentWalker.java @@ -117,13 +117,14 @@ public QueryRunner getQueryRunnerForIntervals(final Query query, final .applyPreMergeDecoration() .mergeResults(true) .applyPostMergeDecoration() - .map(runner -> new MetricsEmittingQueryRunner<>( - emitter, - queryRunnerFactory.getToolchest(), - runner, - MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, - queryMetrics -> {} - ) + .map(runner -> + new MetricsEmittingQueryRunner<>( + emitter, + queryRunnerFactory.getToolchest(), + runner, + MetricsEmittingQueryRunner.NOOP_METRIC_REPORTER, + queryMetrics -> {} + ) ) .emitCPUTimeMetric(emitter, cpuAccumulator); } From 17e57008ccf00ca14d5ed348d62edf91bea86847 Mon Sep 17 00:00:00 2001 From: GWphua Date: Fri, 5 Dec 2025 22:58:28 +0800 Subject: [PATCH 39/39] Cleanup --- .../segment/DatasketchesProjectionTest.java | 3 +- .../query/MetricsEmittingQueryRunner.java | 2 +- .../apache/druid/query/QueryDataSource.java | 4 +- .../groupby/DefaultGroupByQueryMetrics.java | 10 ----- .../groupby/GroupByQueryQueryToolChest.java | 2 +- .../epinephelinae/ConcurrentGrouper.java | 45 ++++++++----------- .../epinephelinae/RowBasedGrouperHelper.java | 7 +-- .../segment/CursorFactoryProjectionTest.java | 7 +-- .../druid/segment/CursorHolderPreaggTest.java | 3 +- .../IncrementalIndexCursorFactoryTest.java | 3 +- .../apache/druid/server/ServerManager.java | 4 +- 11 files changed, 33 insertions(+), 57 deletions(-) diff --git a/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java b/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java index 374518f3ef33..fb768d9ea48e 100644 --- a/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java +++ b/extensions-core/datasketches/src/test/java/org/apache/druid/segment/DatasketchesProjectionTest.java @@ -277,8 +277,7 @@ public DatasketchesProjectionTest( ), TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), - (query, future) -> { - } + (query, future) -> {} ); } diff --git a/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java b/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java index f806280cf602..9673be4ebe2d 100644 --- a/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java +++ b/processing/src/main/java/org/apache/druid/query/MetricsEmittingQueryRunner.java @@ -34,7 +34,7 @@ */ public class MetricsEmittingQueryRunner implements QueryRunner { - public static final ObjLongConsumer> NOOP_METRIC_REPORTER = (metrics, value) -> { }; + public static final ObjLongConsumer> NOOP_METRIC_REPORTER = (metrics, value) -> {}; private final ServiceEmitter emitter; private final QueryToolChest> queryToolChest; diff --git a/processing/src/main/java/org/apache/druid/query/QueryDataSource.java b/processing/src/main/java/org/apache/druid/query/QueryDataSource.java index 7809d5a0e372..61db5b62f39a 100644 --- a/processing/src/main/java/org/apache/druid/query/QueryDataSource.java +++ b/processing/src/main/java/org/apache/druid/query/QueryDataSource.java @@ -37,10 +37,10 @@ public class QueryDataSource implements DataSource { @JsonProperty - private final Query query; + private final Query query; @JsonCreator - public QueryDataSource(@JsonProperty("query") Query query) + public QueryDataSource(@JsonProperty("query") Query query) { this.query = Preconditions.checkNotNull(query, "'query' must be nonnull"); } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java index b97c4803a251..b7f8fb5c17fd 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/DefaultGroupByQueryMetrics.java @@ -101,14 +101,4 @@ public long getMergeBufferAcquisitionTime() { return mergeBufferAcquisitonTime; } - - // TODO: Delete after debugging... - @Override - public String toString() - { - return "bytesSpilledToStorage[" + bytesSpilledToStorage - + "], mergeDictionarySize[" + mergeDictionarySize - + "], mergeBufferAcquisitonTime[" + mergeBufferAcquisitonTime - + ']'; - } } diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java index 568e9bbb710e..317e12c7a072 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java @@ -492,7 +492,7 @@ public Sequence run(QueryPlus queryPlus, ResponseContext r dimensionSpecs.add(dimensionSpec); } } - // TODO: Is this where I aggregate the responseContext? + // TODO: Do I need to aggregate the responseContext for the subquery? return runner.run( queryPlus.withQuery(groupByQuery.withDimensionSpecs(dimensionSpecs)), responseContext diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java index 7d5aa30316be..dcb41653bc04 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ConcurrentGrouper.java @@ -46,7 +46,6 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -415,20 +414,22 @@ private List>> parallelSortAndGetGroupersIterat { // The number of groupers is same with the number of processing threads in the executor final List>>> futures = groupers.stream() - .map(grouper -> - executor.submit( - new AbstractPrioritizedCallable>>(priority) - { - @Override - public CloseableIterator> call() - { - return grouper.iterator(true); - } - } - ) - ) - .collect(Collectors.toList() - ); + .map(grouper -> + executor.submit( + new AbstractPrioritizedCallable>>( + priority) + { + @Override + public CloseableIterator> call() + { + return grouper.iterator( + true); + } + } + ) + ) + .collect(Collectors.toList() + ); ListenableFuture>>> future = Futures.allAsList(futures); try { @@ -495,17 +496,9 @@ private List tryMergeDictionary() @Override public Map getQueryMetricsMap() { - Map map = new HashMap<>(); - - groupers.forEach(spillingGrouper -> { - Map metricsMap = spillingGrouper.getQueryMetricsMap(); - - for (Map.Entry entry : metricsMap.entrySet()) { - map.compute(entry.getKey(), (key, value) -> value == null ? entry.getValue() : value + entry.getValue()); - } - }); - - return map; + return groupers.stream() + .flatMap(grouper -> grouper.getQueryMetricsMap().entrySet().stream()) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, Long::sum)); } @Override diff --git a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java index 4a02f7118787..d5235b3920c4 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java @@ -167,9 +167,9 @@ public static Pair, Accumulator * and dim filters) are respected, and its aggregators are used in standard (not combining) form. The input * ResultRows are assumed to be results originating from the provided "subquery". * - * @param query the query (and context) that we are grouping for - * @param subquery optional subquery (and context) that produced the intermediate rows, or {@code null} when - * operating in combining mode (see description above) + * @param query the query that we are grouping for + * @param subquery optional subquery that we are receiving results from (see combining vs. subquery + * mode above) * @param config groupBy query config * @param processingConfig processing config * @param bufferSupplier supplier of merge buffers @@ -210,6 +210,7 @@ public static Pair, Accumulator // See method-level javadoc; we go into combining mode if there is no subquery. final boolean combining = subquery == null; + final List valueTypes = DimensionHandlerUtils.getValueTypesFromDimensionSpecs(query.getDimensions()); final GroupByQueryConfig querySpecificConfig = config.withOverrides(query); diff --git a/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java b/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java index d5e2d83f183e..95d4d79bab27 100644 --- a/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java +++ b/processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java @@ -581,8 +581,7 @@ public CursorFactoryProjectionTest( resourcesReservationPool, TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), - (query, future) -> { - } + (query, future) -> {} ); this.timeseriesEngine = new TimeseriesQueryEngine(nonBlockingPool); } @@ -2250,25 +2249,21 @@ private ExpectedProjectionGroupBy(@Nullable String expectedProjection) @Override public void numDimensions(GroupByQuery query) { - // no-op for projection tests } @Override public void numMetrics(GroupByQuery query) { - // no-op for projection tests } @Override public void numComplexMetrics(GroupByQuery query) { - // no-op for projection tests } @Override public void granularity(GroupByQuery query) { - // no-op for projection tests } @Override diff --git a/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java b/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java index e9f9149b9b93..e3e097187b96 100644 --- a/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java +++ b/processing/src/test/java/org/apache/druid/segment/CursorHolderPreaggTest.java @@ -101,8 +101,7 @@ public void setup() ), TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), - (query, future) -> { - } + (query, future) -> {} ); this.cursorFactory = new CursorFactory() diff --git a/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java b/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java index 11ceb249e9bd..1d3460a1680a 100644 --- a/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java +++ b/processing/src/test/java/org/apache/druid/segment/incremental/IncrementalIndexCursorFactoryTest.java @@ -174,8 +174,7 @@ public IncrementalIndexCursorFactoryTest(String indexType, boolean sortByDim) ), TestHelper.makeJsonMapper(), TestHelper.makeSmileMapper(), - (query, future) -> { - } + (query, future) -> {} ); topnQueryEngine = new TopNQueryEngine(nonBlockingPool); } diff --git a/server/src/main/java/org/apache/druid/server/ServerManager.java b/server/src/main/java/org/apache/druid/server/ServerManager.java index dc744f5c9415..bc0b885d0ea5 100644 --- a/server/src/main/java/org/apache/druid/server/ServerManager.java +++ b/server/src/main/java/org/apache/druid/server/ServerManager.java @@ -655,7 +655,7 @@ public Sequence run(QueryPlus queryPlus, ResponseContext responseContext) toolChest ); - final QueryRunner datasourceMetricsEmittingQueryRunner = new MetricsEmittingQueryRunner<>( + final QueryRunner finalizedMetricsEmittingQueryRunner = new MetricsEmittingQueryRunner<>( emitter, toolChest, finalizeResultsQueryRunner, @@ -664,7 +664,7 @@ public Sequence run(QueryPlus queryPlus, ResponseContext responseContext) ); final QueryRunner queryRunner = CPUTimeMetricQueryRunner.safeBuild( - datasourceMetricsEmittingQueryRunner, + finalizedMetricsEmittingQueryRunner, toolChest, emitter, cpuTimeAccumulator,