test(workflow-operator): extend operator descriptor and predicate coverage

[aglinxinyuan]

What changes were proposed in this PR?

Adds 47 tests across five common/workflow-operator classes (33 -> 80 in these suites). No source changes; all four extended specs are pure insertions (0 deletions), so no existing test was modified.

• AggregationOperation: the SUM/COUNT/MIN/MAX merge lambdas — invoked when several workers report into the global stage and never exercised by AggregateOpSpec — each cross-checked against the equivalent single-pass aggregation; CONCAT's empty-partial branch; the unknown-function error message; and getFinal's detached-copy contract.
• FilterPredicate: the two attribute-type cases nothing reached — ANY (routes through the string path, with numeric coercion and a lexicographic fallback) and BINARY (throws unsupported attribute type) — plus the IS_NULL/IS_NOT_NULL short-circuit ahead of the type switch, value trimming, and NumberFormatException propagation.
• ECDFPlotOpDesc and TimeSeriesOpDesc: operatorInfo and output schema, the optional settings that gate generated Plotly arguments, the generated-code empty-table guards, and — per the repo convention for LogicalOp subtypes — a JSON round-trip through LogicalOp asserting every config field survives, plus a minimal payload deserializing to the documented defaults. Neither spec had a round-trip before.
• OPVersion (new spec): the "N/A" fallback for a path with no commit history, and the name-keyed memoization contract.

Some branches were deliberately not contrived, and are listed with reasons in the review notes — notably FilterPredicate's unreachable default: throw and dead null-guard ternaries (evaluate returns before them), and OPVersion's success path (needs an openable repository; inside a git worktree the .git entry is a file and jgit leaves the handle null, so the spec asserts only what holds in both environments).

Any related issues, documentation, discussions?

Closes #7016.

How was this PR tested?

sbt -java-home <jbr-17> "WorkflowOperator/testOnly *AggregationOperationSpec *FilterPredicateSpec *ECDFPlotOpDescSpec *TimeSeriesOpDescSpec *OPVersionSpec" -> 80 succeeded, 0 failed. Test/scalafmtCheck + Test/scalafix --check clean.

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

Generated-by: Claude Code (Opus 4.8 [1M context])

[github-actions]

Automated Reviewer Suggestions

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

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

[github-actions]

⚠️ Benchmark changes need a look

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

Compared against main ab58d47 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	369	0.225	26,212/34,075/34,075 us	🔴 +14.7% / 🔴 +115.6%
⚪	bs=100 sw=10 sl=64	801	0.489	123,350/140,831/140,831 us	⚪ within ±5% / 🔴 +31.2%
⚪	bs=1000 sw=10 sl=64	911	0.556	1,099,762/1,140,061/1,140,061 us	⚪ within ±5% / 🔴 +12.1%

Baseline details

Latest main ab58d47 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	369 tuples/sec	428 tuples/sec	787.55 tuples/sec	-13.8%	-53.1%
bs=10 sw=10 sl=64	MB/s	0.225 MB/s	0.261 MB/s	0.481 MB/s	-13.8%	-53.2%
bs=10 sw=10 sl=64	p50	26,212 us	22,856 us	12,255 us	+14.7%	+113.9%
bs=10 sw=10 sl=64	p95	34,075 us	32,472 us	15,802 us	+4.9%	+115.6%
bs=10 sw=10 sl=64	p99	34,075 us	32,472 us	19,008 us	+4.9%	+79.3%
bs=100 sw=10 sl=64	throughput	801 tuples/sec	818 tuples/sec	997.81 tuples/sec	-2.1%	-19.7%
bs=100 sw=10 sl=64	MB/s	0.489 MB/s	0.499 MB/s	0.609 MB/s	-2.0%	-19.7%
bs=100 sw=10 sl=64	p50	123,350 us	120,370 us	100,690 us	+2.5%	+22.5%
bs=100 sw=10 sl=64	p95	140,831 us	146,243 us	107,316 us	-3.7%	+31.2%
bs=100 sw=10 sl=64	p99	140,831 us	146,243 us	113,823 us	-3.7%	+23.7%
bs=1000 sw=10 sl=64	throughput	911 tuples/sec	929 tuples/sec	1,030 tuples/sec	-1.9%	-11.6%
bs=1000 sw=10 sl=64	MB/s	0.556 MB/s	0.567 MB/s	0.629 MB/s	-1.9%	-11.6%
bs=1000 sw=10 sl=64	p50	1,099,762 us	1,074,767 us	981,213 us	+2.3%	+12.1%
bs=1000 sw=10 sl=64	p95	1,140,061 us	1,142,017 us	1,027,605 us	-0.2%	+10.9%
bs=1000 sw=10 sl=64	p99	1,140,061 us	1,142,017 us	1,055,466 us	-0.2%	+8.0%

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,542.46,200,128000,369,0.225,26212.49,34075.33,34075.33
1,100,10,64,20,2495.72,2000,1280000,801,0.489,123350.22,140830.90,140830.90
2,1000,10,64,20,21954.20,20000,12800000,911,0.556,1099761.75,1140061.48,1140061.48

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.00%. Comparing base (469e8f0) to head (507e8da).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7018      +/-   ##
============================================
+ Coverage     78.99%   79.00%   +0.01%     
- Complexity     3788     3796       +8     
============================================
  Files          1160     1160              
  Lines         46105    46105              
  Branches       5115     5115              
