You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "aishikbh (via GitHub)" <gi...@apache.org> on 2024/02/03 20:46:44 UTC
[PR] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
aishikbh opened a new pull request, #12361:
URL: https://github.com/apache/pinot/pull/12361
Added the following helper functions:
- generateBuilderConfigs : Generates builder configs from JsonNode segment metadata
- generateDefaultConfig: Generates default config from JsonNode comnsMetadata
- areStarTreeBuilderConfigsEqual: compares if 2 builderConfig lists are equal
--
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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "aishikbh (via GitHub)" <gi...@apache.org>.
aishikbh commented on code in PR #12361:
URL: https://github.com/apache/pinot/pull/12361#discussion_r1479573707
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -83,6 +85,27 @@ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable Lis
return builderConfigs;
}
+ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
+ boolean enableDefaultStarTree, Schema schema, JsonNode segmentMetadata) {
+ List<StarTreeV2BuilderConfig> builderConfigs = new ArrayList<>();
+ if (indexConfigs != null) {
+ for (StarTreeIndexConfig indexConfig : indexConfigs) {
+ StarTreeV2BuilderConfig builderConfig = StarTreeV2BuilderConfig.fromIndexConfig(indexConfig);
+ if (!builderConfigs.contains(builderConfig)) {
Review Comment:
`equals()` is overridden properly : [here](https://github.com/apache/pinot/blob/5c81c0a88a0a95e5bf150363bb3ed5b5203de795/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/StarTreeV2BuilderConfig.java#L245-L256). reused it in the equality test as well. Thank you for pointing this out.
--
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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "aishikbh (via GitHub)" <gi...@apache.org>.
aishikbh commented on code in PR #12361:
URL: https://github.com/apache/pinot/pull/12361#discussion_r1479234913
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -83,6 +85,27 @@ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable Lis
return builderConfigs;
}
+ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
+ boolean enableDefaultStarTree, Schema schema, JsonNode segmentMetadata) {
+ List<StarTreeV2BuilderConfig> builderConfigs = new ArrayList<>();
+ if (indexConfigs != null) {
+ for (StarTreeIndexConfig indexConfig : indexConfigs) {
+ StarTreeV2BuilderConfig builderConfig = StarTreeV2BuilderConfig.fromIndexConfig(indexConfig);
+ if (!builderConfigs.contains(builderConfig)) {
+ builderConfigs.add(builderConfig);
+ }
+ }
+ }
+ if (enableDefaultStarTree) {
Review Comment:
the current behaviour is that we add default + custom right now.
Existing `generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
boolean enableDefaultStarTree, SegmentMetadata segmentMetadata)` hints at that as well.
--
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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee merged PR #12361:
URL: https://github.com/apache/pinot/pull/12361
--
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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "aishikbh (via GitHub)" <gi...@apache.org>.
aishikbh commented on code in PR #12361:
URL: https://github.com/apache/pinot/pull/12361#discussion_r1479573707
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -83,6 +85,27 @@ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable Lis
return builderConfigs;
}
+ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
+ boolean enableDefaultStarTree, Schema schema, JsonNode segmentMetadata) {
+ List<StarTreeV2BuilderConfig> builderConfigs = new ArrayList<>();
+ if (indexConfigs != null) {
+ for (StarTreeIndexConfig indexConfig : indexConfigs) {
+ StarTreeV2BuilderConfig builderConfig = StarTreeV2BuilderConfig.fromIndexConfig(indexConfig);
+ if (!builderConfigs.contains(builderConfig)) {
Review Comment:
`equals()` is properly overridden properly : [here](https://github.com/apache/pinot/blob/5c81c0a88a0a95e5bf150363bb3ed5b5203de795/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/StarTreeV2BuilderConfig.java#L245-L256)
--
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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #12361:
URL: https://github.com/apache/pinot/pull/12361#discussion_r1479216829
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -83,6 +85,27 @@ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable Lis
return builderConfigs;
}
+ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
+ boolean enableDefaultStarTree, Schema schema, JsonNode segmentMetadata) {
+ List<StarTreeV2BuilderConfig> builderConfigs = new ArrayList<>();
+ if (indexConfigs != null) {
+ for (StarTreeIndexConfig indexConfig : indexConfigs) {
+ StarTreeV2BuilderConfig builderConfig = StarTreeV2BuilderConfig.fromIndexConfig(indexConfig);
+ if (!builderConfigs.contains(builderConfig)) {
Review Comment:
`StarTreeV2BuilderConfig` <- can we double check the `equals()` function for this class?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -83,6 +85,27 @@ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable Lis
return builderConfigs;
}
+ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
+ boolean enableDefaultStarTree, Schema schema, JsonNode segmentMetadata) {
+ List<StarTreeV2BuilderConfig> builderConfigs = new ArrayList<>();
+ if (indexConfigs != null) {
+ for (StarTreeIndexConfig indexConfig : indexConfigs) {
+ StarTreeV2BuilderConfig builderConfig = StarTreeV2BuilderConfig.fromIndexConfig(indexConfig);
+ if (!builderConfigs.contains(builderConfig)) {
+ builderConfigs.add(builderConfig);
+ }
+ }
+ }
+ if (enableDefaultStarTree) {
Review Comment:
What's the current behavior if
1. custom startree config is added
2. enableDefaultStarTree is set to true
Are we adding default startree or default startree + custom startreee?
--
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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12361:
URL: https://github.com/apache/pinot/pull/12361#issuecomment-1925460370
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12361?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `35 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`ab525ee`)](https://app.codecov.io/gh/apache/pinot/commit/ab525ee9296fec122135fb5f877c45fa37f9c376?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.72% compared to head [(`15d7b68`)](https://app.codecov.io/gh/apache/pinot/pull/12361?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.67%.
> Report is 1 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12361?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...t/segment/local/startree/StarTreeBuilderUtils.java](https://app.codecov.io/gh/apache/pinot/pull/12361?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS9TdGFyVHJlZUJ1aWxkZXJVdGlscy5qYXZh) | 0.00% | [30 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12361?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...l/startree/v2/builder/StarTreeV2BuilderConfig.java](https://app.codecov.io/gh/apache/pinot/pull/12361?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL1N0YXJUcmVlVjJCdWlsZGVyQ29uZmlnLmphdmE=) | 88.37% | [1 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12361?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 #12361 +/- ##
============================================
- Coverage 61.72% 61.67% -0.05%
Complexity 207 207
============================================
Files 2426 2426
Lines 132667 132747 +80
Branches 20499 20525 +26
============================================
- Hits 81888 81874 -14
- Misses 44774 44863 +89
- Partials 6005 6010 +5
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12361/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/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.65% <52.05%> (-0.02%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.81% <0.00%> (-26.78%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.65% <52.05%> (-0.07%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.80% <0.00%> (-26.74%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.67% <52.05%> (-0.05%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.67% <52.05%> (-0.05%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.86% <0.00%> (-0.07%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12361/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.70% <52.05%> (-0.02%)` | :arrow_down: |
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/12361?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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "aishikbh (via GitHub)" <gi...@apache.org>.
aishikbh commented on code in PR #12361:
URL: https://github.com/apache/pinot/pull/12361#discussion_r1479573707
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -83,6 +85,27 @@ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable Lis
return builderConfigs;
}
+ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
+ boolean enableDefaultStarTree, Schema schema, JsonNode segmentMetadata) {
+ List<StarTreeV2BuilderConfig> builderConfigs = new ArrayList<>();
+ if (indexConfigs != null) {
+ for (StarTreeIndexConfig indexConfig : indexConfigs) {
+ StarTreeV2BuilderConfig builderConfig = StarTreeV2BuilderConfig.fromIndexConfig(indexConfig);
+ if (!builderConfigs.contains(builderConfig)) {
Review Comment:
`equals()` is overridden properly : [here](https://github.com/apache/pinot/blob/5c81c0a88a0a95e5bf150363bb3ed5b5203de795/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/StarTreeV2BuilderConfig.java#L245-L256)
--
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] Add Helper Functions in StarTreeBuilderUtils and StarTreeV2BuilderConfig [pinot]
Posted by "aishikbh (via GitHub)" <gi...@apache.org>.
aishikbh commented on code in PR #12361:
URL: https://github.com/apache/pinot/pull/12361#discussion_r1479234913
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/StarTreeBuilderUtils.java:
##########
@@ -83,6 +85,27 @@ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable Lis
return builderConfigs;
}
+ public static List<StarTreeV2BuilderConfig> generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
+ boolean enableDefaultStarTree, Schema schema, JsonNode segmentMetadata) {
+ List<StarTreeV2BuilderConfig> builderConfigs = new ArrayList<>();
+ if (indexConfigs != null) {
+ for (StarTreeIndexConfig indexConfig : indexConfigs) {
+ StarTreeV2BuilderConfig builderConfig = StarTreeV2BuilderConfig.fromIndexConfig(indexConfig);
+ if (!builderConfigs.contains(builderConfig)) {
+ builderConfigs.add(builderConfig);
+ }
+ }
+ }
+ if (enableDefaultStarTree) {
Review Comment:
yes that is the current behaviour right now.
Existing `generateBuilderConfigs(@Nullable List<StarTreeIndexConfig> indexConfigs,
boolean enableDefaultStarTree, SegmentMetadata segmentMetadata)` hints at that as well.
--
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