test(verify): compare the charts the two paths drew by kz930 · Pull Request #8509 · apache/texera

kz930 · 2026-09-11T04:01:04Z

What changes were proposed in this PR?

A visualization operator produces no table, so the table comparison has nothing to read. It writes a plotly figure and an HTML page, and this compares those.

A chart's meaning is in the numbers behind it, so the comparison reads those rather than the rendered picture, which would fail over a layout detail that carries none. The figure is compared trace by trace.

The page needs two things normalized away first, neither of which is markup the operator chose: a Styler id, which pandas regenerates on every run, and the line ending, which on Windows is CRLF for a file Python opened in text mode while the same markup carried through JSONL keeps the LF the engine wrote.

#8359 compares two tables, which is a different question, but the figure comparison here runs the --plotly mode of the compare.py that #8359 adds, so this merges after it.

Any related issues, documentation, discussions?

Part of #8325, 7 of 27; that issue lists the set in order.

Closes #8508, the task this change is the whole of.

How was this PR tested?

VisualizationHtmlComparatorSpec pins both normalizations: two pages differing only in a Styler id compare equal, and so do two differing only in line endings, while a page differing in the markup itself does not.

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

Generated-by: Claude Code (Claude Opus 5)

🤖 Generated with Claude Code

A chart's meaning is in the numbers behind it, so the comparison reads those rather than the rendered picture, which would fail over a layout detail that carries none. Plotly writes both a JSON figure and an HTML page, and each needs its own reading: the figure is compared trace by trace, and the page has a Styler id regenerated per run and a line ending chosen by whichever platform wrote the file, neither of which is markup the operator chose. Split out of apache#8359 on review. That change compares two tables, which is a different question and shares no code with this one. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

github-actions · 2026-09-11T04:01:23Z

Automated Reviewer Suggestions

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

Contributors with relevant context: @Xiao-zhen-Liu
You can notify them by mentioning @Xiao-zhen-Liu in a comment.

codecov-commenter · 2026-09-11T04:05:18Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.81%. Comparing base (7d05326) to head (b4623de).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8509   +/-   ##
=========================================
  Coverage     91.80%   91.81%           
- Complexity     5023     5026    +3     
=========================================
  Files          1279     1279           
  Lines         56557    56557           
  Branches       7419     7419           
=========================================
+ Hits          51922    51926    +4     
+ Misses         2769     2768    -1     
+ Partials       1866     1863    -3

Flag	Coverage Δ		*Carryforward flag
access-control-service	`77.44% <ø> (ø)`
agent-service	`99.53% <ø> (ø)`		Carriedforward from 7d05326
amber	`85.79% <ø> (+0.02%)`	⬆️
computing-unit-managing-service	`64.05% <ø> (ø)`
config-service	`87.73% <ø> (ø)`
file-service	`82.31% <ø> (ø)`
frontend	`96.51% <ø> (ø)`		Carriedforward from 7d05326
notebook-migration-service	`83.77% <ø> (ø)`
pyamber	`98.50% <ø> (ø)`		Carriedforward from 7d05326
workflow-compiling-service	`78.30% <ø> (ø)`

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

The uuid pattern matched anywhere in the page, so two tables whose cells read T_dead and T_beef compared equal and a real difference in exported data went unreported. Replace the uuid only where a Styler writes it, in the id attribute and the selector that targets it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

A cell is not escaped, so a value reading `#T_dead` looked like a CSS selector and two tables disagreeing about it still compared equal. The uuid is now replaced inside the `<style>` element and inside tags, which is everywhere a Styler writes one and nowhere the table speaks. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

carloea2 · 2026-09-14T06:24:54Z

There is also a build dependency missing in this PR: VisualizationJsonComparator imports PythonWorkerPool from WorkflowOperator test sources, but WorkflowCompilingService does not depend on those test classes. The current CI job fails to compile that import. Please include the test dependency here so this PR builds independently. CI: https://github.com/apache/texera/actions/runs/34804943402/job/103854922027

VisualizationJsonComparator reuses PythonWorkerPool, which lives in workflow-operator's test sources, so the module needs it in test scope. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

kz930 · 2026-09-14T07:36:17Z

Good catch, thanks. Added the test-scope dependency in dfa14dc, so the PR builds on its own now. The same line is in #8357 with identical wording, so the two merge without a conflict.

