[BUG][AQE][LocalOrder] Remove check of discontinuous map task ids #354
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
Codecov Report
@@ Coverage Diff @@
## master #354 +/- ##
============================================
- Coverage 58.40% 58.30% -0.10%
+ Complexity 1554 1550 -4
============================================
Files 193 193
Lines 10758 10754 -4
Branches 935 936 +1
============================================
- Hits 6283 6270 -13
- Misses 4104 4111 +7
- Partials 371 373 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jerqi
reviewed
Nov 23, 2022
| @@ -128,23 +129,13 @@ public List<ShuffleDataSegment> split(ShuffleIndexResult shuffleIndexResult) { | |||
| } | |||
|
|
|||
| if (expectTaskIds.contains(taskAttemptId)) { | |||
| if (bufferOffset != 0 && index - lastExpectedBlockIndex > 1) { | |||
There was a problem hiding this comment.
This checking logic is to check the discontinuous blocks in a ordered sequence. So this will throw exception when encountering the discontinuous task map ids
jerqi
reviewed
Nov 23, 2022
| && bufferSegments.size() > 0 | ||
| && (expectTaskIds.contains(taskAttemptId) ? index - lastExpectedBlockIndex != 1 : true)) | ||
| || bufferOffset >= readBufferSize) { | ||
| boolean condition1 = |
There was a problem hiding this comment.
Could we give this a better name?
There was a problem hiding this comment.
conditionOfdiscontinuousBlocks
jerqi
reviewed
Nov 23, 2022
| && expectTaskIds.contains(taskAttemptId) | ||
| && index - lastExpectedBlockIndex != 1; | ||
|
|
||
| boolean condition2 = bufferOffset >= readBufferSize; |
OK. Let me take a look. By the way, the local_order looks good in our internal environment. But If having the memory skipping, AQE will benefit more. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Remove check of discontinuous map task ids
Why are the changes needed?
In the [startMapIndex, endMapIndex) sequence, the index is not the map task id, which only indicate the range of the succeed map task ids set.
So that means. if the range is [0, 5), it only indicate the number of tasks is 5. And the task id list may be as follows:
[task-1, task-3, task-9, task-15, task-16]Does this PR introduce any user-facing change?
No
How was this patch tested?