feat(visualization): require the Filled Area Plot line group under its switch - #8285
Open
kz930 wants to merge 1 commit into
Open
feat(visualization): require the Filled Area Plot line group under its switch#8285kz930 wants to merge 1 commit into
kz930 wants to merge 1 commit into
Conversation
…s switch Turning on Split Plot by Line Group makes the line group required, because code generation asserts it and the run ends on `Line Group cannot be empty`. The field is declared optional and carried no conditional constraint, so the property panel accepted the configuration and the error waited for the run. A field that is optional is exactly the one a user leaves behind. The schema states the rule instead, in the conditional form the Sklearn text columns already use: required under `facetColumn`, so the panel refuses the configuration while it is being written. Conditional rather than a plain required, so a freshly dropped operator, whose switch is off, is not flagged for a field it has no use for. The assertion stays as the last line of defence, and the tests that pin it stay with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8285 +/- ##
============================================
- Coverage 93.93% 93.93% -0.01%
Complexity 4761 4761
============================================
Files 1191 1191
Lines 48308 48308
Branches 5377 5377
============================================
- Hits 45378 45376 -2
- Misses 1495 1497 +2
Partials 1435 1435
*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 | 357 | 0.218 | 25,644/47,613/47,613 us | 🔴 +21.5% / 🔴 +222.0% |
| 🔴 | bs=100 sw=10 sl=64 | 778 | 0.475 | 124,562/152,087/152,087 us | 🟢 -10.3% / 🔴 +50.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 928 | 0.566 | 1,071,516/1,133,416/1,133,416 us | ⚪ within ±5% / 🔴 -17.2% |
Baseline details
Latest main 50321e4 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 357 tuples/sec | 414 tuples/sec | 844.57 tuples/sec | -13.8% | -57.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.218 MB/s | 0.252 MB/s | 0.515 MB/s | -13.5% | -57.7% |
| bs=10 sw=10 sl=64 | p50 | 25,644 us | 21,114 us | 11,837 us | +21.5% | +116.6% |
| bs=10 sw=10 sl=64 | p95 | 47,613 us | 39,607 us | 14,788 us | +20.2% | +222.0% |
| bs=10 sw=10 sl=64 | p99 | 47,613 us | 39,607 us | 17,651 us | +20.2% | +169.7% |
| bs=100 sw=10 sl=64 | throughput | 778 tuples/sec | 835 tuples/sec | 1,084 tuples/sec | -6.8% | -28.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.475 MB/s | 0.51 MB/s | 0.662 MB/s | -6.9% | -28.2% |
| bs=100 sw=10 sl=64 | p50 | 124,562 us | 116,875 us | 94,505 us | +6.6% | +31.8% |
| bs=100 sw=10 sl=64 | p95 | 152,087 us | 169,578 us | 100,941 us | -10.3% | +50.7% |
| bs=100 sw=10 sl=64 | p99 | 152,087 us | 169,578 us | 108,642 us | -10.3% | +40.0% |
| bs=1000 sw=10 sl=64 | throughput | 928 tuples/sec | 933 tuples/sec | 1,119 tuples/sec | -0.5% | -17.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.566 MB/s | 0.569 MB/s | 0.683 MB/s | -0.5% | -17.2% |
| bs=1000 sw=10 sl=64 | p50 | 1,071,516 us | 1,068,792 us | 924,404 us | +0.3% | +15.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,133,416 us | 1,163,516 us | 967,785 us | -2.6% | +17.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,133,416 us | 1,163,516 us | 995,630 us | -2.6% | +13.8% |
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,560.79,200,128000,357,0.218,25643.99,47612.67,47612.67
1,100,10,64,20,2569.46,2000,1280000,778,0.475,124562.36,152087.29,152087.29
2,1000,10,64,20,21549.95,20000,12800000,928,0.566,1071516.38,1133416.14,1133416.14
kz930
added a commit
to Nicoleee1108/texera_workflow_to_py
that referenced
this pull request
Aug 31, 2026
…s switch Turning on Split Plot by Line Group makes the line group required, because code generation asserts it and the run ends on `Line Group cannot be empty`. The field is declared optional and carried no conditional constraint, so the property panel accepted the configuration and the error waited for the run. The schema states the rule instead, in the conditional form the Sklearn text columns already use, so the panel refuses the configuration while it is being written. The assertion stays as the last line of defence. That rule is also one the tooling can read, which is what lets the operator leave the curated tier. Its handler existed for two things auto could not supply: the line group under the flipped switch, which the constraint now fills, and a set of columns that draws a chart at all. The second is three sample columns: the operator refuses line groups whose x sets are disjoint, and left to the first column of each type the verification plotted a string against an index and rendered an error page, which compares equal on both paths and asks nothing. All four variants now draw a real figure. Taken from apache#8285, minus the standalone half, which is ours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
/request-review @aglinxinyuan @carloea2 |
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?
Turning on
Split Plot by Line GroupmakesLine Grouprequired, because code generation asserts it and the run ends onLine Group cannot be empty. The field is declared@JsonProperty(required = false)and carried no conditional constraint, so the property panel accepted the configuration and the error waited for the run. A field that is optional is exactly the one a user leaves behind.The schema states the rule instead, in the conditional form the Sklearn text columns already use (#7643):
required: ["lineGroup"]underfacetColumn, so the panel refuses the configuration while it is being written. Conditional rather than a plain required, so a freshly dropped operator, whose switch is off, is not flagged for a field it has no use for.The assertion stays as the last line of defence, and the two tests that pin it stay with it. Nothing about the generated Python changes.
One thing worth noting beyond the panel: a rule stated in the schema is one the tooling can read. Our translator's configuration generator fills a conditionally-required field from exactly this shape, which is how it fills the Sklearn text columns under Count Vectorizer; the assertion in Scala was invisible to it.
Any related issues, documentation, discussions?
Closes #8283. Same shape as #7643, which stated the Sklearn rule.
How was this PR tested?
FilledAreaPlotOpDescSpecgains an assertion on the generated schema: the line group is not unconditionally required, and the conditional rule requires it under the switch. Removing the annotation turns that test red.WorkflowOperator/testpasses: 2517 tests.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)