github-actions · 2026-09-14T07:56:59Z

⚠️ Benchmark changes need a look

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

Compared against main 7d05326 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	554	0.338	16,938/23,228/23,228 us	🟢 -10.6% / 🔴 +63.7%
🔴	bs=100 sw=10 sl=64	1,239	0.756	79,916/104,919/104,919 us	🔴 +5.4% / 🟢 -11.1%
🔴	bs=1000 sw=10 sl=64	1,447	0.883	691,455/776,883/776,883 us	🔴 +5.5% / 🟢 +21.7%

Baseline details

Latest main 7d05326 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	554 tuples/sec	575 tuples/sec	880.16 tuples/sec	-3.7%	-37.1%
bs=10 sw=10 sl=64	MB/s	0.338 MB/s	0.351 MB/s	0.537 MB/s	-3.7%	-37.1%
bs=10 sw=10 sl=64	p50	16,938 us	16,317 us	11,544 us	+3.8%	+46.7%
bs=10 sw=10 sl=64	p95	23,228 us	25,995 us	14,186 us	-10.6%	+63.7%
bs=10 sw=10 sl=64	p99	23,228 us	25,995 us	17,122 us	-10.6%	+35.7%
bs=100 sw=10 sl=64	throughput	1,239 tuples/sec	1,274 tuples/sec	1,151 tuples/sec	-2.7%	+7.6%
bs=100 sw=10 sl=64	MB/s	0.756 MB/s	0.778 MB/s	0.703 MB/s	-2.8%	+7.6%
bs=100 sw=10 sl=64	p50	79,916 us	77,811 us	89,900 us	+2.7%	-11.1%
bs=100 sw=10 sl=64	p95	104,919 us	99,543 us	96,346 us	+5.4%	+8.9%
bs=100 sw=10 sl=64	p99	104,919 us	99,543 us	110,344 us	+5.4%	-4.9%
bs=1000 sw=10 sl=64	throughput	1,447 tuples/sec	1,494 tuples/sec	1,189 tuples/sec	-3.1%	+21.7%
bs=1000 sw=10 sl=64	MB/s	0.883 MB/s	0.912 MB/s	0.726 MB/s	-3.2%	+21.7%
bs=1000 sw=10 sl=64	p50	691,455 us	667,258 us	873,675 us	+3.6%	-20.9%
bs=1000 sw=10 sl=64	p95	776,883 us	736,104 us	918,440 us	+5.5%	-15.4%
bs=1000 sw=10 sl=64	p99	776,883 us	736,104 us	953,075 us	+5.5%	-18.5%

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,360.72,200,128000,554,0.338,16938.20,23228.04,23228.04
1,100,10,64,20,1614.51,2000,1280000,1239,0.756,79916.10,104918.72,104918.72
2,1000,10,64,20,13823.52,20000,12800000,1447,0.883,691454.89,776883.20,776883.20

carloea2

The HTML ID handling now preserves cell text. The comparator tests look good.

carloea2

The HTML comparison reads only the first nonempty runtime row. If native returns HTML A and B but standalone returns only A, this still passes. Visualization operators can emit one HTML result per input row. Please compare every row and add a multirow case.

The comparison read the first row of the runtime path's output and stopped, so a run that drew a chart the exported script never drew still passed. The two sides are now compared as sequences, and the failure says how many each path drew and what the unmatched one held. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

kz930 · 2026-09-18T20:24:52Z

Fixed in fe7ac26: the two sides are compared as sequences now, so a run that drew a chart the exported script never drew fails instead of passing on its first row, and the message says how many each path drew. The multirow case is in, with the second chart named in the failure.

carloea2

The HTML comparison now checks every runtime row and rejects a missing extra result. The earlier finding is fixed. Looks good.

The comparison had no spec of its own, and the case worth holding is the multi-figure one: an operator that draws a figure per input row agrees on the first long after it has stopped agreeing, so a comparison that read one figure and stopped called that a match. A figure the other path drew differently, and one it never drew at all, are each their own test. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

The comparison says "the run drew 2 and the exported script drew 1" where the assertion was still reading the wording this spec was written against. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

carloea2

The added figure comparison tests cover the revised chart handling. No new blocker found.

…own spec Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

carloea2

