test(workflow-core): add unit test coverage for PhysicalPlan and PhysicalOp#6056
Open
aglinxinyuan wants to merge 1 commit into
Open
test(workflow-core): add unit test coverage for PhysicalPlan and PhysicalOp#6056aglinxinyuan wants to merge 1 commit into
aglinxinyuan wants to merge 1 commit into
Conversation
…icalOp Targets Codecov-missed lines: PhysicalPlan (71), PhysicalOp (66) — graph mutations, partition-info derivation, dependee/blocking links, maxChains, layered topological order, plan-level schema propagation, op factories, exec-code accessors, and builder guards.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Scala unit-test coverage for previously under-tested workflow-core physical planning/execution-model code (PhysicalPlan + PhysicalOp), aligning with the repository’s ongoing Codecov-driven test-coverage efforts. No production code is modified.
Changes:
- Introduces
PhysicalPlanSpeccovering link/operator mutation behavior, schema propagation through links/plan, partition selection logic, and DAG/graph utilities (bridges, chains, layering). - Introduces
PhysicalOpSpeccovering schema-propagation wrappers, factory defaults, dependee/dependency semantics, executor-code helpers, and builder/default behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PhysicalPlanSpec.scala | Adds focused unit tests for PhysicalPlan link/operator management, schema flow, partitioning decisions, and graph-derived helpers. |
| common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PhysicalOpSpec.scala | Adds focused unit tests for PhysicalOp factories, schema propagation utilities, dependency helpers, executor code accessors, and default/builder behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6056 +/- ##
============================================
+ Coverage 56.37% 56.69% +0.31%
- Complexity 2989 3023 +34
============================================
Files 1129 1129
Lines 43794 43802 +8
Branches 4743 4743
============================================
+ Hits 24689 24832 +143
+ Misses 17654 17511 -143
- Partials 1451 1459 +8
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 380 | 0.232 | 25,262/33,989/33,989 us | 🔴 +18.8% / 🔴 +129.9% |
| 🔴 | bs=100 sw=10 sl=64 | 792 | 0.483 | 128,012/138,144/138,144 us | 🟢 -12.9% / 🔴 +29.2% |
| ⚪ | bs=1000 sw=10 sl=64 | 905 | 0.552 | 1,105,475/1,149,813/1,149,813 us | ⚪ within ±5% / 🔴 +12.4% |
Baseline details
Latest main 878eb8a from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 380 tuples/sec | 404 tuples/sec | 786.27 tuples/sec | -5.9% | -51.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.232 MB/s | 0.247 MB/s | 0.48 MB/s | -6.1% | -51.7% |
| bs=10 sw=10 sl=64 | p50 | 25,262 us | 21,274 us | 12,495 us | +18.8% | +102.2% |
| bs=10 sw=10 sl=64 | p95 | 33,989 us | 36,961 us | 14,784 us | -8.0% | +129.9% |
| bs=10 sw=10 sl=64 | p99 | 33,989 us | 36,961 us | 18,468 us | -8.0% | +84.0% |
| bs=100 sw=10 sl=64 | throughput | 792 tuples/sec | 799 tuples/sec | 991.49 tuples/sec | -0.9% | -20.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.483 MB/s | 0.488 MB/s | 0.605 MB/s | -1.0% | -20.2% |
| bs=100 sw=10 sl=64 | p50 | 128,012 us | 120,726 us | 100,929 us | +6.0% | +26.8% |
| bs=100 sw=10 sl=64 | p95 | 138,144 us | 158,636 us | 106,894 us | -12.9% | +29.2% |
| bs=100 sw=10 sl=64 | p99 | 138,144 us | 158,636 us | 114,085 us | -12.9% | +21.1% |
| bs=1000 sw=10 sl=64 | throughput | 905 tuples/sec | 917 tuples/sec | 1,023 tuples/sec | -1.3% | -11.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.552 MB/s | 0.559 MB/s | 0.624 MB/s | -1.3% | -11.6% |
| bs=1000 sw=10 sl=64 | p50 | 1,105,475 us | 1,088,670 us | 983,835 us | +1.5% | +12.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,149,813 us | 1,128,821 us | 1,023,777 us | +1.9% | +12.3% |
| bs=1000 sw=10 sl=64 | p99 | 1,149,813 us | 1,128,821 us | 1,053,883 us | +1.9% | +9.1% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,526.38,200,128000,380,0.232,25262.36,33989.18,33989.18
1,100,10,64,20,2525.20,2000,1280000,792,0.483,128012.33,138143.92,138143.92
2,1000,10,64,20,22094.70,20000,12800000,905,0.552,1105474.70,1149812.57,1149812.57
This file contains hidden or 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
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 PR?
Add unit test coverage for the two largest uncovered files in
workflow-coreper the Codecov report. No production-code changes.PhysicalPlan.scalaaddOperator/addLink(incl. schema hand-off + undeclared-port rejection),removeLink,setOperator,getPhysicalOpByWorkerId,getLinksBetween,getOutputPartitionInfo(all three branches), blocking/dependee link detection,getDependeeLinksRemovedDAG,getNonBridgeNonBlockingLinks(bridge vs diamond),maxChains(maximal-subset filter),layeredReversedTopologicalOrder(diamond, parallel edges, empty), plan-levelpropagateSchemaPhysicalOp.scalaSchemaPropagationFuncwrapper, all four factory families (source/oneToOne/manyToOne/local, both overloads each),dependeeInputs/isInputLinkDependee,isPythonBased/getCode,withPartitionRequirement/withDerivePartition/withIsOneToManyOp/withPveName, partial-schema propagation gating,getInputPortDependencyPairs,addOutputLinkguards, primary-constructor defaultsNew
PhysicalPlanSpecandPhysicalOpSpecinworkflow-core(no name clash repo-wide); deliberately does not duplicate thePhysicalOp/plan surfaces already covered byWorkflowCoreTypesSpec.Any related issues, documentation, discussions?
Follow-up to the review feedback on #6043: prioritize tests that fill uncovered code paths.
How was this PR tested?
sbt "WorkflowCore/testOnly *PhysicalPlanSpec *PhysicalOpSpec"— 38 tests, all greensbt "WorkflowCore/Test/scalafmtCheck"andsbt "WorkflowCore/scalafixAll --check"— cleanWas this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8 [1M context])