============================================
+ Hits          36419    36426       +7     
+ Misses         8067     8058       -9     
- Partials       1619     1621       +2     

Flag	Coverage Δ		*Carryforward flag
access-control-service	70.00% <ø> (ø)
agent-service	76.76% <ø> (ø)		Carriedforward from 469e8f0
amber	72.25% <ø> (+0.04%)	⬆️
computing-unit-managing-service	20.49% <ø> (ø)
config-service	66.66% <ø> (ø)
file-service	67.21% <ø> (ø)
frontend	82.99% <ø> (ø)		Carriedforward from 469e8f0
notebook-migration-service	78.94% <ø> (ø)
pyamber	95.38% <ø> (ø)		Carriedforward from 469e8f0
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.

[Copilot]

Pull request overview

This PR expands test coverage in common/workflow-operator by adding new spec cases for aggregation merge behavior, filter predicate edge branches, visualization operator descriptors (including JSON round-trips), and introducing a new spec for OPVersion version-resolution caching behavior.

Changes:

• Adds coverage for previously untested aggregation merge lambdas and additional CONCAT / error / final-stage behaviors.
• Extends FilterPredicate tests to cover ANY, unsupported attribute types, null-check short-circuiting, whitespace trimming, and parse-failure propagation.
• Adds metadata/schema assertions, optional-argument generation checks, generated-code guard checks, and JSON round-trip tests for ECDF and Time Series visualization descriptors; introduces a new OPVersionSpec.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/visualization/ecdfPlot/ECDFPlotOpDescSpec.scala	Adds operatorInfo/output schema checks, argument-shape assertions, generated-code guards, and JSON round-trip/defaults tests.
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/timeSeriesPlot/TimeSeriesOpDescSpec.scala	Adds optional-column gating checks, plot-type/range-slider gating, generated-code shape assertions, and JSON round-trip/defaults tests.
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/OPVersionSpec.scala	Introduces tests for "N/A" fallback and name-keyed memoization behavior.
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/filter/FilterPredicateSpec.scala	Adds tests for ANY, unsupported types, null-check short-circuit, trimming, and numeric parsing failure behavior.
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/aggregate/AggregationOperationSpec.scala	Adds direct coverage of SUM/COUNT/MIN/MAX merge behavior and additional CONCAT/unknown-function/getFinal assertions.

Comments suppressed due to low confidence (3)

common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/OPVersionSpec.scala:69

• This fallback assertion is not guaranteed in a normal git clone: if OPVersion's git handle is non-null, git.log().addPath(missing).call().iterator().next() can throw (e.g., empty history -> NoSuchElementException), which isn't caught and would fail the test. Make the fallback deterministic (or update OPVersion to catch empty history) before asserting "N/A".

  "OPVersion.getVersion" should "fall back to \"N/A\" for a path with no commit history" in {
    val name = uniqueName()
    withCleanCache(name) {
      OPVersion.getVersion(name, uniqueMissingPath()) shouldBe "N/A"
    }

common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/OPVersionSpec.scala:85

• This test depends on getVersion successfully returning (and caching) a value for a missing path. In environments where OPVersion can open the git repo, a missing path may throw (empty history -> NoSuchElementException) instead of caching "N/A". Wrap the resolving call in withGitNull (or adjust OPVersion to handle empty history) so the memoization assertion is stable.

    withCleanCache(name) {
      opMap.containsKey(name) shouldBe false
      val resolved = OPVersion.getVersion(name, uniqueMissingPath())
      opMap.containsKey(name) shouldBe true
      opMap.get(name) shouldBe resolved

common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/metadata/OPVersionSpec.scala:120

• This test assumes unknown-path resolution returns "N/A" on the first call. As implemented, missing paths can throw before caching when OPVersion's git handle is non-null (empty iterator -> NoSuchElementException). Make the first resolution deterministic (e.g., withGitNull) so the repeated-call contract is what you're actually testing.

  it should "resolve unknown paths consistently across repeated calls" in {
    val name = uniqueName()
    withCleanCache(name) {
      val path = uniqueMissingPath()
      val first = OPVersion.getVersion(name, path)
      val second = OPVersion.getVersion(name, path)
      first shouldBe "N/A"
      second shouldBe first

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mengw15]

LGTM