Reviewed the current head. All eight focused comparator tests and scalafmtCheckAll pass with compare.py and the IntegrationTest annotation supplied from #8359. The earlier cell-text normalization finding remains fixed. The standalone branch still needs those dependency files, so this is not a merge-ready approval. Leaving it as a draft.

carloea2

All 8 visualization comparator tests and scalafmtCheckAll pass with compare.py supplied from #8359. The earlier cell-text normalization finding stays fixed, and extra chart rows are rejected. Approved for the declared dependency order, not for merging before #8359.

…-two-charts # Conflicts: # build.sbt

…ify-compare-two-charts

carloea2

This conflicts with current main in build.sbt. Please keep both the DAO test-fixture dependency and the WorkflowOperator test-class dependency when rebasing.

…-two-charts # Conflicts: # build.sbt

kz930 · 2026-11-11T00:21:07Z

Merged main and kept both the DAO and WorkflowOperator test dependencies.

carloea2

The rebase is clean now. Both JSON comparator tests fail because compare.py is not on main yet. All 8 comparator tests pass with the script from #8359, so the declared merge order still applies.

github-actions Bot assigned kz930 Sep 11, 2026

github-actions Bot added feature platform Non-amber Scala service paths labels Sep 11, 2026

kz930 mentioned this pull request Sep 11, 2026

test(verify): compare what the two paths produced #8359

Draft

kz930 mentioned this pull request Sep 11, 2026

A workflow cannot be taken out of Texera as a script that runs on its own #8325

Open

21 of 31 tasks

kz930 marked this pull request as draft September 11, 2026 05:55

carloea2 reviewed Sep 11, 2026

View reviewed changes

Comment thread ...e/src/test/scala/org/apache/texera/amber/translator/verify/VisualizationHtmlComparator.scala Resolved

kz930 and others added 2 commits September 12, 2026 00:51

build: let the compiling service read the worker pool

dfa14dc

VisualizationJsonComparator reuses PythonWorkerPool, which lives in workflow-operator's test sources, so the module needs it in test scope. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

github-actions Bot added dependencies Pull requests that update a dependency file common labels Sep 14, 2026

carloea2 approved these changes Sep 18, 2026

View reviewed changes

carloea2 suggested changes Sep 18, 2026

View reviewed changes

carloea2 approved these changes Sep 18, 2026

View reviewed changes

kz930 and others added 2 commits September 18, 2026 15:52

test(verify): read the count out of the message compare.py now writes

e47995c

The comparison says "the run drew 2 and the exported script drew 1" where the assertion was still reading the wording this spec was written against. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

kz930 mentioned this pull request Sep 18, 2026

test(verify): run every operator both ways and report what it did #8362

Draft

carloea2 approved these changes Sep 19, 2026

View reviewed changes

test(verify): leave the page-to-page chart cases to the comparator's …

739b316

…own spec Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

carloea2 reviewed Oct 13, 2026

View reviewed changes

carloea2 approved these changes Oct 23, 2026

View reviewed changes

kz930 added 2 commits October 31, 2026 23:10

Merge remote-tracking branch 'upstream/main' into feat/verify-compare…

400d27a

…-two-charts # Conflicts: # build.sbt

Merge remote-tracking branch 'refs/remotes/apache-main' into feat/ver…

f51b6f5

…ify-compare-two-charts

carloea2 reviewed Nov 10, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat/verify-compare…

b4623de

…-two-charts # Conflicts: # build.sbt

carloea2 reviewed Nov 11, 2026

View reviewed changes

Conversation

kz930 commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this PR?

Any related issues, documentation, discussions?

How was this PR tested?

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

Uh oh!

github-actions Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Reviewer Suggestions

Uh oh!

codecov-commenter commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

carloea2 commented Sep 14, 2026

Uh oh!

kz930 commented Sep 14, 2026

Uh oh!

github-actions Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Benchmark changes need a look

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

kz930 commented Sep 18, 2026

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

kz930 commented Nov 11, 2026

Uh oh!

carloea2 left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kz930 commented Sep 11, 2026 •

edited

Loading

github-actions Bot commented Sep 11, 2026 •

edited

Loading

codecov-commenter commented Sep 11, 2026 •

edited

Loading

github-actions Bot commented Sep 14, 2026 •

edited

Loading