incubator-uniffle icon indicating copy to clipboard operation
incubator-uniffle copied to clipboard

[#1749] feat(remote merge): Introduce new reader for reading sorted data.

Open zhengchenyu opened this issue 1 year ago • 2 comments

What changes were proposed in this pull request?

Introduce new reader for reading sorted data. Since #1748 already provides methods for merging blocks, we need to provide a method for reading merged block. The record obtained from the getSortedShuffleData method is sorted using the comparatorClassName which is passed by registerShuffle.

Why are the changes needed?

Fix: #1749

Does this PR introduce any user-facing change?

Yes, add doc in a separated issue

How was this patch tested?

unit test, integration test, test real job in cluster.

zhengchenyu avatar Aug 13 '24 03:08 zhengchenyu

Codecov Report

Attention: Patch coverage is 37.05722% with 462 lines in your changes missing coverage. Please review.

Project coverage is 52.72%. Comparing base (5ddcc28) to head (e54d40d). Report is 45 commits behind head on master.

Files Patch % Lines
...pache/uniffle/server/ShuffleServerGrpcService.java 0.00% 182 Missing :warning:
.../uniffle/client/record/reader/RMRecordsReader.java 58.50% 101 Missing and 21 partials :warning:
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java 0.00% 75 Missing :warning:
...a/org/apache/uniffle/client/record/RecordBlob.java 64.10% 13 Missing and 1 partial :warning:
...he/uniffle/client/impl/ShuffleWriteClientImpl.java 0.00% 13 Missing :warning:
...ffle/client/request/RssRegisterShuffleRequest.java 0.00% 11 Missing :warning:
.../java/org/apache/uniffle/client/record/Record.java 41.17% 10 Missing :warning:
...client/request/RssGetSortedShuffleDataRequest.java 0.00% 10 Missing :warning:
...iffle/client/request/RssStartSortMergeRequest.java 0.00% 10 Missing :warning:
...ient/response/RssGetSortedShuffleDataResponse.java 0.00% 8 Missing :warning:
... and 5 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2034      +/-   ##
============================================
- Coverage     52.77%   52.72%   -0.05%     
- Complexity     2498     2667     +169     
============================================
  Files           398      423      +25     
  Lines         18135    19946    +1811     
  Branches       1660     1847     +187     
============================================
+ Hits           9570    10516     +946     
- Misses         7981     8765     +784     
- Partials        584      665      +81     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Aug 13 '24 04:08 codecov-commenter

Test Results

 2 882 files  + 75   2 882 suites  +75   5h 56m 46s :stopwatch: + 3m 26s  1 019 tests + 27   1 018 :white_check_mark: + 28   1 :zzz: ±0  0 :x: ±0  12 868 runs  +405  12 853 :white_check_mark: +406  15 :zzz: ±0  0 :x: ±0 

Results for commit 727d072e. ± Comparison against base commit a6aefcb3.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Aug 13 '24 04:08 github-actions[bot]

@zhengchenyu Could you add documents in an another pull request?

jerqi avatar Sep 11 '24 03:09 jerqi

@zhengchenyu Could you add documents in an another pull request?

Yes, I will. When the main feature are merged, I will submit a document.

zhengchenyu avatar Sep 11 '24 03:09 zhengchenyu