chore(workflow-operator): make FilledAreaPlot disjoint-group tolerance a true 5% - #7149
Merged
aglinxinyuan merged 2 commits intoAug 5, 2026
Merged
Conversation
…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
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 #7149 +/- ##
============================================
+ Coverage 79.27% 81.86% +2.58%
- Complexity 3787 4174 +387
============================================
Files 1160 1162 +2
Lines 46124 46836 +712
Branches 5114 5263 +149
============================================
+ Hits 36563 38340 +1777
+ Misses 7942 6757 -1185
- Partials 1619 1739 +120
*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 | 438 | 0.268 | 22,453/34,387/34,387 us | 🔴 +9.9% / 🔴 +121.3% |
| ⚪ | bs=100 sw=10 sl=64 | 945 | 0.577 | 104,679/128,960/128,960 us | ⚪ within ±5% / 🔴 +18.2% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,096 | 0.669 | 911,873/1,067,841/1,067,841 us | 🔴 +13.0% / 🟢 +9.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 | 438 tuples/sec | 448 tuples/sec | 767.32 tuples/sec | -2.2% | -42.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.268 MB/s | 0.273 MB/s | 0.468 MB/s | -1.8% | -42.8% |
| bs=10 sw=10 sl=64 | p50 | 22,453 us | 20,429 us | 12,772 us | +9.9% | +75.8% |
| bs=10 sw=10 sl=64 | p95 | 34,387 us | 32,680 us | 15,538 us | +5.2% | +121.3% |
| bs=10 sw=10 sl=64 | p99 | 34,387 us | 32,680 us | 18,948 us | +5.2% | +81.5% |
| bs=100 sw=10 sl=64 | throughput | 945 tuples/sec | 982 tuples/sec | 972.51 tuples/sec | -3.8% | -2.8% |
| bs=100 sw=10 sl=64 | MB/s | 0.577 MB/s | 0.599 MB/s | 0.594 MB/s | -3.7% | -2.8% |
| bs=100 sw=10 sl=64 | p50 | 104,679 us | 100,056 us | 103,020 us | +4.6% | +1.6% |
| bs=100 sw=10 sl=64 | p95 | 128,960 us | 129,169 us | 109,070 us | -0.2% | +18.2% |
| bs=100 sw=10 sl=64 | p99 | 128,960 us | 129,169 us | 118,964 us | -0.2% | +8.4% |
| bs=1000 sw=10 sl=64 | throughput | 1,096 tuples/sec | 1,108 tuples/sec | 1,005 tuples/sec | -1.1% | +9.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.669 MB/s | 0.676 MB/s | 0.613 MB/s | -1.0% | +9.1% |
| bs=1000 sw=10 sl=64 | p50 | 911,873 us | 901,128 us | 1,002,400 us | +1.2% | -9.0% |
| bs=1000 sw=10 sl=64 | p95 | 1,067,841 us | 945,365 us | 1,039,228 us | +13.0% | +2.8% |
| bs=1000 sw=10 sl=64 | p99 | 1,067,841 us | 945,365 us | 1,069,081 us | +13.0% | -0.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,456.20,200,128000,438,0.268,22453.27,34387.20,34387.20
1,100,10,64,20,2115.66,2000,1280000,945,0.577,104678.61,128959.87,128959.87
2,1000,10,64,20,18246.42,20000,12800000,1096,0.669,911872.68,1067841.46,1067841.46
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>
Contributor
Contributor
|
The tolerance fix looks right — count > (n * 5) // 100 is exactly equivalent to count / n > 5% at every group count, not just multiples of 100. One note on the tests: the runtime test is the only one of the ten that exercises the tolerance, and it cancels in CI — the amber job has an interpreter but no pandas or plotly. The nine that do run pin the exact emitted text, so they go red on any rewrite, correct or not, and say nothing about behavior. Solution: amber already has the shape for this — AMBER_TEST_FILTER in amber/build.sbt plus the IntegrationTest tag under amber/src/test/integration, with amber-integration as the job that provisions Python |
This was referenced Jul 31, 2026
Contributor
Author
|
/request-review @aglinxinyuan |
aglinxinyuan
changed the title
aglinxinyuan
approved these changes
Aug 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a latent arithmetic bug in the Filled Area Plot operator’s generated Python guard so the “5% disjoint line-group tolerance” is computed correctly for charts with fewer than 100 groups, and adds targeted regression coverage around the generated guard and its runtime behavior.
Changes:
- Fix tolerance computation from
(len(grouped) // 100) * 5to(len(grouped) * 5) // 100so it represents a true 5% floor instead of frequently becoming 0. - Clean up the generated guard logic (hoist per-group
unique()set computation, useis None, simplify toelse, andbreakonce the error condition is reached). - Add new Scala tests covering both the emitted guard text and an optional runtime verification that executes the generated Python against boundary datasets (skipping via
cancelwhen Python deps aren’t available).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/filledAreaPlot/FilledAreaPlotOpDesc.scala | Corrects the 5% tolerance arithmetic and refines the emitted pandas groupby validation loop. |
| common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/filledAreaPlot/FilledAreaPlotOpDescSpec.scala | Adds regression tests for the generated guard (string assertions) plus an optional runtime Python execution test for boundary cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
renovate-bot
pushed a commit
to renovate-bot/apache-_-texera
that referenced
this pull request
Aug 14, 2026
…che#7207) ### What changes were proposed in this PR? The `amber` job runs the `WorkflowOperator` test suite and installs no Python packages. A test that *executes* an operator's generated template, rather than only `py_compile`-ing it, therefore fails its dependency probe and cancels — and a cancellation is neither a pass nor a failure, so the suite still reports "All tests passed" and the gap leaves no trace in the build. This gives `WorkflowOperator` the split `amber` already has: - **Tag** — `IntegrationTest` under `common/workflow-operator/src/test`. amber's cannot be reused: it lives in `amber/src/test/integration`, and amber depends on this module rather than the reverse. - **Filter** — `common/workflow-operator/build.sbt` reads the same `AMBER_TEST_FILTER`. The `amber` job already sets it on the step that invokes `WorkflowOperator/jacoco`, so no workflow change is needed for the exclusion to take effect. - **Job** — `"WorkflowOperator/test"` added to `amber-integration`, which already runs `integration-only` with `amber/requirements.txt` and `amber/operator-requirements.txt` installed (pandas 2.2.3, plotly 5.24.1). - **Shared logic** — the env-var-to-ScalaTest-args mapping moves to `project/TestFilters.scala`, beside the build's other shared helpers (`AddMetaInfLicenseFiles.scala`, `JdkOptions.scala`); each module passes its own env var and tag, the two a caller has to match to the workflow and to the annotation. One behavior changes, and amber inherits it: a value that is neither of the two now fails at project load, where the inline code fell through to running everything. Nothing sets a third value, so what this catches is a typo that would otherwise leave a job running the whole suite while reporting the subset it asked for. - **First user** — `PythonCodeRawInvalidTextSpec` gains a tagged case asserting that pandas and plotly import in the interpreter it already resolves: `py_compile` only parses the emitted code, while running it needs the packages it imports. Being tagged, it also exercises the routing, and it turns a missing install in `amber-integration` into a failure rather than the silent cancellation above. Elsewhere a bare interpreter is a local-setup fact, so it cancels instead. The second half addresses the scaling problem in the same issue: testing operators one at a time does not scale when each one costs a spawn. - **Batching** — the `py_compile` check spawned `python -I -S -B -m py_compile` once per `PythonOperatorDescriptor`, 117 of them serially, where the interpreter boot is the entire cost and the compile is under a millisecond. It now goes through `PythonWorkerPool`: a worker launched once with the same `-I -S` isolation, serving many sources over a line-delimited JSON protocol. What the check accepts is unchanged — `compile(source, path, "exec")` is what `py_compile` does before writing a `.pyc`, and raises the same SyntaxError — and any worker the pool cannot give out or keep falls back to the spawn, so behavior is never worse than before: one that would not start, would not report ready, or died mid-job all leave through the same exception, and the check counts the descriptors that took the spawn into its summary so a run the pool served none of does not read as a green pooled run. `TEXERA_TEST_PYTHON_WORKER=0` selects the old path outright. - **Parallelism** — the descriptors are fanned out across the pool's workers, four at a time; the fan-out's executor is sized to the pool's cap, so nothing runs past it. Together: 1119 ms to 310 ms. That cap is per sub-pool, keyed by script, arguments and environment, and is env-overridable, so it is the fan-out's own sizing that bounds this — not a ScalaTest parameter, which no suite in the module currently parallelizes under anyway. - **Where the pool lives** — this module's test scope, not beside its caller. The tests this PR unblocks execute generated templates against pandas and plotly, where a spawn costs 260-310 ms, mostly the imports, dwarfing the ~4 ms a job computes for. Without a shared seam each such test hand-rolls a driver, a stdout protocol, a timeout and an interpreter probe — the runtime test in apache#7149 already does. Other modules reach it through a `test->test` dependency. ### Any related issues, documentation, discussions? Fixes apache#7186. Found while reviewing apache#7149, whose runtime test is the case cancelling today; tagging it is a one-line follow-up once this is in, and converting its hand-rolled driver to the pool is the natural second one. The pooled-worker design, and the measurements behind those figures, are in apache#6975. ### How was this PR tested? - `amber` is unchanged by the extraction: `show WorkflowExecutionService/Test/testOptions` under all three env values gives arguments byte-identical to its previous inline code — `-l <tag>`, `-n <tag>`, nothing when unset. `WorkflowOperator` was checked the same way and yields its own tag in those positions. - The new case, run as `WorkflowOperator/testOnly *PythonCodeRawInvalidTextSpec`: passes under `integration-only` (1 test selected); is excluded under `skip-integration` while the spec's two existing tests still run; and, pointed at a bare interpreter, fails under `integration-only` and cancels with no filter set. - The pooled check agrees with the spawn it replaces: 117/117 descriptors pass either way, and `TEXERA_TEST_PYTHON_WORKER=0` flips between them. Timed with `-oD`: 1119 ms spawning, 310 ms pooled. - Red-checked, since a green suite only proves the two paths agree on valid code: appending a syntax error to every generated source takes the pooled path to `ok=0/117` and fails the test, so it still detects what it is there to detect. - Each way a worker could escape the pool's borrow/return discipline was reproduced before being fixed, and `PythonWorkerPoolSpec` keeps a case on each: an interrupted caller used to leave a worker neither returned nor discarded, and the case that refills every slot and then asks for that many jobs again spins at the cap until its bound without the fix; a first line that is not the protocol used to raise `JsonParseException` past the caller's fallback, and the fixture now has a mode that writes one; an interpreter that cannot be started arrives as the pool's own exception rather than `ProcessBuilder`'s. - The fallback is never worse than the spawn it replaces, checked end to end: with the pooled path pointed at an interpreter that does not exist, all 117 descriptors fall back, all 117 still pass, and the summary reports 117 fallbacks. - `WorkflowOperator/test` under `skip-integration`: 286 suites, 2163 tests, none failed. - `sbt scalafmtCheckAll` is clean, and `WorkflowOperator`'s `Compile` and `Test` scalafix checks pass. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) --------- Signed-off-by: Kary Zheng <150742834+kz930@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Xinyuan Lin <xinyual3@uci.edu> Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com>
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?
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: anX_valuestypo 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-groupset(...unique())is hoisted into one local (was built twice per group),== Nonebecomesis None, the always-trueelif not ...becomeselse, and the loopbreaks 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_valuestypo 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 toFilledAreaPlotOpDescSpec(TDD: written red first, green after the fix):.unique()per iteration,breakplacement after the error assignment, and that user-provided column names are never emitted verbatim.pytexeraimport 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/scalafmtCheckandWorkflowOperator/Test/scalafmtCheck(no violations),sbt "WorkflowOperator/scalafixAll --check"(clean), and the fullsbt WorkflowOperator/testmodule 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)