fix(visualization): declare the numeric-only chart settings as numbers - #7233
Open
kz930 wants to merge 1 commit into
Open
fix(visualization): declare the numeric-only chart settings as numbers#7233kz930 wants to merge 1 commit into
kz930 wants to merge 1 commit into
Conversation
Contributor
Backport auto-label reportThis
|
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 #7233 +/- ##
============================================
+ Coverage 80.58% 82.09% +1.51%
- Complexity 3890 4081 +191
============================================
Files 1162 1162
Lines 46264 46315 +51
Branches 5153 5160 +7
============================================
+ Hits 37280 38024 +744
+ Misses 7319 6584 -735
- Partials 1665 1707 +42
*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 | 377 | 0.23 | 27,814/34,055/34,055 us | 🔴 +20.2% / 🔴 +123.1% |
| ⚪ | bs=100 sw=10 sl=64 | 807 | 0.492 | 124,161/137,316/137,316 us | ⚪ within ±5% / 🔴 +27.9% |
| 🔴 | bs=1000 sw=10 sl=64 | 926 | 0.565 | 1,075,112/1,240,656/1,240,656 us | 🔴 +10.1% / 🔴 +20.3% |
Baseline details
Latest main 436b37e from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 377 tuples/sec | 418 tuples/sec | 782.98 tuples/sec | -9.8% | -51.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.23 MB/s | 0.255 MB/s | 0.478 MB/s | -9.8% | -51.9% |
| bs=10 sw=10 sl=64 | p50 | 27,814 us | 23,138 us | 12,467 us | +20.2% | +123.1% |
| bs=10 sw=10 sl=64 | p95 | 34,055 us | 37,123 us | 15,695 us | -8.3% | +117.0% |
| bs=10 sw=10 sl=64 | p99 | 34,055 us | 37,123 us | 18,950 us | -8.3% | +79.7% |
| bs=100 sw=10 sl=64 | throughput | 807 tuples/sec | 835 tuples/sec | 996.92 tuples/sec | -3.4% | -19.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.492 MB/s | 0.51 MB/s | 0.608 MB/s | -3.5% | -19.1% |
| bs=100 sw=10 sl=64 | p50 | 124,161 us | 122,059 us | 100,899 us | +1.7% | +23.1% |
| bs=100 sw=10 sl=64 | p95 | 137,316 us | 141,213 us | 107,385 us | -2.8% | +27.9% |
| bs=100 sw=10 sl=64 | p99 | 137,316 us | 141,213 us | 115,226 us | -2.8% | +19.2% |
| bs=1000 sw=10 sl=64 | throughput | 926 tuples/sec | 928 tuples/sec | 1,024 tuples/sec | -0.2% | -9.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.565 MB/s | 0.566 MB/s | 0.625 MB/s | -0.2% | -9.6% |
| bs=1000 sw=10 sl=64 | p50 | 1,075,112 us | 1,073,131 us | 989,300 us | +0.2% | +8.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,240,656 us | 1,126,807 us | 1,031,689 us | +10.1% | +20.3% |
| bs=1000 sw=10 sl=64 | p99 | 1,240,656 us | 1,126,807 us | 1,058,394 us | +10.1% | +17.2% |
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,530.09,200,128000,377,0.230,27814.47,34054.79,34054.79
1,100,10,64,20,2479.07,2000,1280000,807,0.492,124160.99,137315.72,137315.72
2,1000,10,64,20,21595.35,20000,12800000,926,0.565,1075111.81,1240656.01,1240656.01Eight settings across three chart operators accept only numbers -- every one of them is used solely as `float(...)` or `int(...)` -- but were declared as plain strings, so the form let any text through and each operator then mishandled it differently: Gauge Chart's Delta and Threshold and Bullet Chart's Threshold Value discarded the value silently, leaving a finished-looking chart with the indicator missing; Bullet Chart's Delta Reference and its step bounds rendered an error page; Contour Plot's Grid Size raised a ValueError, as did leaving it blank. Declaring them numeric moves the check into the schema, so the form rejects a non-number before the run and every operator behaves the same way. The parsing disappears from the generated code with it: the values are spliced as numbers, so `float(...)`, `int(...)`, the `try/except ValueError` blocks and Gauge Chart's `json.loads` of its steps are all gone, along with the `import json` that became dead. Half-filled step bounds are dropped while building the list rather than checked in Python, which is what the "Invalid step values" note existed for. `@JsonDeserialize(contentAs = ...)` is required on each field and names the boxed class on purpose. Scala erases `Option[Double]`'s element type, so without the annotation Jackson leaves the raw JSON value inside the Option and the first use throws ClassCastException; with the primitive class instead, a blank value coerces to 0, which for a threshold is the difference between "at zero" and "not drawn". Each field gets a test for the shapes a stored workflow can hold -- number, numeric string, blank, null, absent -- including one that reads the value back as a number, since a round-trip test cannot catch the missing annotation. Workflows keep loading: a numeric string saved before this change still reads as a number, and a blank one as unset. A workflow that stored a value that is not a number at all now fails to load rather than silently dropping it -- those charts were already rendering without the setting. Gauge Chart's step bounds are fixed alongside the seven settings the issue lists: they are the same declaration with the same consumer, and a non-numeric bound was swallowed by a bare `except`, plotting the gauge with no steps at all. Fixes apache#7213 Generated-by: Claude Code (claude-opus-5[1m])
kz930
force-pushed
the
fix/7213-numeric-viz-settings
branch
from
August 2, 2026 04:00
3cc8582 to
b309bfe
Compare
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
3 participants
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?
Eight settings across three chart operators accept only numbers — each is used solely as
float(...)orint(...)— but were declared as plain strings, so the form let any text through and each operator then mishandled it differently:delta,threshold; Bullet ChartthresholdValue— discarded silently, leaving a finished-looking chart with the indicator missingdeltaReference, stepstart/end— error pagegridSize—ValueError, and the same when left blankThis PR declares them as
Option[Double]/Option[Int]:try/except ValueErrorblocks and Gauge Chart'sjson.loadsof its steps are gone, along with animport jsonthat became dead@JsonDeserialize(contentAs = ...)names the boxed class: Scala erasesOption's element type, so without it Jackson leaves the raw JSON value inside the Option and the first use throwsClassCastException, and the primitive class would read a blank as 0except, plotting the gauge with no steps at allCompatibility: a numeric string saved before this change still loads as a number and a blank one as unset; a workflow that stored a value that is not a number now fails to load rather than silently dropping it — those charts were already rendering without the setting.
Any related issues, documentation, discussions?
Fixes #7213.
How was this PR tested?
contentAssurvives itdelta_ref = 40.0,threshold_val = None,grid_size = 10as the form's default) and the dropped half-filled stepstart ≥ endscalafmtCheckcleanWas this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5[1m])