-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-33756][SQL] Make BytesToBytesMap's MapIterator idempotent #30728
Conversation
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #132645 has finished for PR 30728 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #132656 has finished for PR 30728 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #132748 has finished for PR 30728 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #132761 has finished for PR 30728 at commit
|
core/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java
Outdated
Show resolved
Hide resolved
51e4652
to
f52d851
Compare
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #132836 has finished for PR 30728 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #132864 has finished for PR 30728 at commit
|
Just happen to find the error and I've submitted a PR(#30823) to fix the flaky test in Github Action :) |
Very thanks.. |
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #133022 has finished for PR 30728 at commit
|
### What changes were proposed in this pull request? Make MapIterator of BytesToBytesMap `hasNext` method idempotent ### Why are the changes needed? The `hasNext` maybe called multiple times, if not guarded, second call of hasNext method after reaching the end of iterator will throw NoSuchElement exception. ### Does this PR introduce _any_ user-facing change? NO. ### How was this patch tested? Update a unit test to cover this case. Closes #30728 from advancedxy/SPARK-33756. Authored-by: Xianjin YE <advancedxy@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit 1339168) Signed-off-by: Sean Owen <srowen@gmail.com>
### What changes were proposed in this pull request? Make MapIterator of BytesToBytesMap `hasNext` method idempotent ### Why are the changes needed? The `hasNext` maybe called multiple times, if not guarded, second call of hasNext method after reaching the end of iterator will throw NoSuchElement exception. ### Does this PR introduce _any_ user-facing change? NO. ### How was this patch tested? Update a unit test to cover this case. Closes #30728 from advancedxy/SPARK-33756. Authored-by: Xianjin YE <advancedxy@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit 1339168) Signed-off-by: Sean Owen <srowen@gmail.com>
### What changes were proposed in this pull request? Make MapIterator of BytesToBytesMap `hasNext` method idempotent ### Why are the changes needed? The `hasNext` maybe called multiple times, if not guarded, second call of hasNext method after reaching the end of iterator will throw NoSuchElement exception. ### Does this PR introduce _any_ user-facing change? NO. ### How was this patch tested? Update a unit test to cover this case. Closes #30728 from advancedxy/SPARK-33756. Authored-by: Xianjin YE <advancedxy@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit 1339168) Signed-off-by: Sean Owen <srowen@gmail.com>
Merged to master/3.1/3.0/2.4 |
thanks, sean |
What changes were proposed in this pull request?
Make MapIterator of BytesToBytesMap
hasNext
method idempotentWhy are the changes needed?
The
hasNext
maybe called multiple times, if not guarded, second call of hasNext method after reaching the end of iterator will throw NoSuchElement exception.Does this PR introduce any user-facing change?
NO.
How was this patch tested?
Update a unit test to cover this case.