[CELEBORN-2275][CIP-14] Add C++ merge-write and Java-read hybrid integration test

[afterincomparableyum]

What changes were proposed in this pull request?

Add a new C++ test client that for the mergeData/pushMergedData write path and validates data integrity by reading back from the Java ShuffleClient. This complements the existing pushData based hybrid test by covering the merge write path.

• Add DataSumWithMergeWriterClient.cpp and its CMake build target
• Add CppMergeWriteJavaReadTest entry points for NONE, LZ4, and ZSTD compression codecs
• Add runCppMergeWriteJavaRead to JavaCppHybridReadWriteTestBase
• Update cpp_integration CI workflow to run the new tests

Why are the changes needed?

This is to add integration tests for #3611.

Does this PR resolve a correctness bug?

No.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested through running unit tests and compiling locally.

[afterincomparableyum]

Will open this PR once #3611 gets merged.

[github-actions]

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[afterincomparableyum]

Update PR to rebase off of main.

[afterincomparableyum]

Ping @SteNicholas @RexXiong for review

[Copilot]

Pull request overview

This PR adds a new C++ merge-write + Java-read hybrid integration test to validate the C++ mergeData/pushMergedData write path by reading back data via the Java ShuffleClient, and wires it into CI for multiple compression codecs.

Changes:

• Add a new C++ integration test client (cppDataSumWithMergeWriterClient) that writes via merge-write and emits per-partition checksums.
• Add Scala entry points to run the new merge-write hybrid test for NONE/LZ4/ZSTD.
• Update the cpp_integration GitHub Actions workflow to execute the new merge-write hybrid tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
worker/src/test/scala/org/apache/celeborn/service/deploy/cluster/JavaCppHybridReadWriteTestBase.scala	Adds the runCppMergeWriteJavaRead test driver and data verification logic.
worker/src/test/scala/org/apache/celeborn/service/deploy/cluster/CppMergeWriteJavaReadTestWithNONE.scala	Adds NONE codec entry point for the new hybrid test.
worker/src/test/scala/org/apache/celeborn/service/deploy/cluster/CppMergeWriteJavaReadTestWithLZ4.scala	Adds LZ4 codec entry point for the new hybrid test.
worker/src/test/scala/org/apache/celeborn/service/deploy/cluster/CppMergeWriteJavaReadTestWithZSTD.scala	Adds ZSTD codec entry point for the new hybrid test.
cpp/celeborn/tests/DataSumWithMergeWriterClient.cpp	Implements the C++ merge-write test client used by the hybrid test.
cpp/celeborn/tests/CMakeLists.txt	Adds CMake targets for building the new C++ merge-writer test client.
.github/workflows/cpp_integration.yml	Runs the new merge-write hybrid tests in CI.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[SteNicholas]

⏺ Review: [CELEBORN-2275][CIP-14] Add C++ merge-write and Java-read hybrid integration test

PR: #3619 | Author: (CIP-14 contributor) | Change: +268 / -1 across 8 files

What it does

Adds integration tests for the C++ mergeData/pushMergedData write path, validated by reading data back from the Java ShuffleClient. Covers three compression codecs: NONE, LZ4, ZSTD. Mirrors the existing pushData-based hybrid test pattern.

Files changed:

• .github/workflows/cpp_integration.yml — 3 new CI steps
• cpp/celeborn/tests/CMakeLists.txt — new build target
• cpp/celeborn/tests/DataSumWithMergeWriterClient.cpp — new C++ test client
• 3 Scala test entry points (one per codec)
• JavaCppHybridReadWriteTestBase.scala — core merge-write/Java-read test logic

---

Detailed Analysis

C++ writer (DataSumWithMergeWriterClient.cpp)

• Writes data in - delimited format per partition, accumulates expected sums, writes them to a result file for the Java side to verify against.
• Uses std::rand() without srand() — deterministic (default seed), fine for reproducibility in tests.
• assert(argc == 10) compiles away under NDEBUG. Acceptable for test code but a proper error message would be more debuggable.
• Logic is correct and mirrors the existing DataSumWithWriterClient.cpp.

Java reader (JavaCppHybridReadWriteTestBase.scala :: runCppMergeWriteJavaRead)

• Parsing correctness verified: The --delimited stream (e.g. -42-7-100) is parsed byte-by-byte. The first - adds a phantom data=0 (harmless), and the last value is captured after the loop. dataCnt ends up correct because the phantom +1 and
  the missing final +1 cancel out. Correct but fragile — a comment explaining this invariant would help.
• Setup, binary execution, and validation all mirror the existing runCppWriteJavaRead pattern closely.

---

Issues

1. var should be val — var sums (~line 305) is never reassigned. Should be val.
2. Resource leak — Source.fromFile(cppResultFile, "utf-8").getLines.toList never closes the Source. Should use Using or try-finally.
3. Missing newline at EOF in cpp_integration.yml, CMakeLists.txt, and DataSumWithMergeWriterClient.cpp.
4. Parsing logic deserves a comment — The cancellation between the phantom first - and the post-loop final-value capture is non-obvious. A one-line comment would save future readers time.

---

Verdict

Approve with minor nits. This is a well-structured PR that closely follows existing patterns. The C++ write format and Java read parser are compatible — I verified the checksum logic end-to-end and found no correctness bugs. All issues are minor (style, resource handling, EOF newlines) and non-blocking.