test(verify): run an operator through the engine and keep what it wrote - #8356
Conversation
An operator's answer cannot be compared against anything until there is a way to get one. `OpExecHarness` builds the physical operator a descriptor describes, feeds it the rows of a JSONL file per input port, and writes what each output port produced back out, schema in a sidecar because JSONL carries values alone and cannot say a column is an integer rather than a number. The tests that need a Python interpreter are tagged and split into a job that provisions one, so the job that does not stays as fast as it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8356 +/- ##
============================================
+ Coverage 93.62% 93.63% +0.01%
Complexity 4842 4842
============================================
Files 1211 1211
Lines 49991 49991
Branches 6125 6125
============================================
+ Hits 46805 46811 +6
+ Misses 1675 1672 -3
+ Partials 1511 1508 -3
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 382 | 0.233 | 25,745/33,986/33,986 us | 🔴 +25.7% / 🔴 +110.7% |
| 🔴 | bs=100 sw=10 sl=64 | 945 | 0.577 | 104,993/135,798/135,798 us | 🔴 +18.2% / 🔴 +24.9% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,081 | 0.66 | 908,149/1,064,685/1,064,685 us | 🔴 +13.2% / 🟢 -8.9% |
Baseline details
Latest main 4d7fd49 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 382 tuples/sec | 461 tuples/sec | 755.28 tuples/sec | -17.1% | -49.4% |
| bs=10 sw=10 sl=64 | MB/s | 0.233 MB/s | 0.281 MB/s | 0.461 MB/s | -17.1% | -49.5% |
| bs=10 sw=10 sl=64 | p50 | 25,745 us | 20,484 us | 12,957 us | +25.7% | +98.7% |
| bs=10 sw=10 sl=64 | p95 | 33,986 us | 29,676 us | 16,134 us | +14.5% | +110.7% |
| bs=10 sw=10 sl=64 | p99 | 33,986 us | 29,676 us | 20,333 us | +14.5% | +67.1% |
| bs=100 sw=10 sl=64 | throughput | 945 tuples/sec | 976 tuples/sec | 980.1 tuples/sec | -3.2% | -3.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.577 MB/s | 0.596 MB/s | 0.598 MB/s | -3.2% | -3.5% |
| bs=100 sw=10 sl=64 | p50 | 104,993 us | 103,308 us | 101,894 us | +1.6% | +3.0% |
| bs=100 sw=10 sl=64 | p95 | 135,798 us | 114,893 us | 108,718 us | +18.2% | +24.9% |
| bs=100 sw=10 sl=64 | p99 | 135,798 us | 114,893 us | 122,482 us | +18.2% | +10.9% |
| bs=1000 sw=10 sl=64 | throughput | 1,081 tuples/sec | 1,102 tuples/sec | 1,011 tuples/sec | -1.9% | +6.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.66 MB/s | 0.673 MB/s | 0.617 MB/s | -1.9% | +6.9% |
| bs=1000 sw=10 sl=64 | p50 | 908,149 us | 909,443 us | 996,422 us | -0.1% | -8.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,064,685 us | 940,438 us | 1,037,670 us | +13.2% | +2.6% |
| bs=1000 sw=10 sl=64 | p99 | 1,064,685 us | 940,438 us | 1,072,152 us | +13.2% | -0.7% |
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,522.99,200,128000,382,0.233,25744.62,33986.17,33986.17
1,100,10,64,20,2117.03,2000,1280000,945,0.577,104992.72,135797.72,135797.72
2,1000,10,64,20,18495.01,20000,12800000,1081,0.660,908148.99,1064684.51,1064684.51Both harnesses run the same physical plan; only the executor behind it differs. Preparing that plan is the same work either way: propagate schemas at the external input ports, and refuse a fixture set that does not cover them. Widening the two to `private[verify]` lets the Python harness call them instead of carrying a second copy that has to be kept in step by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three kinds of comment came out. A drawing of the string the code below assembles. A restatement of a branch the reader can see. And the word MVP, which dated the scope to a moment rather than stating it. What replaces them says the same thing shorter, or says what the code cannot: which cases the harness does not drive and why none of them has an operator asking for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The filter routes the Python-forking specs to the job that provisions Python and keeps them out of the fast unit job. It selects on `IntegrationTest`, which this change introduces, so the two belong together: configured any earlier it selects on a tag nothing carries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
The engine harness looks good.
|
@aglinxinyuan may you take a look at this? |
|
/request-review @mengw15 |
The wiring is read from getPhysicalPlan, which is why a join, a split and a source all work; the enumeration ahead of that said the same thing twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It restated its own signature and listed a codec table that lives in TupleIO. The reasons a reader cannot derive stay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/request-review @aglinxinyuan |
…rator-in-engine # Conflicts: # workflow-compiling-service/build.sbt
|
@aglinxinyuan could you re-add this to the merge queue? It was dropped because Docker Hub refused the MinIO testcontainer pull ( |
What changes were proposed in this PR?
An operator's answer cannot be compared against anything until there is a way
to get one.
OpExecHarnessbuilds the physical operator a descriptordescribes, feeds it the rows of a JSONL file per input port, and writes what
each output port produced back out, schema in a sidecar because JSONL carries
values alone and cannot say a column is an integer rather than a number.
The tests that need a Python interpreter are tagged and split into a job that
provisions one, so the job that does not stays as fast as it was.
Any related issues, documentation, discussions?
Part of #8325, 3 of 27; that issue lists the set in order.
Closes #8408, the task this change is the whole of.
How was this PR tested?
The tests in this change cover it. The whole set is exercised together once the last piece lands: every operator run through the engine and through its generated script, and the two answers compared.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)