You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Nemesis123925 (via GitHub)" <gi...@apache.org> on 2023/11/02 23:19:02 UTC
[PR] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Nemesis123925 opened a new pull request, #11941:
URL: https://github.com/apache/pinot/pull/11941
As Stated in the [issue/11939](https://github.com/apache/pinot/issues/11939)
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1793290339
Sure, let me try to do that
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1797119813
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11941?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11941](https://app.codecov.io/gh/apache/pinot/pull/11941?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a563090) into [master](https://app.codecov.io/gh/apache/pinot/commit/fee11d6dc5678600c021a5993ead4b9c5102c76c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fee11d6) will **decrease** coverage by `33.78%`.
> Report is 10 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11941 +/- ##
=============================================
- Coverage 61.41% 27.64% -33.78%
+ Complexity 207 201 -6
=============================================
Files 2378 2378
Lines 128865 128865
Branches 19927 19927
=============================================
- Hits 79143 35621 -43522
- Misses 44001 90567 +46566
+ Partials 5721 2677 -3044
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11941/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/11941/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/11941/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/11941/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/11941/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/11941/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%> (-61.39%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/11941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.64% <0.00%> (-33.64%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/11941/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%> (-61.41%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/11941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.64% <0.00%> (-33.62%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.64% <0.00%> (-33.78%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.63% <0.00%> (-33.78%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.63% <0.00%> (-0.01%)` | :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.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/11941?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...aggregation/function/AggregationFunctionUtils.java](https://app.codecov.io/gh/apache/pinot/pull/11941?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uVXRpbHMuamF2YQ==) | `0.00% <0.00%> (-68.94%)` | :arrow_down: |
| [...va/org/apache/pinot/core/plan/ProjectPlanNode.java](https://app.codecov.io/gh/apache/pinot/pull/11941?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1Byb2plY3RQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
... and [1162 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11941/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1797049371
I try the way that Jackie Jiang proposed to use `toString()` of `HashSet` in the test to generate the `PROJECT` clause. It doesn't seems to work because the NonDex tool, which we use to check the falkyness, use differet and random seed for all the `HashSet`. So almost in every run that the order is still not the same.
I check the way that walteraddr proposed, it seems like it needs to change the `validateRows` functions in `QueriesTestUtils.java` which is also used by other functions. Is there other better options that we can do this way?
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1793509836
I can see a simpler way is to create a `HashSet` in the test and use `toString()` to generate the `PROJECT` clause since for the same java version the order should be deterministic
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1811886481
Roger that
Here is the solution I suggest
In `QueriesTestUtils.java`, I write two new functions:
```
public static void testExplainSegmentsResult(BrokerResponseNative brokerResponse, ResultTable expectedResultTable) {
assertEquals(brokerResponse.getResultTable().getDataSchema(), expectedResultTable.getDataSchema());
validateExplainedRows(brokerResponse.getResultTable().getRows(), expectedResultTable.getRows());
}
private static void validateExplainedRows(List<Object[]> actual, List<Object[]> expected) {
assertEquals(actual.size(), expected.size());
for (int j = 0; j < actual.size(); j++) {
Object[] act = actual.get(j);
Object[] exp = expected.get(j);
String attributeToSort = "PROJECT";
assertEquals(act.length, exp.length);
if (act[0].toString().startsWith(attributeToSort)) {
char[] act0 = act[0].toString().toCharArray();
char[] exp0 = exp[0].toString().toCharArray();
Arrays.sort(act0);
Arrays.sort(exp0);
act[0] = new String(act0);
exp[0] = new String(exp0);
}
assertEquals(act, exp);
}
}
```
In `ExplainPlanQueriesTest.java`, I redirect the `checkWithQueryExecutor` to call `testExplainSegmentsResult`
If apply this fix, instead of fixing all `27` flaky tests in `ExplainPlanQueriesTest.java`, this will fix `14` of them, which are
```
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelect
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregate
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregateUsingFilterGroupBy
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregateUsingFilterGroupByVerbose
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregateUsingFilterIndex
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregateUsingFilterIndexGroupBy
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregateUsingFilterIndexGroupByHaving
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregateUsingFilterIndexGroupByOrderBy
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectAggregateUsingFilterOnTextIndexColumn
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectColumnUsingFilterOnRangeIndexColumn
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectColumnsUsingFilter
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectColumnsUsingFilterOnInvertedIndexColumn
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectColumnsVariationsOfAndOperators
org.apache.pinot.queries.ExplainPlanQueriesTest#testSelectColumnsVariationsOfOrOperators
```
If you guys think this fix is good, I will push the new commit.
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1802207720
i would suggest instead of using `QueriesTestUtils.testInterSegmentsResult`, we should create a test validator specific for explained results.
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1806891932
correct. explain query itself is not technically a "query" thus the response doesn't necessarily need to be interpretd/validated via the same method.
see https://www.postgresql.org/docs/current/using-explain.html#USING-EXPLAIN:
> If you want to feed EXPLAIN's output to a program for further analysis, you should use one of its machine-readable output formats (XML, JSON, or YAML) instead.
we are not nearly there yet for checking the plan. but i think parsing each operator and compare in unordered fashion should be suffice now
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1806995065
Got it, let me try doing it.
I also have a question such that If I created a function like `ValidateExplained`, do I need to write the test to test `ValidateExplained` function?
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1811542285
I found that a part of the non-determinism of `numSegmentsForThisPlan` happens insde of [reduceOnDataTable](https://github.com/Nemesis123925/pinot/blob/master/pinot-core/src/test/java/org/apache/pinot/queries/ExplainPlanQueriesTest.java#L416), which in my understanding it compare and select the best query generated from the two servers, and add up the `numSegments` of them if the two queries are identical, which happens in [extractUniqueExplainPlansAcrossServers](https://github.com/Nemesis123925/pinot/blob/be74dc1429e4a72c2d51946167c281f039b60165/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExplainPlanDataTableReducer.java#L131). Due to the non-determinism inside of the `PROJECT` clause created by `NonDex`, the test which suppose to have the two generates queries identical will differ from the `PROJECT` field, thus be understood as different queries by [extractUniqueExplainPlansAcrossServers](https://github.com/Nemesis123925/pinot/blob/be74dc1429e4a72c2d51946167c281
f039b60165/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExplainPlanDataTableReducer.java#L131). Thus the function will select the best one instead of adding their `numSegments` together, and create the nondeternism inside of the `numSegmentsForThisPlan` part.
For this part, I think the solution would be sorting the `PROJECT` clause before the two generated queries get into the [extractUniqueExplainPlansAcrossServers](https://github.com/Nemesis123925/pinot/blob/be74dc1429e4a72c2d51946167c281f039b60165/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExplainPlanDataTableReducer.java#L131) function, so the function will add the `numSegment` up instead of choising the better one.
However this is not the only cause of the non-deternism of the `numSegmentsForThisPlan` part. I found that applying `NonDex` will change the `numSegments` part even before the two generqte queries get into the [extractUniqueExplainPlansAcrossServers](https://github.com/Nemesis123925/pinot/blob/be74dc1429e4a72c2d51946167c281f039b60165/pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExplainPlanDataTableReducer.java#L131) function. So I will be keep exploring that part.
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1806482941
@walterddr Do you mean that I can create a different `validateRow` function for `ExplainPlanQueriesTest`, so we can solve the nondeternimism without changing the `HashSet` to `LinkedHashSet`?
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1806893124
IMO at this stage adding a special handling for PROJECT operator, the rest can be string match should be fine.
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1799016993
I took a look at the failed test and don't have too much clue on how to solve that. Can I get a hint?
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1806997098
Also I am aware of the code coverage change of my change. Can I ask the tool we are using to detect code coverage here? so I can validate the coverage myself before updating the change
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1808726430
I tried to change the `LinkedHashSet` and `LinkedHashMap` back to `HashSet` and `HashMap`, and found out that there is also nonderterminism in the `PLAN_START` node, which the `numSegmentsForThisPlan` also got randomized. While I am debuging this part, do you guys have any clue what is causing this nondeterminism?
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #11941:
URL: https://github.com/apache/pinot/pull/11941
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1811799919
i think we can separate the 2 issues, IIUC the project ordering issue is independent from the numSegmentsForThisPlan
1. the ordering issue will occur even if only 1 server is servering, thus making sure the PROJECT node compare method in test is order invariant is the main thing.
2. the numSegmentsForThisPlan IMO is really a "judgement call", b/c
- technically speaking we should return ALL plans if there are multiple returns from different servers.
- however pinot doesn't really have a "logical" vs. "physical" plan concept, where "logical" is invariant of the data layout; and "physical" should be reported on every execution path.
- the result we are comparing here is neither truly logical or physical, that it only return a single plan ("as logical") but it consolidates from multiple server return ("physical").
hence my suggestion is we address the PROJECT ordering issue first in this PR and address the plan issue separately
CC @somandal who improved upon this part of the logic to chime in :-)
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1807177544
- IMO you dont need to write test to validate test
- i usually use intellij's Run with coverage to visualize inside the IDE, but you can also use mvn plugin to report such if that's more preferrable. there's no restriction here.
Thank you for considering the test validity and coverage! this is superb!
--
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] fix flakyness by replacing HashSet and HashMap with LinkedHashSet and LinkedHashMap [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11941:
URL: https://github.com/apache/pinot/pull/11941#issuecomment-1800804684
I guess we need to modify the test to not test on the string value. @walterddr any suggestion?
--
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