Skip to content

feat(workflow-operator): stop forcing a color column on every Bubble Chart - #7396

Open
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:fix/bubblechart-optional-color-column
Open

feat(workflow-operator): stop forcing a color column on every Bubble Chart#7396
kz930 wants to merge 1 commit into
apache:mainfrom
kz930:fix/bubblechart-optional-color-column

Conversation

@kz930

@kz930 kz930 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Bubble Chart's Color-Column was declared required = true with @NotNull, but the generated Python reads it only inside the Enable Color branch. The effect was that a freshly dropped Bubble Chart stayed invalid until the user picked a color column, even when they wanted plain bubbles — and the column they picked was then never used.

This PR makes the field optional and puts it behind the toggle via toggleHidden, so it disappears from the panel when Enable Color is off. That matches Ternary Plot, which has the same toggle-plus-column pair and already declares its color field optional.

The color decision also moves out of the generated Python and into Scala. The old template emitted an if '...' == 'true': comparison over a Scala Boolean; it is now a colorArg computed at build time, guarded on both the toggle and the column being non-empty. That second half matters: with the required flag gone, an empty column would otherwise reach px.scatter(color=''), which plotly rejects — the same failure fixed for Bar Chart in #6792.

Behavior for existing workflows is unchanged. enableColor keeps its meaning, so no saved chart changes appearance.

The operator reference page is updated to match the new requirement and description.

Any related issues, documentation, discussions?

Closes #7395

How was this PR tested?

Existing BubbleChartOpDescSpec passes unchanged, including the assertion that pins the no-color output line. Three cases were added to it, covering the toggle-and-column matrix: enabled with a column chosen (color is emitted), enabled with no column (color is omitted rather than emitted empty), and disabled with a column chosen (the column is not emitted).

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

@github-actions github-actions Bot added fix docs Changes related to documentations common labels Aug 7, 2026
@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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.

@github-actions
github-actions Bot requested a review from xuang7 August 7, 2026 23:54
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

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

  • Committers with relevant context: @parshimers
    You can request their reviews formally with /request-review @parshimers.

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

…Chart

Color-Column was declared required, but the generated Python reads it
only inside the Enable Color branch. A user who wanted plain bubbles
still had to pick a column, and the column they picked did nothing.

Make the field optional and hide it behind the toggle, the way Ternary
Plot already does. The color decision moves from a Python-level string
comparison to Scala, so an unset column falls back to uniform bubbles
instead of reaching px.scatter as an empty string, which plotly rejects.

Closes apache#7395

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kz930
kz930 force-pushed the fix/bubblechart-optional-color-column branch from c475524 to 6e35895 Compare August 7, 2026 23:57
@kz930 kz930 changed the title fix(workflow-operator): stop forcing a color column on every Bubble Chart feat(workflow-operator): stop forcing a color column on every Bubble Chart Aug 7, 2026
@github-actions
github-actions Bot requested a review from parshimers August 7, 2026 23:58
@codecov-commenter

codecov-commenter commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.26%. Comparing base (e7cd5b1) to head (6e35895).
⚠️ Report is 60 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7396      +/-   ##
============================================
+ Coverage     82.73%   83.26%   +0.53%     
- Complexity     4089     4151      +62     
============================================
  Files          1162     1164       +2     
  Lines         46302    46419     +117     
  Branches       5162     5172      +10     
============================================
+ Hits          38308    38652     +344     
+ Misses         6302     6041     -261     
- Partials       1692     1726      +34     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 83.65% <ø> (ø) Carriedforward from e7cd5b1
amber 80.83% <100.00%> (+1.30%) ⬆️
computing-unit-managing-service 50.72% <ø> (+7.11%) ⬆️
config-service 65.97% <ø> (ø)
file-service 69.05% <ø> (+1.31%) ⬆️
frontend 83.55% <ø> (ø) Carriedforward from e7cd5b1
notebook-migration-service 78.89% <ø> (ø)
pyamber 97.36% <ø> (ø) Carriedforward from e7cd5b1
workflow-compiling-service 26.31% <ø> (ø)

*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.

@xuang7
xuang7 removed request for parshimers and xuang7 August 8, 2026 00:00
@xuang7 xuang7 removed fix release/v1.2 back porting to release/v1.2 labels Aug 8, 2026
@kz930

kz930 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 May you take a look at this?

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 5e41efa 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 398 0.243 23,777/32,124/32,124 us 🔴 +7.7% / 🔴 +98.8%
🔴 bs=100 sw=10 sl=64 897 0.547 109,591/158,102/158,102 us 🔴 +19.6% / 🔴 +42.6%
🔴 bs=1000 sw=10 sl=64 1,091 0.666 907,890/1,073,753/1,073,753 us 🔴 +10.9% / 🟢 -11.1%
Baseline details

Latest main 5e41efa from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 398 tuples/sec 429 tuples/sec 755.31 tuples/sec -7.2% -47.3%
bs=10 sw=10 sl=64 MB/s 0.243 MB/s 0.262 MB/s 0.461 MB/s -7.3% -47.3%
bs=10 sw=10 sl=64 p50 23,777 us 22,068 us 12,952 us +7.7% +83.6%
bs=10 sw=10 sl=64 p95 32,124 us 34,585 us 16,161 us -7.1% +98.8%
bs=10 sw=10 sl=64 p99 32,124 us 34,585 us 19,292 us -7.1% +66.5%
bs=100 sw=10 sl=64 throughput 897 tuples/sec 984 tuples/sec 957.58 tuples/sec -8.8% -6.3%
bs=100 sw=10 sl=64 MB/s 0.547 MB/s 0.6 MB/s 0.584 MB/s -8.8% -6.4%
bs=100 sw=10 sl=64 p50 109,591 us 99,326 us 104,473 us +10.3% +4.9%
bs=100 sw=10 sl=64 p95 158,102 us 132,174 us 110,867 us +19.6% +42.6%
bs=100 sw=10 sl=64 p99 158,102 us 132,174 us 120,336 us +19.6% +31.4%
bs=1000 sw=10 sl=64 throughput 1,091 tuples/sec 1,084 tuples/sec 986.93 tuples/sec +0.6% +10.5%
bs=1000 sw=10 sl=64 MB/s 0.666 MB/s 0.661 MB/s 0.602 MB/s +0.8% +10.6%
bs=1000 sw=10 sl=64 p50 907,890 us 923,594 us 1,021,008 us -1.7% -11.1%
bs=1000 sw=10 sl=64 p95 1,073,753 us 968,353 us 1,059,187 us +10.9% +1.4%
bs=1000 sw=10 sl=64 p99 1,073,753 us 968,353 us 1,093,309 us +10.9% -1.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,501.99,200,128000,398,0.243,23777.48,32123.69,32123.69
1,100,10,64,20,2230.52,2000,1280000,897,0.547,109590.66,158101.79,158101.79
2,1000,10,64,20,18329.33,20000,12800000,1091,0.666,907890.43,1073753.39,1073753.39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common docs Changes related to documentations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bubble Chart's Color-Column is required even though it is only used when Enable Color is on

4 participants