You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/07/09 03:39:28 UTC
[GitHub] [druid] gianm opened a new pull request, #12763: Automatic sizing for GroupBy dictionaries.
gianm opened a new pull request, #12763:
URL: https://github.com/apache/druid/pull/12763
Merging and selector dictionary sizes currently both default to 100MB.
This is not optimal, because it can lead to OOM on small servers and
insufficient resource utilization on larger servers. It also invites
end users to try to tune it when queries run out of dictionary space,
which can make things worse if the end user sets it to too high.
So, this patch:
- Adds automatic tuning for selector and merge dictionaries. Selectors
use up to 15% of the heap and merge buffers use up to 30% of the heap
(aggregate across all queries).
- Updates out-of-memory error messages to emphasize enabling disk
spilling vs. increasing memory parameters. With the memory parameters
automatically sized, it is more likely that an end user will get
benefit from enabling disk spilling.
- Removes the query context parameters that allow lowering of configured
dictionary sizes. These complicate the calculation, and I don't see a
reasonable use case for them.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917505168
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
How about defining the MAX as 1GiB instead of 1GB? I think 1GiB would be less confusing.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917223273
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
These two MIN and MAX ensure the range of automatic computed value, I think it's better to document them.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917223575
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -75,11 +82,11 @@
@JsonProperty
// Size of on-heap string dictionary for merging, per-processing-thread; when exceeded, partial results will be
// emitted to the merge buffer early.
- private long maxSelectorDictionarySize = 100_000_000L;
+ private long maxSelectorDictionarySize = AUTOMATIC;
@JsonProperty
// Size of on-heap string dictionary for merging, per-query; when exceeded, partial results will be spilled to disk
- private long maxMergingDictionarySize = 100_000_000L;
+ private long maxMergingDictionarySize = AUTOMATIC;
Review Comment:
I think we can change the type of `maxSelectorDictionarySize` and `maxMergingDictionarySize` from `long` to `HumanReadableBytes` to support human-readable format configuration.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917747025
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
When representing a size of memory, binary format is commonly used while decimal format is usually for disk size.
It's OK to keep it in decimal format.
--
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
[GitHub] [druid] gianm commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917449423
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
Review Comment:
I added this explanation:
```
+ // Constants for sizing merging and selector dictionaries. Rationale for these constants:
+ // 1) In no case do we want total aggregate dictionary size to exceed 40% of max memory.
+ // 2) In no case do we want any dictionary to exceed 1GB of memory: if heaps are giant, better to spill at
+ // "reasonable" sizes rather than get giant dictionaries. (There is probably some other reason the user
+ // wanted a giant heap, so we shouldn't monopolize it with dictionaries.)
+ // 3) Use somewhat more memory for merging dictionary vs. selector dictionaries, because if a merging
+ // dictionary is full we must spill to disk, whereas if a selector dictionary is full we simply emit
+ // early to the merge buffer. So, a merging dictionary filling up has a more severe impact on
+ // query performance.
```
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -75,11 +82,11 @@
@JsonProperty
// Size of on-heap string dictionary for merging, per-processing-thread; when exceeded, partial results will be
// emitted to the merge buffer early.
- private long maxSelectorDictionarySize = 100_000_000L;
+ private long maxSelectorDictionarySize = AUTOMATIC;
@JsonProperty
// Size of on-heap string dictionary for merging, per-query; when exceeded, partial results will be spilled to disk
- private long maxMergingDictionarySize = 100_000_000L;
+ private long maxMergingDictionarySize = AUTOMATIC;
Review Comment:
Good idea. I changed it.
--
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
[GitHub] [druid] gianm commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r918063273
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
Ah, I see. I wrote it as decimal since I am not familiar with that convention. I'll leave it as is for this patch.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917223471
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
BTW, these two are in decimal byte format, should we use binary byte which is 1024 * 1024 and 1024 * 1024 * 1024 ?
--
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
[GitHub] [druid] gianm commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917570413
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
Hmm. Why is it less confusing to use 1GiB instead of 1GB? The old default (100MB) was in decimal.
--
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
[GitHub] [druid] gianm commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917549536
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
Ah, I missed the comment about binary bytes. I dunno if there is a reason to use binary bytes. I kept it decimal since they both seem fine to me.
--
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
[GitHub] [druid] gianm commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917570413
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
Hmm. Why is it less confusing to use 1GiB instead of 1GB? As one data point, the old default (100MB) was in decimal.
--
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
[GitHub] [druid] gianm commented on pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12763:
URL: https://github.com/apache/druid/pull/12763#issuecomment-1180545515
Thanks for reviewing @FrankChen021 @vogievetsky
--
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
[GitHub] [druid] gianm commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917449473
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -259,20 +310,8 @@ public GroupByQueryConfig withOverrides(final GroupByQuery query)
((Number) query.getContextValue(CTX_KEY_MAX_ON_DISK_STORAGE, getMaxOnDiskStorage())).longValue(),
getMaxOnDiskStorage()
);
- newConfig.maxSelectorDictionarySize = Math.min(
- ((Number) query.getContextValue(
- CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE,
- getMaxSelectorDictionarySize()
- )).longValue(),
- getMaxSelectorDictionarySize()
- );
- newConfig.maxMergingDictionarySize = Math.min(
- ((Number) query.getContextValue(
- CTX_KEY_MAX_MERGING_DICTIONARY_SIZE,
- getMaxMergingDictionarySize()
- )).longValue(),
- getMaxMergingDictionarySize()
- );
Review Comment:
Yes, we should. I added a release notes label.
--
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
[GitHub] [druid] gianm merged pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm merged PR #12763:
URL: https://github.com/apache/druid/pull/12763
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917223643
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -259,20 +310,8 @@ public GroupByQueryConfig withOverrides(final GroupByQuery query)
((Number) query.getContextValue(CTX_KEY_MAX_ON_DISK_STORAGE, getMaxOnDiskStorage())).longValue(),
getMaxOnDiskStorage()
);
- newConfig.maxSelectorDictionarySize = Math.min(
- ((Number) query.getContextValue(
- CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE,
- getMaxSelectorDictionarySize()
- )).longValue(),
- getMaxSelectorDictionarySize()
- );
- newConfig.maxMergingDictionarySize = Math.min(
- ((Number) query.getContextValue(
- CTX_KEY_MAX_MERGING_DICTIONARY_SIZE,
- getMaxMergingDictionarySize()
- )).longValue(),
- getMaxMergingDictionarySize()
- );
Review Comment:
Since these two are removed from QueryContext, should we document it in the release notes?
--
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
[GitHub] [druid] gianm commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917449226
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
Yeah, I agree. I changed MIN to 1 byte and didn't document it (it seems unnecessary to mention). I documented the 1GB max.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917223471
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
+ private static final long MIN_AUTOMATIC_DICTIONARY_SIZE = 1_000_000;
+ private static final long MAX_AUTOMATIC_DICTIONARY_SIZE = 1_000_000_000;
Review Comment:
BTW, these two are in decimal byte format, should we use binary byte which is 1024*1024 and 1024*1024*1024 ?
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12763: Automatic sizing for GroupBy dictionaries.
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12763:
URL: https://github.com/apache/druid/pull/12763#discussion_r917223370
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryConfig.java:
##########
@@ -43,12 +48,14 @@
private static final String CTX_KEY_BUFFER_GROUPER_MAX_LOAD_FACTOR = "bufferGrouperMaxLoadFactor";
private static final String CTX_KEY_BUFFER_GROUPER_MAX_SIZE = "bufferGrouperMaxSize";
private static final String CTX_KEY_MAX_ON_DISK_STORAGE = "maxOnDiskStorage";
- private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize";
- private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize";
private static final String CTX_KEY_FORCE_HASH_AGGREGATION = "forceHashAggregation";
private static final String CTX_KEY_INTERMEDIATE_COMBINE_DEGREE = "intermediateCombineDegree";
private static final String CTX_KEY_NUM_PARALLEL_COMBINE_THREADS = "numParallelCombineThreads";
private static final String CTX_KEY_MERGE_THREAD_LOCAL = "mergeThreadLocal";
+ private static final double MERGING_DICTIONARY_HEAP_FRACTION = 0.3;
+ private static final double SELECTOR_DICTIONARY_HEAP_FRACTION = 0.10;
Review Comment:
Why it's 0.3 and 0.1 for these two variables respectively? Could you add some explanation here?
--
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