Skip to content

refactor(workflow-operator): share the chart output contract - #9204

Merged
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:refactor/visualization-output-trait
Oct 29, 2026
Merged

aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:refactor/visualization-output-trait

Conversation

@aglinxinyuan

@aglinxinyuan aglinxinyuan commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

The 48 Python visualization descriptors each restated the same output contract: a getOutputSchemas override returning one html-content STRING column on the single output port, and producesDataFrame = false so the exported script does not print the chart as a leaf DataFrame. A new trait, PythonVisualizationOpDesc, now declares both as final members, and the 48 descriptors extend it. The 41 Plotly charts keep with PlotlyStandaloneCode.

Surface Pinned
output schema one html-content STRING column on the single output port, independent of the input schemas
exported script not printed as a leaf DataFrame
operator metadata JSON schema and operator info unchanged for all 166 operators

The trait has no fields, so no JSON schema changes. Because both members are final, a chart that overrides either one no longer compiles. VisualizationOutputSchemaSpec still 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 that producesDataFrame() 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 two VisualizationOutputSchemaSpec tests 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 in FileScanSourceOpDescSpec, a mount path in PythonUdfUiParameterSupportSpec, an exported-script comparison in UrlVizOpDescSpec); none touches the changed code.
  • sbt "WorkflowCompilingService/testOnly org.apache.texera.amber.translator.*": 41 tests pass before and after.
  • The JSON schema and operator info of all 166 registered operators, dumped before and after, are identical.
  • Mutation check: making the trait's producesDataFrame return true fails the sweep, and so does pointing one chart (BarChartOpDesc) back at PythonOperatorDescriptor with its own correct overrides; overriding either final member 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)

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.
Copilot AI balanced review requested due to automatic review settings October 28, 2026 08:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @kz930, @eugenegujing
    You can notify them by mentioning @kz930, @eugenegujing in a comment.

@codecov-commenter

codecov-commenter commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.04%. Comparing base (92e86d4) to head (d3c4aa1).

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              
Flag Coverage Δ *Carryforward flag
access-control-service 77.44% <ø> (ø)
agent-service 99.16% <ø> (ø) Carriedforward from 712c9b1
amber 86.55% <100.00%> (+0.11%) ⬆️
computing-unit-managing-service 64.02% <ø> (ø)
config-service 87.61% <ø> (ø)
file-service 82.29% <ø> (ø)
frontend 96.47% <ø> (ø) Carriedforward from 712c9b1
notebook-migration-service 83.77% <ø> (ø)
pyamber 98.54% <ø> (ø) Carriedforward from 712c9b1
workflow-compiling-service 74.25% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 0 better · 🔴 13 worse · ⚪ 2 noise (<±5%) · 0 without baseline

Compared against main 92e86d4 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

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

@aglinxinyuan
aglinxinyuan requested a review from kunwp1 October 28, 2026 20:55
@kunwp1
kunwp1 requested a lite review from Copilot October 28, 2026 23:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@kunwp1 kunwp1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Oct 29, 2026
Merged via the queue into apache:main with commit 53a8b2e Oct 29, 2026
38 checks passed
@aglinxinyuan
aglinxinyuan deleted the refactor/visualization-output-trait branch October 29, 2026 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common refactor Refactor the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Declare the chart output contract once for the Python visualization operators

4 participants