You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "saurabhd336 (via GitHub)" <gi...@apache.org> on 2023/02/06 10:52:06 UTC
[GitHub] [pinot] saurabhd336 opened a new pull request, #10231: (WIP) StarTree non deterministic ordering of functions in startree_index_map and other places
saurabhd336 opened a new pull request, #10231:
URL: https://github.com/apache/pinot/pull/10231
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10231: (WIP) StarTree non deterministic ordering of functions in startree_index_map and other places
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10231:
URL: https://github.com/apache/pinot/pull/10231#issuecomment-1422478902
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10231](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (403f039) into [master](https://codecov.io/gh/apache/pinot/commit/b50c58fcd388519572a4863857fe385d2ee63f00?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b50c58f) will **decrease** coverage by `38.27%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10231 +/- ##
=============================================
- Coverage 70.42% 32.16% -38.27%
+ Complexity 5756 193 -5563
=============================================
Files 2016 2016
Lines 109154 109101 -53
Branches 16583 16583
=============================================
- Hits 76868 35087 -41781
- Misses 26916 70877 +43961
+ Partials 5370 3137 -2233
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.67% <0.00%> (+0.01%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.71% <0.00%> (+0.01%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ocal/startree/v2/builder/MultipleTreesBuilder.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL011bHRpcGxlVHJlZXNCdWlsZGVyLmphdmE=) | `0.00% <0.00%> (-88.68%)` | :arrow_down: |
| [...cal/startree/v2/builder/StarTreeIndexCombiner.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL1N0YXJUcmVlSW5kZXhDb21iaW5lci5qYXZh) | `0.00% <0.00%> (-96.00%)` | :arrow_down: |
| [...l/startree/v2/builder/StarTreeV2BuilderConfig.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9idWlsZGVyL1N0YXJUcmVlVjJCdWlsZGVyQ29uZmlnLmphdmE=) | `0.00% <0.00%> (-67.61%)` | :arrow_down: |
| [...local/startree/v2/store/StarTreeIndexMapUtils.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zdGFydHJlZS92Mi9zdG9yZS9TdGFyVHJlZUluZGV4TWFwVXRpbHMuamF2YQ==) | `0.00% <0.00%> (-87.50%)` | :arrow_down: |
| [.../index/startree/AggregationFunctionColumnPair.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L3N0YXJ0cmVlL0FnZ3JlZ2F0aW9uRnVuY3Rpb25Db2x1bW5QYWlyLmphdmE=) | `0.00% <0.00%> (-75.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1262 more](https://codecov.io/gh/apache/pinot/pull/10231?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10231: (WIP) StarTree non deterministic ordering of functions in startree_index_map and other places
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #10231:
URL: https://github.com/apache/pinot/pull/10231#discussion_r1099963548
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/StarTreeIndexCombiner.java:
##########
@@ -54,7 +54,7 @@ public StarTreeIndexCombiner(File indexFile)
*/
public Map<IndexKey, IndexValue> combine(StarTreeV2BuilderConfig builderConfig, File starTreeIndexDir)
throws IOException {
- Map<IndexKey, IndexValue> indexMap = new HashMap<>();
+ Map<IndexKey, IndexValue> indexMap = new TreeMap<>();
Review Comment:
Ack
--
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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10231: (WIP) StarTree non deterministic ordering of functions in startree_index_map and other places
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #10231:
URL: https://github.com/apache/pinot/pull/10231#discussion_r1099956583
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/BaseSingleTreeBuilder.java:
##########
@@ -131,7 +132,7 @@ static class Record {
"Dimension: " + dimension + " does not have dictionary");
}
- Set<AggregationFunctionColumnPair> functionColumnPairs = builderConfig.getFunctionColumnPairs();
+ Set<AggregationFunctionColumnPair> functionColumnPairs = new TreeSet<>(builderConfig.getFunctionColumnPairs());
Review Comment:
Good catch. Verified all instances are created with TreeSet. 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
[GitHub] [pinot] Jackie-Jiang merged pull request #10231: StarTree non deterministic ordering of functions in startree_index_map and other places
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10231:
URL: https://github.com/apache/pinot/pull/10231
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10231: (WIP) StarTree non deterministic ordering of functions in startree_index_map and other places
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10231:
URL: https://github.com/apache/pinot/pull/10231#discussion_r1098055170
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/BaseSingleTreeBuilder.java:
##########
@@ -131,7 +132,7 @@ static class Record {
"Dimension: " + dimension + " does not have dictionary");
}
- Set<AggregationFunctionColumnPair> functionColumnPairs = builderConfig.getFunctionColumnPairs();
+ Set<AggregationFunctionColumnPair> functionColumnPairs = new TreeSet<>(builderConfig.getFunctionColumnPairs());
Review Comment:
This extra map is redundant as the builder config already create the pairs as `TreeSet`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/store/StarTreeIndexMapUtils.java:
##########
@@ -112,6 +112,14 @@ public boolean equals(@Nullable Object obj) {
return false;
}
}
+
+ @Override
+ public int compareTo(IndexKey other) {
+ return Comparator
+ .comparing((IndexKey i) -> i._column, Comparator.nullsLast(Comparator.naturalOrder()))
+ .thenComparing((IndexKey i) -> i._indexType, Comparator.nullsLast(Comparator.naturalOrder()))
Review Comment:
(minor) index type cannot be null
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumnPair.java:
##########
@@ -86,4 +87,12 @@ public boolean equals(Object obj) {
public String toString() {
return toColumnName();
}
+
+ @Override
+ public int compareTo(AggregationFunctionColumnPair other) {
+ return Comparator.comparing((AggregationFunctionColumnPair o) -> o._column,
+ Comparator.nullsLast(Comparator.naturalOrder()))
Review Comment:
(minor) neither of these 2 fields can be null
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/builder/StarTreeIndexCombiner.java:
##########
@@ -54,7 +54,7 @@ public StarTreeIndexCombiner(File indexFile)
*/
public Map<IndexKey, IndexValue> combine(StarTreeV2BuilderConfig builderConfig, File starTreeIndexDir)
throws IOException {
- Map<IndexKey, IndexValue> indexMap = new HashMap<>();
+ Map<IndexKey, IndexValue> indexMap = new TreeMap<>();
Review Comment:
Suggest making it a `List<Pair<IndexKey, IndexValue>>` instead of a `TreeMap` because we don't need to dedup
--
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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10231: (WIP) StarTree non deterministic ordering of functions in startree_index_map and other places
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #10231:
URL: https://github.com/apache/pinot/pull/10231#discussion_r1099964523
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/startree/v2/store/StarTreeIndexMapUtils.java:
##########
@@ -112,6 +112,14 @@ public boolean equals(@Nullable Object obj) {
return false;
}
}
+
+ @Override
+ public int compareTo(IndexKey other) {
+ return Comparator
+ .comparing((IndexKey i) -> i._column, Comparator.nullsLast(Comparator.naturalOrder()))
+ .thenComparing((IndexKey i) -> i._indexType, Comparator.nullsLast(Comparator.naturalOrder()))
Review Comment:
Ack
--
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