Repository navigation
refactor(workflow-operator): share the chart output contract - #9204
Conversation
The 48 Python visualization descriptors each overrode getOutputSchemas to return one html-content STRING column and set producesDataFrame to false. PythonVisualizationOpDesc now declares both, and the descriptors extend it. Schemas, generated Python and operator metadata are unchanged.
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9204 +/- ##
============================================
+ Coverage 91.98% 92.04% +0.05%
+ Complexity 5083 5038 -45
============================================
Files 1285 1286 +1
Lines 56094 55910 -184
Branches 7137 7137
============================================
- Hits 51599 51463 -136
+ Misses 2742 2694 -48
Partials 1753 1753
*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:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 516 | 0.315 | 18,551/26,868/26,868 us | 🔴 +21.6% / 🔴 +91.5% |
| 🔴 | bs=100 sw=10 sl=64 | 1,113 | 0.679 | 87,811/124,951/124,951 us | 🔴 +34.9% / 🔴 +29.8% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,324 | 0.808 | 755,636/800,509/800,509 us | 🔴 +13.6% / 🟢 -16.9% |
Baseline details
Latest main 92e86d4 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 516 tuples/sec | 598 tuples/sec | 905.73 tuples/sec | -13.7% | -43.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.315 MB/s | 0.365 MB/s | 0.553 MB/s | -13.7% | -43.0% |
| bs=10 sw=10 sl=64 | p50 | 18,551 us | 15,257 us | 11,331 us | +21.6% | +63.7% |
| bs=10 sw=10 sl=64 | p95 | 26,868 us | 26,323 us | 14,032 us | +2.1% | +91.5% |
| bs=10 sw=10 sl=64 | p99 | 26,868 us | 26,323 us | 17,338 us | +2.1% | +55.0% |
| bs=100 sw=10 sl=64 | throughput | 1,113 tuples/sec | 1,339 tuples/sec | 1,205 tuples/sec | -16.9% | -7.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.679 MB/s | 0.817 MB/s | 0.735 MB/s | -16.9% | -7.7% |
| bs=100 sw=10 sl=64 | p50 | 87,811 us | 73,793 us | 89,237 us | +19.0% | -1.6% |
| bs=100 sw=10 sl=64 | p95 | 124,951 us | 92,595 us | 96,280 us | +34.9% | +29.8% |
| bs=100 sw=10 sl=64 | p99 | 124,951 us | 92,595 us | 111,344 us | +34.9% | +12.2% |
| bs=1000 sw=10 sl=64 | throughput | 1,324 tuples/sec | 1,504 tuples/sec | 1,239 tuples/sec | -12.0% | +6.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.808 MB/s | 0.918 MB/s | 0.756 MB/s | -12.0% | +6.9% |
| bs=1000 sw=10 sl=64 | p50 | 755,636 us | 665,095 us | 877,826 us | +13.6% | -13.9% |
| bs=1000 sw=10 sl=64 | p95 | 800,509 us | 713,052 us | 919,412 us | +12.3% | -12.9% |
| bs=1000 sw=10 sl=64 | p99 | 800,509 us | 713,052 us | 963,000 us | +12.3% | -16.9% |
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,387.27,200,128000,516,0.315,18550.72,26868.09,26868.09
1,100,10,64,20,1796.64,2000,1280000,1113,0.679,87810.59,124951.31,124951.31
2,1000,10,64,20,15103.60,20000,12800000,1324,0.808,755636.30,800509.46,800509.46
kunwp1
left a comment
There was a problem hiding this comment.
LGTM! Left some minor comments.
getOutputSchemas and producesDataFrame are now final in PythonVisualizationOpDesc, so a chart cannot override either. VisualizationOutputSchemaSpec checks that every visualization descriptor other than HtmlViz and UrlViz extends the trait, and asserts that producesDataFrame is false next to the schema.
What changes were proposed in this PR?
The 48 Python visualization descriptors each restated the same output contract: a
getOutputSchemasoverride returning onehtml-contentSTRING column on the single output port, andproducesDataFrame = falseso the exported script does not print the chart as a leaf DataFrame. A new trait,PythonVisualizationOpDesc, now declares both asfinalmembers, and the 48 descriptors extend it. The 41 Plotly charts keepwith PlotlyStandaloneCode.html-contentSTRING column on the single output port, independent of the input schemasThe trait has no fields, so no JSON schema changes. Because both members are
final, a chart that overrides either one no longer compiles.VisualizationOutputSchemaSpecstill sweeps every registered chart. It now also requires every chart except the two Scala map operators, HtmlViz and UrlViz, to extend the trait, and asserts thatproducesDataFrame()is false next to the schema.Any related issues, documentation, discussions?
Closes #9203.
How was this PR tested?
sbt "WorkflowOperator/test": each of its 2,769 tests (295 suites) has the same result before and after this change, compared test by test from the JUnit XML reports; the twoVisualizationOutputSchemaSpectests whose assertions grew were renamed and pass. Three tests fail on main and on this branch alike on the Windows machine used (a locked temp file inFileScanSourceOpDescSpec, a mount path inPythonUdfUiParameterSupportSpec, an exported-script comparison inUrlVizOpDescSpec); none touches the changed code.sbt "WorkflowCompilingService/testOnly org.apache.texera.amber.translator.*": 41 tests pass before and after.producesDataFramereturn true fails the sweep, and so does pointing one chart (BarChartOpDesc) back atPythonOperatorDescriptorwith its own correct overrides; overriding eitherfinalmember in a chart fails to compile.sbt "WorkflowOperator/scalafmtCheck" "WorkflowOperator/Test/scalafmtCheck" "WorkflowOperator/scalafix --check" "WorkflowOperator/Test/scalafix --check"Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)