Repository navigation
feat(visualization): export the charts that draw over a table's columns - #8365
Conversation
Sixteen plots whose shape comes from the columns they are pointed at: the bar, line and pie, the histograms, the scatter family, the box and violin, the strip and the ECDF, the heat map, the candlestick, the funnel and the dot plot. A line's series configuration and an error band's are one type extending the other, so they travel together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 374 | 0.228 | 25,345/35,572/35,572 us | 🔴 +14.8% / 🔴 +126.3% |
| 🔴 | bs=100 sw=10 sl=64 | 768 | 0.469 | 124,990/186,406/186,406 us | 🔴 -5.5% / 🔴 +77.0% |
| 🟢 | bs=1000 sw=10 sl=64 | 923 | 0.563 | 1,080,565/1,130,237/1,130,237 us | 🟢 -6.7% / 🔴 -12.9% |
Baseline details
Latest main 6a29c59 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 374 tuples/sec | 414 tuples/sec | 790.31 tuples/sec | -9.7% | -52.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.228 MB/s | 0.252 MB/s | 0.482 MB/s | -9.5% | -52.7% |
| bs=10 sw=10 sl=64 | p50 | 25,345 us | 22,068 us | 12,375 us | +14.8% | +104.8% |
| bs=10 sw=10 sl=64 | p95 | 35,572 us | 33,613 us | 15,719 us | +5.8% | +126.3% |
| bs=10 sw=10 sl=64 | p99 | 35,572 us | 33,613 us | 19,058 us | +5.8% | +86.6% |
| bs=100 sw=10 sl=64 | throughput | 768 tuples/sec | 813 tuples/sec | 1,025 tuples/sec | -5.5% | -25.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.469 MB/s | 0.496 MB/s | 0.626 MB/s | -5.4% | -25.0% |
| bs=100 sw=10 sl=64 | p50 | 124,990 us | 119,760 us | 98,759 us | +4.4% | +26.6% |
| bs=100 sw=10 sl=64 | p95 | 186,406 us | 180,431 us | 105,327 us | +3.3% | +77.0% |
| bs=100 sw=10 sl=64 | p99 | 186,406 us | 180,431 us | 120,491 us | +3.3% | +54.7% |
| bs=1000 sw=10 sl=64 | throughput | 923 tuples/sec | 913 tuples/sec | 1,059 tuples/sec | +1.1% | -12.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.563 MB/s | 0.557 MB/s | 0.646 MB/s | +1.1% | -12.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,080,565 us | 1,090,689 us | 965,886 us | -0.9% | +11.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,130,237 us | 1,211,605 us | 1,007,533 us | -6.7% | +12.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,130,237 us | 1,211,605 us | 1,031,622 us | -6.7% | +9.6% |
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,535.28,200,128000,374,0.228,25344.51,35571.91,35571.91
1,100,10,64,20,2604.88,2000,1280000,768,0.469,124990.02,186405.85,186405.85
2,1000,10,64,20,21668.41,20000,12800000,923,0.563,1080565.15,1130236.52,1130236.52
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (13.29%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8365 +/- ##
============================================
- Coverage 92.61% 92.32% -0.30%
+ Complexity 5059 5058 -1
============================================
Files 1258 1258
Lines 53780 53949 +169
Branches 6707 6731 +24
============================================
Hits 49811 49811
- Misses 2313 2478 +165
- Partials 1656 1660 +4
*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:
|
carloea2
left a comment
There was a problem hiding this comment.
Bubble Chart and Histogram 2D change in1df with dropna in place. This can change data used by another branch of the exported workflow. Please keep the input unchanged and test a shared upstream DataFrame.
The translator no longer emits plotly into every script; it asks the operators in the plan what they need beyond pandas. All sixteen charts here draw with plotly, so all sixteen mix in `PlotlyStandaloneCode`, which the hierarchy and graph charts introduce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
The input DataFrame mutation issue from my previous review is still present.
…d over Bubble Chart and Histogram 2D dropped rows in place; Bar Chart and the Box/Violin plot rebound the name. All four now bind their own frame. The translator names a variable per output PORT, not per reader, so two charts drawn from one upstream are handed the same name. A chart that drops its own nulls through that name leaves the chart beside it drawing a table it never received, and a single-branch workflow never notices. Both spellings did it. `inplace=True` mutates the frame outright, and `in1df = in1df.dropna(...)` reads like a local rebinding but renames the shared variable, since the operator bodies are concatenated at module scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed. Bubble Chart, Histogram 2D, Bar Chart and the Box/Violin plot now bind their own frame instead of writing to the input. Covered by the same check over every operator, which comes with the verification runner. |
carloea2
left a comment
There was a problem hiding this comment.
The charts now use a separate frame. This fixes the branch data issue.
The shared table renames the columns whose names hold the characters that end a Python literal. These annotations name one, and are test-only: they steer which column a parity run fills the field with, and have no effect in production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every chart wrote output.html and output.json, and the whole plan runs as one program in one directory, so a workflow ending in two charts kept a single picture. The name is a placeholder now, outputHtml and outputJson, and the translator hands each operator a stem of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@carloea2 this moved after your approval: each chart now names its own output files instead of sharing one. Would you take another look when you have a moment? |
carloea2
left a comment
There was a problem hiding this comment.
The charts use input copies and separate output files. Looks good.
carloea2
left a comment
There was a problem hiding this comment.
The successful 2D histogram export writes the JSON file but not the HTML file. The native operator returns HTML, and the error paths also write HTML. Please write outputHtml on success and test both files.
Every other chart here writes the figure and the page beside it, and this operator's own empty-input and all-null branches write the page too. The one branch that draws a chart wrote only the figure, so a run that succeeded left no page where the engine answers with one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in 34753c0: the branch that draws the chart now writes the page beside the figure, which is what the other fifteen charts here do and what this operator's own empty-input and all-null branches already did. Two tests cover it, one on the generated code and one running it to check both files land. |
carloea2
left a comment
There was a problem hiding this comment.
Histogram2D now writes the HTML output as well as the figure data. The earlier blocker is fixed. Looks good.
…-integration Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
carloea2
left a comment
There was a problem hiding this comment.
No new finding in the current test-tagging change. All 129 focused tests and scalafmtCheckAll pass with PlotlyStandaloneCode from merged #8344 supplied locally. Six probes also confirm Bubble and Histogram2D preserve shared inputs on valid, all-null and empty paths. The HTML and JSON output regression passes.
Command: sbt -batch "scalafmtCheckAll" "WorkflowOperator/testOnly *BarChartOpDescSpec *LineChartOpDescSpec *ContinuousErrorBandsOpDescSpec *HistogramChartOpDescSpec *Histogram2DOpDescSpec *ScatterPlotOpDescSpec *ScatterMatrixChartOpDescSpec *PieChartOpDescSpec *BubbleChartOpDescSpec *BoxViolinPlotOpDescSpec *StripChartOpDescSpec *ECDFPlotOpDescSpec *HeatMapOpDescSpec *CandlestickChartOpDescSpec *FunnelPlotOpDescSpec *DotPlotOpDescSpec *ChartSharedInputReviewSpec -- -oDF".
Please refresh the branch from main and rerun CI. Its current checks are still red, so this is not a merge-ready confirmation.
carloea2
left a comment
There was a problem hiding this comment.
The refreshed branch passes all 129 focused tests across 17 suites, including the generated-script checks for shared inputs, empty tables and nulls. Formatting passes without supplying dependency fixtures. Current CI is green. Looks good.
|
@aglinxinyuan, the refreshed branch passes all 129 focused tests and CI. I have approved it. Could you review it when you have time? |
What changes were proposed in this PR?
Sixteen plots whose shape comes from the columns they are pointed at: the
bar, line and pie, the histograms, the scatter family, the box and violin,
the strip and the ECDF, the heat map, the candlestick, the funnel and the
dot plot.
A line's series configuration and an error band's are one type extending
the other, so they travel together.
Any related issues, documentation, discussions?
Part of #8325, 14 of 27; that issue lists the set in order. It needs #8327 for the trait, so it does not compile until that lands, and the rows these operators add to the verification runner follow with the harness rather than as whole new files here.
Closes #8417, the task this change is the whole of.
How was this PR tested?
Each operator asserts the block it emits in its own spec. Once the harness lands, each is also run through the engine and through its generated script, on every configuration its schema offers, and the two answers compared.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)