You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "kz930 (via GitHub)" <gi...@apache.org> on 2026/08/01 07:22:22 UTC
[PR] fix(visualization): declare the numeric-only chart settings as numbers [texera]
kz930 opened a new pull request, #7233:
URL: https://github.com/apache/texera/pull/7233
### What changes were proposed in this PR?
Eight settings across three chart operators accept only numbers — each 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 `delta`, `threshold`; Bullet Chart `thresholdValue` — discarded silently, leaving a finished-looking chart with the indicator missing
- Bullet Chart `deltaReference`, step `start`/`end` — error page
- Contour Plot `gridSize` — `ValueError`, and the same when left blank
This PR declares them as `Option[Double]` / `Option[Int]`:
- the form rejects a non-number before the run, and all three operators behave the same way
- the generated code no longer parses: values are spliced as numbers, so the conversions, the `try/except ValueError` blocks and Gauge Chart's `json.loads` of its steps are gone, along with an `import json` that became dead
- a step whose bounds are not both filled in is dropped while the list is built, which is what Bullet Chart's "Invalid step values" note existed for
- `@JsonDeserialize(contentAs = ...)` names the boxed class: Scala erases `Option`'s element type, so without it Jackson leaves the raw JSON value inside the Option and the first use throws `ClassCastException`, and the primitive class would read a blank as 0
- Gauge Chart's step bounds are included beyond the seven the issue lists — same declaration, same consumer, and a non-numeric bound was swallowed by a bare `except`, plotting the gauge with no steps at all
Compatibility: 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.
Dendrogram's Color Threshold shares the declaration but fails differently — spliced as a string, scipy raises on any number, and only a blank field or the literal `default` ever plotted — so it is filed separately as #7232.
### How was this PR tested?
- existing specs for the three operators updated to the new types
- per field: deserialization tests for a JSON number, a numeric string, blank, null and absent — plus one that uses the value as a number, the case a round trip cannot catch, since a round trip writes a number back and a missing `contentAs` survives it
- generated-code tests pin the assignments (`delta_ref = 40.0`, `threshold_val = None`, `grid_size = 10` as the form's default) and the dropped half-filled step
- ran the generated Python against a pandas DataFrame for all three operators, configured and left unset: each plots a figure, and the inverted-step case still reports `start ≥ end`
- whole workflow-operator module: 2033 tests passing, `scalafmtCheck` clean
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5[1m])
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(visualization): declare the numeric-only chart settings as numbers [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7233:
URL: https://github.com/apache/texera/pull/7233#issuecomment-5150404104
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/7233?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.57%. Comparing base ([`348fa88`](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3cc8582`](https://app.codecov.io/gh/apache/texera/commit/3cc85820a2b18217f3e2281ead1a39bee24791d0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 13 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #7233 +/- ##
============================================
- Coverage 80.58% 80.57% -0.01%
+ Complexity 3890 3877 -13
============================================
Files 1162 1162
Lines 46264 46240 -24
Branches 5153 5153
============================================
- Hits 37280 37257 -23
Misses 7319 7319
+ Partials 1665 1664 -1
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.42% <ø> (ø)` | | Carriedforward from [348fa88](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.56% <ø> (-0.03%)` | :arrow_down: | Carriedforward from [348fa88](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `20.49% <ø> (ø)` | | Carriedforward from [348fa88](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.97% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.80% <ø> (ø)` | | Carriedforward from [348fa88](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.22% <ø> (ø)` | | Carriedforward from [348fa88](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.89% <ø> (ø)` | | |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `97.36% <ø> (ø)` | | Carriedforward from [348fa88](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/7233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.31% <ø> (ø)` | | Carriedforward from [348fa88](https://app.codecov.io/gh/apache/texera/commit/348fa884442990307b842fd7c0dd6ca005dd320c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/7233?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(visualization): declare the numeric-only chart settings as numbers [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7233:
URL: https://github.com/apache/texera/pull/7233#issuecomment-5150394135
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@aglinxinyuan`, `@eugenegujing`, `@carloea2`
You can notify them by mentioning `@aglinxinyuan`, `@eugenegujing`, `@carloea2` in a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(visualization): declare the numeric-only chart settings as numbers [texera]
Posted by "xuang7 (via GitHub)" <gi...@apache.org>.
xuang7 merged PR #7233:
URL: https://github.com/apache/texera/pull/7233
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(visualization): declare the numeric-only chart settings as numbers [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7233:
URL: https://github.com/apache/texera/pull/7233#issuecomment-5150393510
<!-- backport-auto-label-report -->
### Backport auto-label report
This `fix:` PR was checked against each actively-supported release branch. `release/*` labels drive the post-merge backport, so add or remove one to change where this fix lands.
| Release branch | Analysis |
| --- | --- |
| ✅ `release/v1.2` | Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7. |
_[Auto-label run](https://github.com/apache/texera/actions/runs/30689611833)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(visualization): declare the numeric-only chart settings as numbers [texera]
Posted by "kz930 (via GitHub)" <gi...@apache.org>.
kz930 commented on code in PR #7233:
URL: https://github.com/apache/texera/pull/7233#discussion_r3716593694
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/gaugeChart/GaugeChartOpDesc.scala:
##########
@@ -69,21 +72,26 @@ class GaugeChartOpDesc extends PythonOperatorDescriptor {
OperatorGroupConstants.VISUALIZATION_FINANCIAL_GROUP
)
- private val mapper = new ObjectMapper()
- mapper.registerModule(DefaultScalaModule)
+ /** An unset number reaches the generated code as Python's `None`. */
+ private def numberOrNone(value: Option[Double]): PythonLiteral =
+ value.map(_.toString).getOrElse("None")
- private def serializeSteps(steps: List[GaugeChartSteps]): String = {
- mapper.writeValueAsString(steps)
- }
+ /** The steps whose bounds are both filled in, as a list literal of numbers. */
+ private def stepsLiteral: PythonLiteral =
+ steps
Review Comment:
Normalized — `stepsLiteral` now reads a null list as no steps, the same `Option(steps)` shape Bullet Chart uses. The new Gauge Chart case deserializes `{"operatorType": "GaugeChart", "value": "score", "steps": null}`, asserts the field really arrives null, and asserts the generated code carries `valid_steps = []`; reverting the normalization fails it with the NullPointerException you describe. Bullet Chart gets the same case, since its normalization had no test behind it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(visualization): declare the numeric-only chart settings as numbers [texera]
Posted by "carloea2 (via GitHub)" <gi...@apache.org>.
carloea2 commented on code in PR #7233:
URL: https://github.com/apache/texera/pull/7233#discussion_r3715709318
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/gaugeChart/GaugeChartOpDesc.scala:
##########
@@ -69,21 +72,26 @@ class GaugeChartOpDesc extends PythonOperatorDescriptor {
OperatorGroupConstants.VISUALIZATION_FINANCIAL_GROUP
)
- private val mapper = new ObjectMapper()
- mapper.registerModule(DefaultScalaModule)
+ /** An unset number reaches the generated code as Python's `None`. */
+ private def numberOrNone(value: Option[Double]): PythonLiteral =
+ value.map(_.toString).getOrElse("None")
- private def serializeSteps(steps: List[GaugeChartSteps]): String = {
- mapper.writeValueAsString(steps)
- }
+ /** The steps whose bounds are both filled in, as a list literal of numbers. */
+ private def stepsLiteral: PythonLiteral =
+ steps
Review Comment:
Steps is optional and explicit null input can leave this field null. Calling flatMap then makes code generation fail with a null pointer error. The old path treated null as no steps. Please normalize it here, as the Bullet Chart code already does, and add a null input test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(visualization): declare the numeric-only chart settings as numbers [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7233:
URL: https://github.com/apache/texera/pull/7233#issuecomment-5150409591
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 3 worse · ⚪ 12 noise (<±5%) · 0 without baseline
> Compared against main `e970abb` 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](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/30689620064)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 649 | 0.396 | 15,011/21,145/21,145 us | 🔴 +10.2% / 🔴 +30.5% |
| ⚪ | bs=100 sw=10 sl=64 | 1,352 | 0.825 | 73,074/94,236/94,236 us | ⚪ within ±5% / 🟢 +40.1% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,512 | 0.923 | 653,259/756,462/756,462 us | ⚪ within ±5% / 🟢 +52.4% |
<details><summary>Baseline details</summary>
Latest main `e970abb` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 649 tuples/sec | 704 tuples/sec | 765.25 tuples/sec | -7.8% | -15.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.396 MB/s | 0.43 MB/s | 0.467 MB/s | -7.9% | -15.2% |
| bs=10 sw=10 sl=64 | p50 | 15,011 us | 13,618 us | 12,703 us | +10.2% | +18.2% |
| bs=10 sw=10 sl=64 | p95 | 21,145 us | 21,648 us | 16,199 us | -2.3% | +30.5% |
| bs=10 sw=10 sl=64 | p99 | 21,145 us | 21,648 us | 19,557 us | -2.3% | +8.1% |
| bs=100 sw=10 sl=64 | throughput | 1,352 tuples/sec | 1,340 tuples/sec | 965.22 tuples/sec | +0.9% | +40.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.825 MB/s | 0.818 MB/s | 0.589 MB/s | +0.9% | +40.0% |
| bs=100 sw=10 sl=64 | p50 | 73,074 us | 73,240 us | 103,830 us | -0.2% | -29.6% |
| bs=100 sw=10 sl=64 | p95 | 94,236 us | 94,609 us | 110,778 us | -0.4% | -14.9% |
| bs=100 sw=10 sl=64 | p99 | 94,236 us | 94,609 us | 118,713 us | -0.4% | -20.6% |
| bs=1000 sw=10 sl=64 | throughput | 1,512 tuples/sec | 1,534 tuples/sec | 992.33 tuples/sec | -1.4% | +52.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.923 MB/s | 0.936 MB/s | 0.606 MB/s | -1.4% | +52.4% |
| bs=1000 sw=10 sl=64 | p50 | 653,259 us | 642,822 us | 1,016,959 us | +1.6% | -35.8% |
| bs=1000 sw=10 sl=64 | p95 | 756,462 us | 770,478 us | 1,062,501 us | -1.8% | -28.8% |
| bs=1000 sw=10 sl=64 | p99 | 756,462 us | 770,478 us | 1,082,611 us | -1.8% | -30.1% |
</details>
<details><summary>Raw CSV</summary>
```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,308.40,200,128000,649,0.396,15010.88,21144.58,21144.58
1,100,10,64,20,1479.78,2000,1280000,1352,0.825,73073.77,94235.90,94235.90
2,1000,10,64,20,13225.90,20000,12800000,1512,0.923,653259.10,756462.38,756462.38
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org