You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shauryachats (via GitHub)" <gi...@apache.org> on 2025/02/03 22:41:32 UTC
[PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
shauryachats opened a new pull request, #14981:
URL: https://github.com/apache/pinot/pull/14981
A new configuration to control the size of result holders for MSE is necessary to avoid resizing and rehashing operations in use cases where grouping is needed on high-cardinality columns (e.g., UUIDs).
A simple query where it is necessary is
```
SELECT
count(*)
FROM
table_A
WHERE
(
user_uuid NOT IN (
SELECT
user_uuid
FROM
table_B
)
)
LIMIT
100 option(useMultistageEngine=true, timeoutMs=120000, useColocatedJoin = true, maxRowsInJoin = 40000000)
```
where a group by step occurs on `user_uuid` for `table_B` before the colocated join with `table_A` which has a high cardinality.
More details in the following issue: https://github.com/apache/pinot/issues/14685
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941785723
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/GroupIdGeneratorFactory.java:
##########
@@ -25,24 +25,27 @@ public class GroupIdGeneratorFactory {
private GroupIdGeneratorFactory() {
}
- public static GroupIdGenerator getGroupIdGenerator(ColumnDataType[] keyTypes, int numKeyColumns, int numGroupsLimit) {
+ public static GroupIdGenerator getGroupIdGenerator(ColumnDataType[] keyTypes, int numKeyColumns,
+ int numGroupsLimit, int maxInitialResultHolderCapacity) {
+ // Initial capacity is one more than expected to avoid rehashing if container is full.
+ int initialCapacity = 1 + Math.min(maxInitialResultHolderCapacity, numGroupsLimit);
Review Comment:
Hash table resize would happen based on load factor though right? Adding 1 here might not do much.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -756,6 +757,8 @@ public static class Server {
"pinot.server.query.executor.group.trim.size";
public static final String CONFIG_OF_QUERY_EXECUTOR_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
"pinot.server.query.executor.max.init.group.holder.capacity";
+ public static final String CONFIG_OF_QUERY_EXECUTOR_MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
+ "pinot.server.query.executor.mse.max.init.group.holder.capacity";
Review Comment:
I think we could remove `query.executor` fragment from this altogether. afaik query.executor refers to ServerQueryExecutorV1Impl and the corresponding interface, which are V1 Engine constructs. That would yield:
```
pinot.server.mse.max.init.group.holder.capacity
```
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1943839953
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -62,6 +62,7 @@ public static class AggregateOptions {
public static final String GROUP_TRIM_SIZE = "group_trim_size";
public static final String MAX_INITIAL_RESULT_HOLDER_CAPACITY = "max_initial_result_holder_capacity";
+ public static final String MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY = "mse_max_initial_result_holder_capacity";
Review Comment:
@shauryachats : this hint is not required, since the current hint only controls this value for MSE anyways. (i.e. both of them control the same thing)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941950925
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -87,7 +87,13 @@ public MultistageGroupByExecutor(
_aggType = aggType;
_leafReturnFinalResult = leafReturnFinalResult;
_resultSchema = resultSchema;
+
int maxInitialResultHolderCapacity = getMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
+ Integer mseCapacity = getMSEMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
Review Comment:
Addressed.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -87,7 +87,13 @@ public MultistageGroupByExecutor(
_aggType = aggType;
_leafReturnFinalResult = leafReturnFinalResult;
_resultSchema = resultSchema;
+
int maxInitialResultHolderCapacity = getMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
+ Integer mseCapacity = getMSEMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
+ if (mseCapacity != null) {
+ maxInitialResultHolderCapacity = mseCapacity;
Review Comment:
Addressed.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941785723
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/GroupIdGeneratorFactory.java:
##########
@@ -25,24 +25,27 @@ public class GroupIdGeneratorFactory {
private GroupIdGeneratorFactory() {
}
- public static GroupIdGenerator getGroupIdGenerator(ColumnDataType[] keyTypes, int numKeyColumns, int numGroupsLimit) {
+ public static GroupIdGenerator getGroupIdGenerator(ColumnDataType[] keyTypes, int numKeyColumns,
+ int numGroupsLimit, int maxInitialResultHolderCapacity) {
+ // Initial capacity is one more than expected to avoid rehashing if container is full.
+ int initialCapacity = 1 + Math.min(maxInitialResultHolderCapacity, numGroupsLimit);
Review Comment:
Hash table resize would happen based on load factor though right? Adding 1 here might not do much. (default is usually 0.75)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941907710
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -87,7 +87,13 @@ public MultistageGroupByExecutor(
_aggType = aggType;
_leafReturnFinalResult = leafReturnFinalResult;
_resultSchema = resultSchema;
+
int maxInitialResultHolderCapacity = getMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
+ Integer mseCapacity = getMSEMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
+ if (mseCapacity != null) {
+ maxInitialResultHolderCapacity = mseCapacity;
Review Comment:
I guess the behavior we are implementing is:
1. By default use the previous behavior
2. If a user has explicitly set a hint for the mse initial capacity, or set a server config, then use that.
Can you also call it out in the Issue Description? We'll have to update Pinot Docs too later.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1943839953
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -62,6 +62,7 @@ public static class AggregateOptions {
public static final String GROUP_TRIM_SIZE = "group_trim_size";
public static final String MAX_INITIAL_RESULT_HOLDER_CAPACITY = "max_initial_result_holder_capacity";
+ public static final String MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY = "mse_max_initial_result_holder_capacity";
Review Comment:
@shauryachats : this hint is not required, since the current hint only controls this value for MSE anyways. (i.e. both of them control the same thing). We can add a separate hint for this later if required.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941951138
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -756,6 +757,8 @@ public static class Server {
"pinot.server.query.executor.group.trim.size";
public static final String CONFIG_OF_QUERY_EXECUTOR_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
"pinot.server.query.executor.max.init.group.holder.capacity";
+ public static final String CONFIG_OF_QUERY_EXECUTOR_MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
Review Comment:
Addressed.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941903211
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -62,6 +62,7 @@ public static class AggregateOptions {
public static final String GROUP_TRIM_SIZE = "group_trim_size";
public static final String MAX_INITIAL_RESULT_HOLDER_CAPACITY = "max_initial_result_holder_capacity";
+ public static final String MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY = "mse_max_initial_result_holder_capacity";
Review Comment:
@Jackie-Jiang : do you have any recommendation for the hint name?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14981:
URL: https://github.com/apache/pinot/pull/14981#issuecomment-2632408914
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14981?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `71.11111%` with `13 lines` in your changes missing coverage. Please review.
> Project coverage is 63.55%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`cfe1760`)](https://app.codecov.io/gh/apache/pinot/commit/cfe1760159e1f4ecdb704743fb037890fdb17adc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1668 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/14981?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...va/org/apache/pinot/query/runtime/QueryRunner.java](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2FQueryRunner.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9RdWVyeVJ1bm5lci5qYXZh) | 50.00% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ry/runtime/operator/MultistageGroupByExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMultistageGroupByExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aXN0YWdlR3JvdXBCeUV4ZWN1dG9yLmphdmE=) | 54.54% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/operator/groupby/OneLongKeyGroupIdGenerator.java](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2Fgroupby%2FOneLongKeyGroupIdGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9ncm91cGJ5L09uZUxvbmdLZXlHcm91cElkR2VuZXJhdG9yLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...time/operator/groupby/GroupIdGeneratorFactory.java](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2Fgroupby%2FGroupIdGeneratorFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9ncm91cGJ5L0dyb3VwSWRHZW5lcmF0b3JGYWN0b3J5LmphdmE=) | 87.50% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14981?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14981 +/- ##
============================================
+ Coverage 61.75% 63.55% +1.80%
- Complexity 207 1389 +1182
============================================
Files 2436 2713 +277
Lines 133233 152187 +18954
Branches 20636 23532 +2896
============================================
+ Hits 82274 96724 +14450
- Misses 44911 48152 +3241
- Partials 6048 7311 +1263
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.55% <71.11%> (+1.93%)` | :arrow_up: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.54% <71.11%> (+1.79%)` | :arrow_up: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.53% <71.11%> (+35.80%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.55% <71.11%> (+1.80%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.55% <71.11%> (+1.80%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.04% <71.11%> (+9.15%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14981/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.00% <0.00%> (+6.27%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14981?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941904398
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -756,6 +757,8 @@ public static class Server {
"pinot.server.query.executor.group.trim.size";
public static final String CONFIG_OF_QUERY_EXECUTOR_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
"pinot.server.query.executor.max.init.group.holder.capacity";
+ public static final String CONFIG_OF_QUERY_EXECUTOR_MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
Review Comment:
Remove `QUERY_EXECUTOR` from the var name too
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #14981:
URL: https://github.com/apache/pinot/pull/14981
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941846709
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/GroupIdGeneratorFactory.java:
##########
@@ -25,24 +25,27 @@ public class GroupIdGeneratorFactory {
private GroupIdGeneratorFactory() {
}
- public static GroupIdGenerator getGroupIdGenerator(ColumnDataType[] keyTypes, int numKeyColumns, int numGroupsLimit) {
+ public static GroupIdGenerator getGroupIdGenerator(ColumnDataType[] keyTypes, int numKeyColumns,
+ int numGroupsLimit, int maxInitialResultHolderCapacity) {
+ // Initial capacity is one more than expected to avoid rehashing if container is full.
+ int initialCapacity = 1 + Math.min(maxInitialResultHolderCapacity, numGroupsLimit);
Review Comment:
Agreed, updated.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -756,6 +757,8 @@ public static class Server {
"pinot.server.query.executor.group.trim.size";
public static final String CONFIG_OF_QUERY_EXECUTOR_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
"pinot.server.query.executor.max.init.group.holder.capacity";
+ public static final String CONFIG_OF_QUERY_EXECUTOR_MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY =
+ "pinot.server.query.executor.mse.max.init.group.holder.capacity";
Review Comment:
Good point, updated.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1941911832
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -87,7 +87,13 @@ public MultistageGroupByExecutor(
_aggType = aggType;
_leafReturnFinalResult = leafReturnFinalResult;
_resultSchema = resultSchema;
+
int maxInitialResultHolderCapacity = getMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
+ Integer mseCapacity = getMSEMaxInitialResultHolderCapacity(opChainMetadata, nodeHint);
Review Comment:
nit: naming could be made slightly more precise (e.g. `mseMaxInitialResultHolderCapacity`).
Also you could move this logic to a separate method in the class to keep this clean.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introducing MSE result holder config to minimize rehashing for high cardinality group by [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #14981:
URL: https://github.com/apache/pinot/pull/14981#discussion_r1943839953
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -62,6 +62,7 @@ public static class AggregateOptions {
public static final String GROUP_TRIM_SIZE = "group_trim_size";
public static final String MAX_INITIAL_RESULT_HOLDER_CAPACITY = "max_initial_result_holder_capacity";
+ public static final String MSE_MAX_INITIAL_RESULT_HOLDER_CAPACITY = "mse_max_initial_result_holder_capacity";
Review Comment:
@shauryachats : this hint is not required, since the current hint only controls this value for MSE anyways. (i.e. both of them control the same thing). We can add a separate hint for this later if required.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org