You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "maytasm (via GitHub)" <gi...@apache.org> on 2026/03/12 07:35:42 UTC
[PR] Add spill file count limit for GroupBy query (druid)
maytasm opened a new pull request, #19141:
URL: https://github.com/apache/druid/pull/19141
Add spill file count limit for GroupBy query
### Description
GroupBy queries that group on high-cardinality dimensions can create a large number of spill files. This problem is more likely when queries contain many aggregators and/or aggregators with large memory footprints (e.g., DataSketch). This is because GroupBy can only hold a limited number of unique groupings in memory before flushing to disk — the exact limit depends on the size of each row, which is determined by the size of the aggregators. The issue arises when GroupBy attempts to merge all the spill files. Currently, GroupBy merges spill files by opening all of them simultaneously. Opening these files requires memory for objects such as MappingIterator, SmileParser, etc., which can cause historical nodes to OOM.
This PR fixes the issue by introducing a new property: `druid.query.groupBy.maxSpillFileCount`
The maximum number of spill files allowed per GroupBy query. When the limit is reached, the query fails with a ResourceLimitExceededException. This property can be used to prevent historical nodes from OOMing due to an excessive number of spill files being opened simultaneously during the merge phase. Defaults to Integer.MAX_VALUE (unlimited). Can also be set per query via the query context key `maxSpillFileCount`.
---
Release Notes
- Added a new GroupBy query configuration property druid.query.groupBy.maxSpillFileCount to limit the maximum number of spill files created per query. When the limit is exceeded, the query fails with a clear error message instead of causing historical nodes to OOM during spill file merging. The limit can also be overridden per query via the query context `maxSpillFileCount`.
##### Key changed/added classes in this PR
* `processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/LimitedTemporaryStorage.java`
* `processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java`
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [x] added documentation for new or modified features or behaviors.
- [x] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add spill file count limit for GroupBy query (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm commented on code in PR #19141:
URL: https://github.com/apache/druid/pull/19141#discussion_r2927839393
##########
docs/configuration/index.md:
##########
@@ -2266,6 +2266,7 @@ Supported runtime properties:
|`druid.query.groupBy.maxSelectorDictionarySize`|Maximum amount of heap space (approximately) to use for per-segment string dictionaries. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxMergingDictionarySize`|Maximum amount of heap space (approximately) to use for per-query string dictionaries. When the dictionary exceeds this size, a spill to disk will be triggered. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxOnDiskStorage`|Maximum amount of disk space to use, per-query, for spilling result sets to disk when either the merging buffer or the dictionary fills up. Queries that exceed this limit will fail. Set to zero to disable disk spilling.|0 (disabled)|
+|`druid.query.groupBy.maxSpillFileCount`|Maximum number of spill files allowed per GroupBy query. Queries that exceed this limit will fail.|Integer.MAX_VALUE (unlimited)|
Review Comment:
Im going to say "See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details." here and add more details to the memory-tuning-and-resource-limits section (so as to not repeat myself)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add spill file count limit for GroupBy query (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm commented on code in PR #19141:
URL: https://github.com/apache/druid/pull/19141#discussion_r2927792099
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/TemporaryStorageFileLimitException.java:
##########
@@ -17,42 +17,16 @@
* under the License.
*/
-package org.apache.druid.storage.s3;
+package org.apache.druid.query.groupby.epinephelinae;
-import software.amazon.awssdk.services.s3.model.S3Object;
+import org.apache.druid.java.util.common.StringUtils;
-/**
- * Pairs an {@link S3Object} with its bucket name.
- * In AWS SDK v2, {@link S3Object} does not carry the bucket name, so callers that need it must track it separately.
- */
-public class S3ObjectWithBucket
-{
- private final String bucket;
- private final S3Object s3Object;
-
- public S3ObjectWithBucket(String bucket, S3Object s3Object)
- {
- this.bucket = bucket;
- this.s3Object = s3Object;
- }
-
- public String getBucket()
- {
- return bucket;
- }
+import java.io.IOException;
- public String getKey()
- {
- return s3Object.key();
- }
-
- public long getSize()
- {
- return s3Object.size();
- }
-
- public S3Object getS3Object()
+public class TemporaryStorageFileLimitException extends IOException
+{
+ public TemporaryStorageFileLimitException(final int fileCount)
{
- return s3Object;
+ super(StringUtils.format("Cannot write to disk, hit file count limit of %,d.", fileCount));
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add spill file count limit for GroupBy query (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm merged PR #19141:
URL: https://github.com/apache/druid/pull/19141
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add spill file count limit for GroupBy query (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm commented on code in PR #19141:
URL: https://github.com/apache/druid/pull/19141#discussion_r2927839393
##########
docs/configuration/index.md:
##########
@@ -2266,6 +2266,7 @@ Supported runtime properties:
|`druid.query.groupBy.maxSelectorDictionarySize`|Maximum amount of heap space (approximately) to use for per-segment string dictionaries. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxMergingDictionarySize`|Maximum amount of heap space (approximately) to use for per-query string dictionaries. When the dictionary exceeds this size, a spill to disk will be triggered. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxOnDiskStorage`|Maximum amount of disk space to use, per-query, for spilling result sets to disk when either the merging buffer or the dictionary fills up. Queries that exceed this limit will fail. Set to zero to disable disk spilling.|0 (disabled)|
+|`druid.query.groupBy.maxSpillFileCount`|Maximum number of spill files allowed per GroupBy query. Queries that exceed this limit will fail.|Integer.MAX_VALUE (unlimited)|
Review Comment:
Im going to say "See groupBy memory tuning and resource limits for details." here and add more details to the memory-tuning-and-resource-limits section (so as to not repeat myself)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add spill file count limit for GroupBy query (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19141:
URL: https://github.com/apache/druid/pull/19141#discussion_r2923078420
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/TemporaryStorageFileLimitException.java:
##########
@@ -17,42 +17,16 @@
* under the License.
*/
-package org.apache.druid.storage.s3;
+package org.apache.druid.query.groupby.epinephelinae;
-import software.amazon.awssdk.services.s3.model.S3Object;
+import org.apache.druid.java.util.common.StringUtils;
-/**
- * Pairs an {@link S3Object} with its bucket name.
- * In AWS SDK v2, {@link S3Object} does not carry the bucket name, so callers that need it must track it separately.
- */
-public class S3ObjectWithBucket
-{
- private final String bucket;
- private final S3Object s3Object;
-
- public S3ObjectWithBucket(String bucket, S3Object s3Object)
- {
- this.bucket = bucket;
- this.s3Object = s3Object;
- }
-
- public String getBucket()
- {
- return bucket;
- }
+import java.io.IOException;
- public String getKey()
- {
- return s3Object.key();
- }
-
- public long getSize()
- {
- return s3Object.size();
- }
-
- public S3Object getS3Object()
+public class TemporaryStorageFileLimitException extends IOException
+{
+ public TemporaryStorageFileLimitException(final int fileCount)
{
- return s3Object;
+ super(StringUtils.format("Cannot write to disk, hit file count limit of %,d.", fileCount));
Review Comment:
nit: spill file count
##########
docs/configuration/index.md:
##########
@@ -2266,6 +2266,7 @@ Supported runtime properties:
|`druid.query.groupBy.maxSelectorDictionarySize`|Maximum amount of heap space (approximately) to use for per-segment string dictionaries. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxMergingDictionarySize`|Maximum amount of heap space (approximately) to use for per-query string dictionaries. When the dictionary exceeds this size, a spill to disk will be triggered. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxOnDiskStorage`|Maximum amount of disk space to use, per-query, for spilling result sets to disk when either the merging buffer or the dictionary fills up. Queries that exceed this limit will fail. Set to zero to disable disk spilling.|0 (disabled)|
+|`druid.query.groupBy.maxSpillFileCount`|Maximum number of spill files allowed per GroupBy query. Queries that exceed this limit will fail.|Integer.MAX_VALUE (unlimited)|
Review Comment:
maybe also mention why you might want to use this, either a) to prevent OOM or b) to prevent excessive spill files, while the total spill byte size is under `druid.query.groupBy.maxOnDiskStorage`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add spill file count limit for GroupBy query (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19141:
URL: https://github.com/apache/druid/pull/19141#discussion_r2923085277
##########
docs/configuration/index.md:
##########
@@ -2266,6 +2266,7 @@ Supported runtime properties:
|`druid.query.groupBy.maxSelectorDictionarySize`|Maximum amount of heap space (approximately) to use for per-segment string dictionaries. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxMergingDictionarySize`|Maximum amount of heap space (approximately) to use for per-query string dictionaries. When the dictionary exceeds this size, a spill to disk will be triggered. See [groupBy memory tuning and resource limits](../querying/groupbyquery.md#memory-tuning-and-resource-limits) for details.|100000000|
|`druid.query.groupBy.maxOnDiskStorage`|Maximum amount of disk space to use, per-query, for spilling result sets to disk when either the merging buffer or the dictionary fills up. Queries that exceed this limit will fail. Set to zero to disable disk spilling.|0 (disabled)|
+|`druid.query.groupBy.maxSpillFileCount`|Maximum number of spill files allowed per GroupBy query. Queries that exceed this limit will fail.|Integer.MAX_VALUE (unlimited)|
Review Comment:
maybe also mention why you might want to use this, either a) to prevent OOM and/or to prevent excessive spill files, while the total spill byte size is under `druid.query.groupBy.maxOnDiskStorage`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add spill file count limit for GroupBy query (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm commented on PR #19141:
URL: https://github.com/apache/druid/pull/19141#issuecomment-4050736609
> maybe this should extend InternalServerError so we can classify it? I see the other group by exception doesn't do this, but maybe that predates the exception classes
That's not needed. SpillingGrouper.aggregate() doesn't throw the Exception. It returns AggregateResult with a failure reason. That will be handled upstream.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org