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/27 07:20:10 UTC
[PR] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Nemesis123925 opened a new pull request, #12048:
URL: https://github.com/apache/pinot/pull/12048
After the this [PR](https://github.com/apache/pinot/pull/11941), I continue to explore how to fix the unfixed test by that PR.
In this [section](https://github.com/apache/pinot/pull/11941#issuecomment-1811542285), I said that [reduceOnDataTable](https://github.com/Nemesis123925/pinot/blob/master/pinot-core/src/test/java/org/apache/pinot/queries/ExplainPlanQueriesTest.java#L416) might be contributing to the non-determinism inside of the `numSegmentsForThisPlan`. But that's not the only cause. After exploring, I found that when nonDex is applied, the two `queries` generated by the two `servers` might be completely differet. There is once that the produced and selected `queries` only have two rows inside.
So here I resubmit the initial commit that I did on the previous PR, because the non-determinism inside these tests can actually cause more trouble that can not be solved by sorting the `PROJECT` field. While the initial commit completely solve the non-determinism, it also cause the code coverage to change, and a test from other class to fail.
By Opening this PR, I hope to reexplore the possibility to modifiy on my original commit, to make it has less code coverage changes and no other test failures, because this way completely sove the root of non-determinism instead of just changing it only in the test part. At the same time, I hope to communicate with @somandal, who is expert in this field, to talk about the possibility of using other fixs to eliminiate the non-determinism.
--
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] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #12048:
URL: https://github.com/apache/pinot/pull/12048#issuecomment-1841826671
Yes with the help of `Nondex`, it's not guaranteed to show everytime but it can be reproduced.
Can use `mvn -pl <module_name> edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=<test_name>` to reproduce
--
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] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12048:
URL: https://github.com/apache/pinot/pull/12048#issuecomment-1827311461
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12048?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`f79b618`)](https://app.codecov.io/gh/apache/pinot/commit/f79b618141e07c140663791959d96956ad91d811?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.62% compared to head [(`86e561a`)](https://app.codecov.io/gh/apache/pinot/pull/12048?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.79%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12048 +/- ##
=============================================
- Coverage 61.62% 46.79% -14.84%
+ Complexity 1151 946 -205
=============================================
Files 2386 1788 -598
Lines 129567 93953 -35614
Branches 20053 15192 -4861
=============================================
- Hits 79847 43962 -35885
- Misses 43913 46875 +2962
+ Partials 5807 3116 -2691
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12048/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/12048/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/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12048/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/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12048/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/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.79% <100.00%> (-14.72%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.74% <100.00%> (-14.87%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.76% <100.00%> (-14.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.79% <100.00%> (-14.84%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.79% <100.00%> (-14.83%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.79% <100.00%> (-0.15%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12048/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/12048?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] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #12048:
URL: https://github.com/apache/pinot/pull/12048#issuecomment-1846341253
I am still searching for the root cause of this length change, but changing to `LinkedHashSet` does make all the flakiness disappear. While I am searching for the result, can somebody give a hint of how could possible cause 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] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Posted by "Nemesis123925 (via GitHub)" <gi...@apache.org>.
Nemesis123925 commented on PR #12048:
URL: https://github.com/apache/pinot/pull/12048#issuecomment-1839961033
Yes I got this part. If the issue if only about ordering of some unimportant field, it's not worthy to change the main code which influence the performance. But after testing, I found that some of the issues are not only about ordering, but about length of the two queries provided.
As I run the test with `NonDex`
```
[ERROR] org.apache.pinot.queries.ExplainPlanQueriesTest.testSelectColumnsVariationsOfAndOperatorsVerbose -- Time elapsed: 0.006 s <<< FAILURE!
java.lang.AssertionError: expected [7] but found [12]
```
This failure is about the first line in `validateRows`, which is asserting on the `length` of generated query and expected query. Difference on this part could lead to more serious bugs, and I think also has less relation with the test code.
However I am not the expert of this project. I might have understood something wrong in the code and if there is a way to fix it without changing the main code I will definitely take it . If someone can give me more hints on this part I will be really appreciating.
--
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] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12048:
URL: https://github.com/apache/pinot/pull/12048#issuecomment-1843745021
Hmm, are you saying that if we use `HashSet` it can cause wrong behavior? I still don't follow how could it return different length
--
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] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12048:
URL: https://github.com/apache/pinot/pull/12048#issuecomment-1851167202
Checking the code, and I feel the problem is raised from the `ExplainPlanDataTableReducer.extractUniqueExplainPlansAcrossServers()` where we try to extract the unique explain plans. The equivalent plan might be count as different when order is non-deterministic. @somandal Can you help take a look?
--
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] Try Fix By Replacing HashSet by LinkedHashSet [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #12048:
URL: https://github.com/apache/pinot/pull/12048#issuecomment-1841705270
@Nemesis123925 This failure seems not related to the set ordering. Can you constantly reproduce it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@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