Skip to content

fix(workflow-operator): make FilledAreaPlot disjoint-group tolerance a true 5% - #7149

Open
eugenegujing wants to merge 2 commits into
apache:mainfrom
eugenegujing:fix/filled-area-plot-tolerance
Open

fix(workflow-operator): make FilledAreaPlot disjoint-group tolerance a true 5%#7149
eugenegujing wants to merge 2 commits into
apache:mainfrom
eugenegujing:fix/filled-area-plot-tolerance

Conversation

@eugenegujing

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Background (#7146). The Filled Area Plot operator draws stacked area lines, one per Line Group, which only makes sense when the groups share an x axis. FilledAreaPlotOpDesc.performTableCheck() therefore emits a Python guard that suppresses the chart when too many groups have x-value sets disjoint from the others; the comment above it documents the rule as "more than 5 percents of the groups have disjoint sets of x attributes". When the guard fires, the run still completes successfully — the operator just yields fallback HTML instead of the chart, with no warning anywhere.

The defect. The guard computes the threshold as (len(grouped) // 100) * 5. The integer division floors first, so the tolerance is 0 for any chart with fewer than 100 line groups, and a single disjoint group suppresses the whole chart — e.g. a 40-group chart with 1 disjoint group (2.5%, well under the documented 5%) was suppressed. The two expressions only agree when the group count is an exact multiple of 100. This has been latent since the operator was introduced in #2086: an X_values typo in the same block kept the accumulated x-value set from ever growing, so the guard fired constantly for an unrelated reason; #6894 fixed that typo, which made the tolerance arithmetic the deciding factor for the first time.

The change. Swap the operand order to (len(grouped) * 5) // 100, plus four behavior-neutral cleanups to the same emitted block: the per-group set(...unique()) is hoisted into one local (was built twice per group), == None becomes is None, the always-true elif not ... becomes else, and the loop breaks once the error is set.

Below 20 groups both expressions yield 0, which is correct (1/19 = 5.3% exceeds 5%), so the fix only changes behavior at 20+ line groups.

Any related issues, documentation, discussions?

Fixes #7146. Adjacent to #6728 / #6894, which fixed an X_values typo in the same block but did not touch the tolerance arithmetic.

How was this PR tested?

performTableCheck() previously had no test coverage. This PR adds 10 tests to FilledAreaPlotOpDescSpec (TDD: written red first, green after the fix):

  • 9 assertions on the generated guard, including the tolerance expression, a single .unique() per iteration, break placement after the error assignment, and that user-provided column names are never emitted verbatim.
  • 1 runtime test that executes the generated Python (real pandas groupby + plotly rendering; only the pytexera import seam is stubbed) across 9 boundary datasets: 19 groups/1 disjoint still suppressed (5.3% > 5%), 20/1 and 40/2 at exactly 5.0% render, 40/1 renders (the reported bug), 40/3 suppressed, plus all-disjoint, single-group, empty-table, and missing-column cases. It cancels (not fails) when no python with pandas+plotly is available.

Full spec: 20/20 passing. Red-check verified: reverting only the tolerance line makes the runtime test fail on the 40/1 and exactly-5% cases.

Checks from [CONTRIBUTING.md] all pass locally: sbt WorkflowOperator/scalafmtCheck and WorkflowOperator/Test/scalafmtCheck (no violations), sbt "WorkflowOperator/scalafixAll --check" (clean), and the full sbt WorkflowOperator/test module suite — 1967 succeeded, 0 failed, 2 pending.

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

Co-authored by: Claude Code (Claude Fable 5)

…a true 5%

The generated guard computed `tolerance = (len(grouped) // 100) * 5`, which floors to 0 for any chart with fewer than 100 line groups, so a single disjoint line group suppressed the whole chart. Swap the operand order: `(len(grouped) * 5) // 100`. Behavior only changes at 20+ groups; below that both forms correctly give 0.

Ride-along cleanups in the same emitted block: hoist the per-group `set(...unique())` into one local, use `is None`, turn the always-true `elif not ...` into `else`, and `break` once the verdict is final.

Adds the first tests covering performTableCheck(): 9 assertions on the generated guard plus a runtime boundary test that executes the generated Python (cancels gracefully when pandas/plotly are unavailable).

Fixes apache#7146
@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Jul 31, 2026
@github-actions
github-actions Bot requested a review from xuang7 July 31, 2026 00:01
@github-actions

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

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

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

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

@codecov-commenter

codecov-commenter commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.43%. Comparing base (02ae12b) to head (e9ac8ba).
⚠️ Report is 42 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7149      +/-   ##
============================================
+ Coverage     79.27%   79.43%   +0.16%     
- Complexity     3787     3832      +45     
============================================
  Files          1160     1161       +1     
  Lines         46124    46146      +22     
  Branches       5114     5134      +20     
============================================
+ Hits          36563    36655      +92     
+ Misses         7942     7858      -84     
- Partials       1619     1633      +14     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 76.76% <ø> (ø) Carriedforward from 0ad1a6c
amber 73.05% <100.00%> (+0.44%) ⬆️
computing-unit-managing-service 20.49% <ø> (ø)
config-service 66.66% <ø> (ø)
file-service 66.80% <ø> (-0.41%) ⬇️
frontend 83.08% <ø> (ø) Carriedforward from 0ad1a6c
notebook-migration-service 78.89% <ø> (-0.06%) ⬇️
pyamber 96.34% <ø> (ø) Carriedforward from 0ad1a6c
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.

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 51b6af3 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 378 0.231 23,281/38,015/38,015 us 🔴 -16.6% / 🔴 +140.8%
bs=100 sw=10 sl=64 941 0.574 103,842/132,330/132,330 us ⚪ within ±5% / 🔴 +20.0%
bs=1000 sw=10 sl=64 1,097 0.67 911,142/965,735/965,735 us ⚪ within ±5% / 🟢 -10.1%
Baseline details

Latest main 51b6af3 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 378 tuples/sec 453 tuples/sec 771.62 tuples/sec -16.6% -51.0%
bs=10 sw=10 sl=64 MB/s 0.231 MB/s 0.276 MB/s 0.471 MB/s -16.3% -51.0%
bs=10 sw=10 sl=64 p50 23,281 us 21,428 us 12,626 us +8.6% +84.4%
bs=10 sw=10 sl=64 p95 38,015 us 35,023 us 15,786 us +8.5% +140.8%
bs=10 sw=10 sl=64 p99 38,015 us 35,023 us 19,344 us +8.5% +96.5%
bs=100 sw=10 sl=64 throughput 941 tuples/sec 966 tuples/sec 971.56 tuples/sec -2.6% -3.1%
bs=100 sw=10 sl=64 MB/s 0.574 MB/s 0.589 MB/s 0.593 MB/s -2.5% -3.2%
bs=100 sw=10 sl=64 p50 103,842 us 99,624 us 103,463 us +4.2% +0.4%
bs=100 sw=10 sl=64 p95 132,330 us 133,726 us 110,296 us -1.0% +20.0%
bs=100 sw=10 sl=64 p99 132,330 us 133,726 us 118,690 us -1.0% +11.5%
bs=1000 sw=10 sl=64 throughput 1,097 tuples/sec 1,111 tuples/sec 1,001 tuples/sec -1.3% +9.6%
bs=1000 sw=10 sl=64 MB/s 0.67 MB/s 0.678 MB/s 0.611 MB/s -1.2% +9.6%
bs=1000 sw=10 sl=64 p50 911,142 us 904,619 us 1,008,988 us +0.7% -9.7%
bs=1000 sw=10 sl=64 p95 965,735 us 945,807 us 1,055,260 us +2.1% -8.5%
bs=1000 sw=10 sl=64 p99 965,735 us 945,807 us 1,074,689 us +2.1% -10.1%
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,529.31,200,128000,378,0.231,23280.98,38014.81,38014.81
1,100,10,64,20,2126.50,2000,1280000,941,0.574,103842.37,132329.78,132329.78
2,1000,10,64,20,18226.96,20000,12800000,1097,0.670,911141.98,965734.82,965734.82

Empty commit to rerun the amber-integration job: ReconfigurationIntegrationSpec failed once on macOS only (known flaky pause/resume race, unrelated to this change; the same tests passed on ubuntu).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Yicong-Huang

Copy link
Copy Markdown
Contributor

cc @kz930 @carloea2, PTAL!

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

Labels

common fix release/v1.2 back porting to release/v1.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Filled Area Plot: the documented 5% disjoint-group tolerance evaluates to 0 for fewer than 100 line groups

3 participants