test(amber): drop the unused pgroonga override from DatasetResourceSpec by aglinxinyuan · Pull Request #8403 · apache/texera

aglinxinyuan · 2026-09-04T12:03:42Z

What changes were proposed in this PR?

DatasetResourceSpec.beforeAll set the JVM-global FulltextSearchQueryUtils.usePgroonga to false
and never put it back. The write did nothing for this suite and everything to the suites after it.

Why it does nothing here. usePgroonga is read at exactly one place in src/main:
FulltextSearchQueryUtils.scala:52. That read is inside getFullTextSearchFilter, which is called
from exactly two places in src/main — VersionedResourceSearchQueryBuilder.scala:128 and
WorkflowSearchQueryBuilder.scala:120. DatasetResourceSpec reaches neither with a keyword:

step	what it does
the four tests	two use only `UserDao`; two call `DatasetSearchQueryBuilder.constructQuery(uid, SearchQueryParams(resourceType = DATASET_RESOURCE_TYPE), includePublic = true)`
`SearchQueryBuilder.constructQuery` (`final`)	`constructFromClause` + `constructWhereClause` + `mappedResourceSchema.allFields` + `getGroupByFields`; none of the first, third or fourth touches full-text (`constructFromClause` only builds jOOQ joins, `getGroupByFields` is `Seq.empty`)
`constructWhereClause`	reaches `getFullTextSearchFilter(splitKeywords, List(DATASET.NAME, DATASET.DESCRIPTION))`
`splitKeywords`	derived from `params.keywords`, which the tests leave at its `new util.ArrayList[String]()` default (`DashboardResource.scala:72`), so it is empty
`getFullTextSearchFilter`	`fields` is non-empty so the `:39` guard does not fire, but `trimmedKeywords.isEmpty` returns `noCondition()` at `:46` — before the `:52` read

Why it does something to everyone else. amber has no Test / fork and serialises its suites in
one JVM, so false stayed set for every suite scheduled after this one, moving their full-text
rendering onto the to_tsvector/to_tsquery arm.

before:  beforeAll: usePgroonga = false  ->  suite's own 4 tests: never read it
                                         ->  every later suite in the JVM: reads false
after:   flag untouched at its production default true

So the write is deleted rather than captured and restored. A capture-and-restore would still leave
the value wrong during this suite and would depend on when the suite object is constructed and on
where sbt happens to schedule it; deleting the write removes the leak unconditionally.

Also removed, from the same copy-paste block:

private def getKeywordsArray, which has no caller. After the deletion the only getKeywordsArray
in the repository is WorkflowResourceSpec's own private copy (definition at :201, 16 call
sites, all in that file). Being private, this file's copy could only ever have been called from
this file, and was not.
import org.apache.texera.web.resource.dashboard.{FulltextSearchQueryUtils} and import java.util,
which were the only imports those two members needed.

A short comment replaces the write, recording that the flag is left at its production default because
no test here reaches the read, and what a keyword test added here would have to do instead
(MockTexeraDB strips the full-text index block out of the DDL, so the embedded Postgres has no
pgroonga extension — such a test would need the to_tsvector arm, and would have to put the flag
back). Without it the next author copying WorkflowResourceSpec's pattern re-adds the leak.

Why a second file is in the diff. DatasetSearchQueryBuilderSpec's header paragraph carried a
standing instruction — every keyword assertion in that spec must stay branch-independent — and
justified it by asserting this leak as fact: that DatasetResourceSpec and WorkflowResourceSpec
"both set it and neither restores it", and that an assertion on pgroonga_condition "would pass solo
and fail in a full-module run". This PR falsifies the first half for DatasetResourceSpec and the
second half outright. The instruction is still right, so the paragraph now justifies it by the flag
being JVM-global mutable state that any suite in the run may write, and states precisely which parts
of a rendered predicate survive onto both arms: the coalesce(...) || ' ' || coalesce(...) expression
(built at FulltextSearchQueryUtils:49-51, before the if, and embedded verbatim by either arm) and
each individual keyword token — but not their joining, which the two arms render differently (test
5 below). Nothing else in that file changed — no assertion, no other comment, no reformatting.

What this PR does not do. It does not touch WorkflowResourceSpec, which genuinely needs the
false arm (it runs real keyword searches against the embedded Postgres) and still leaks it; that is
a separate change, #8404, which restores the flag there rather than deleting the write. It does not touch src/main — usePgroonga remains a public mutable var. It
does not add or rename a test, and it does not change any assertion anywhere.

Any related issues, documentation, discussions?

Closes #8399

How was this PR tested?

Every number below was read out of amber/target/test-reports/TEST-*.xml, not from sbt's console
summary. Local, Windows, Java 17, module WorkflowExecutionService. 1cbe857007 is the base commit.

1. The suite stays green. testOnly ...file.DatasetResourceSpec: tests="4" failures="0" errors="0".

2. The flag read is unreachable from this suite — measured, not just argued. Temporarily armed
the read site in src/main (if ({ sys.error("READ REACHED"); usePgroonga }), reverted afterwards)
and ran the suite together with a control that does reach the read:

suite	XML	meaning
`DatasetResourceSpec`	`tests="4" failures="0"`	none of its four tests reaches the read
`FulltextSearchQueryUtilsSpec`	`tests="14" failures="3"`	the probe is armed; exactly its three flag-reading tests died

3. The leak is real and the deletion removes it — same invocation, pinned order. sbt gives
separate testOnly invocations fresh classloaders, so the probe puts both suites in one invocation
and pins their order with a Suites subclass that overrides runNestedSuites to construct each
nested suite immediately before running it (sbt's own ScalaTest-framework semantics; Suites(a, b)
would evaluate both constructors up front).

tree	printed before / after `DatasetResourceSpec` ran	observer suite
`1cbe857007`	`true` / `false`	FAILURE, `false was not equal to true`
this branch	`true` / `true`	`tests="5" failures="0"`

Control, so the observer is not vacuously red on base: run alone in its own invocation on
1cbe857007 it is tests="1" failures="0" — the default really is true, and the false came from
DatasetResourceSpec.

4. The downstream arm switch, and that it turns nothing red. Same pinned order
(DatasetResourceSpec then DatasetSearchQueryBuilderSpec, one invocation), the only variable being
this file:

tree	flag observed after the subject	arm that selects (test 5)	downstream result
`1cbe857007`	`false`	`to_tsvector`/`to_tsquery`	`tests="28" failures="0"`
this branch	`true`	`pgroonga_condition`	`tests="28" failures="0"`

That is the point of the change — downstream suites move back onto the production arm — and it costs
nothing, because those assertions are branch-independent.

5. Which parts of a rendered predicate are actually arm-independent. A throwaway spec rendered
getFullTextSearchFilter on both arms and dumped the SQL (DSL.using(POSTGRES).renderInlined, no DB
needed):

keywords	`usePgroonga = true`	`usePgroonga = false`
`["alpha"]`	`... &@~ pgroonga_condition('alpha', ...)`	`to_tsvector('english', ...) @@ to_tsquery('english', 'alpha')`
`["alpha", "beta"]`	`... pgroonga_condition('alpha beta', ...)`	two predicates AND-ed, `to_tsquery('english', 'alpha')` and `... 'beta'`
`["alpha beta"]`	`... pgroonga_condition('alpha beta', ...)`	`... @@ to_tsquery('english', 'alpha & beta')`

The COALESCE(name, '') || ' ' || COALESCE(description, '') expression and each individual token
appear on both arms; the joined string alpha beta appears only on the true arm. Asserted as such
(tests="4" failures="0"), which is what licenses the wording in DatasetSearchQueryBuilderSpec's
paragraph. And the spec's existing assertions really are arm-independent: pinning the flag to each
arm and running it gives tests="24" failures="0" both ways.

Correction, so nobody carries the old sentence forward: an earlier revision of that paragraph (and
of this PR body) said the tokens "render identically on either arm". Review caught it and the table
above is why it was wrong — only individual tokens and the coalesce expression survive both
arms, not a joined multi-token string. The shipped paragraph now says exactly that. Nothing in the
spec asserted on a joined string, so no test changed.

6. No regression. AMBER_TEST_FILTER=skip-integration WorkflowExecutionService/test, run on the
base commit first and then on this branch in the same worktree. Counting <failure>, <error> and
<skipped> children of <testcase> separately, because the distinction matters here:

	report files	tests	`<failure>`	`<error>`	`<skipped>`
`1cbe857007`	195	2303	84	1	1
this branch	195	2303	84	1	1

The two non-pass identity lists are byte-identical (diff is empty). The 85 failures/errors are all
pre-existing local-environment failures — ResultExportServiceSpec 17, DataProcessingSpec 16,
ExecutionStatsServiceSpec 12, ExecutionResultServiceSpec 11, SyncExecutionResourceSpec 8,
InputPortMaterializationReaderThreadSpec 8, PveResourceSpec 6, ReconfigurationSpec 2,
PauseSpec 2, and one each in WorkflowExecutionServiceSpec,
GitVersionControlLocalFileStorageSpec and DefaultCostEstimatorSpec (that last is a
construction-time abort) — none of them in the dashboard search path this PR touches. The single
<skipped> is NetworkOutputBufferSpec's pendingUntilFixed test, which is not a failure at all.

Correction: an earlier revision of this body reported "86 non-pass identities" with a tail of "8
singletons/pairs". Review could not reproduce 86 and measured 85. Both measurements were right about
the XML — 86 was 85 failures/errors plus that one pendingUntilFixed <skipped> row, silently
lumped in with the failures. The table above separates them. It was a bad count, not a flake.

Worth recording, because it is why the identity diff alone is not sufficient evidence: sbt's suite
order is not stable across invocations of the same command. In the base run
DatasetSearchQueryBuilderSpec ran 1st of 195 and DatasetResourceSpec 54th — so the downstream
spec happened to run before the leak and saw true anyway; in the branch run they were 141st and
102nd. Tests 3-5 are the ordered evidence; the module runs only show that nothing else moved.

7. Lint. WorkflowExecutionService/scalafmtCheck, WorkflowExecutionService/Test/scalafmtCheck
and WorkflowExecutionService/scalafixAll --check all exit 0, the last with its cache cleared so it
really re-scanned both changed files (Running scalafix on 274 Scala sources / on 209 Scala sources; the one warning it prints is pre-existing, in OutputManagerSpec). scalafix is the gate
that matters here: each deletion orphans an import.

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

Generated-by: Claude Code (Opus 5)

`DatasetResourceSpec.beforeAll` set the JVM-global `FulltextSearchQueryUtils.usePgroonga` to `false` and never put it back. The write had no effect on this suite: both of its `constructQuery` calls pass no keywords, so `getFullTextSearchFilter` returns at its empty-keyword guard before it ever reads the flag. Its only effect was to leave `false` behind for whichever suites sbt schedules after it in the same JVM (amber has no `Test / fork`), silently moving their full-text rendering onto the `to_tsvector`/`to_tsquery` arm. Delete the write rather than capture-and-restore it: with nothing here reading the flag, leaving it at its production default removes the leak outright, with no dependence on when the suite object is constructed or on how sbt orders the run. The `getKeywordsArray` helper that came along with the write is deleted too -- it has no caller (`WorkflowResourceSpec` has its own private copy) -- and the two imports that only those two members needed go with it. `DatasetSearchQueryBuilderSpec`'s header paragraph asserted this leak as fact while explaining why its keyword assertions must stay branch-independent. The instruction stands, so the paragraph now justifies it by the flag being JVM-global mutable state rather than by any particular suite's behaviour, and states precisely what survives onto both arms -- the concatenated `coalesce` expression and each individual keyword token, but not their joining, which the two arms render differently.

github-actions · 2026-09-04T12:03:58Z

Automated Reviewer Suggestions

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

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

Copilot

🟢 Approval recommended

The changes are a low-risk cleanup confined to tests/comments and remove a confirmed JVM-global state leak without altering production code paths.

Pull request overview

This PR removes an unnecessary JVM-global test override (FulltextSearchQueryUtils.usePgroonga = false) from DatasetResourceSpec that was not exercised by that suite but could leak into subsequently executed suites (amber tests run in a shared JVM). It also updates nearby test documentation to reflect that the flag is shared mutable state and to clarify what parts of the rendered full-text predicate are stable across both rendering arms.

Changes:

Remove the usePgroonga = false write from DatasetResourceSpec.beforeAll and replace it with an explanatory comment about why the suite leaves the flag at its production default.
Delete an unused local helper (getKeywordsArray) and the imports that only existed to support it.
Update DatasetSearchQueryBuilderSpec’s header documentation to justify arm-independent assertions without relying on a specific suite’s leak behavior.

File summaries

File	Description
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/DatasetResourceSpec.scala	Removes a leaking JVM-global flag override and deletes an unused helper/imports; adds an explanatory comment to prevent reintroducing the leak.
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala	Updates scaladoc to describe `usePgroonga` as shared mutable state and clarifies which predicate substrings are arm-independent.

Review details

Files reviewed: 2/2 changed files
Comments generated: 0
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-09-04T12:10:43Z

⚠️ Benchmark changes need a look

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

Compared against main 1cbe857 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	365	0.223	26,418/35,395/35,395 us	🔴 +14.2% / 🔴 +116.8%
⚪	bs=100 sw=10 sl=64	804	0.491	120,288/151,947/151,947 us	⚪ within ±5% / 🔴 +37.8%
🔴	bs=1000 sw=10 sl=64	918	0.56	1,080,343/1,251,006/1,251,006 us	🔴 +11.5% / 🔴 +18.8%

Baseline details

Latest main 1cbe857 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	365 tuples/sec	422 tuples/sec	749.39 tuples/sec	-13.5%	-51.3%
bs=10 sw=10 sl=64	MB/s	0.223 MB/s	0.258 MB/s	0.457 MB/s	-13.6%	-51.2%
bs=10 sw=10 sl=64	p50	26,418 us	23,129 us	13,045 us	+14.2%	+102.5%
bs=10 sw=10 sl=64	p95	35,395 us	33,888 us	16,328 us	+4.4%	+116.8%
bs=10 sw=10 sl=64	p99	35,395 us	33,888 us	19,518 us	+4.4%	+81.3%
bs=100 sw=10 sl=64	throughput	804 tuples/sec	843 tuples/sec	968.07 tuples/sec	-4.6%	-16.9%
bs=100 sw=10 sl=64	MB/s	0.491 MB/s	0.515 MB/s	0.591 MB/s	-4.7%	-16.9%
bs=100 sw=10 sl=64	p50	120,288 us	117,586 us	103,330 us	+2.3%	+16.4%
bs=100 sw=10 sl=64	p95	151,947 us	145,032 us	110,305 us	+4.8%	+37.8%
bs=100 sw=10 sl=64	p99	151,947 us	145,032 us	121,264 us	+4.8%	+25.3%
bs=1000 sw=10 sl=64	throughput	918 tuples/sec	936 tuples/sec	998 tuples/sec	-1.9%	-8.0%
bs=1000 sw=10 sl=64	MB/s	0.56 MB/s	0.571 MB/s	0.609 MB/s	-1.9%	-8.1%
bs=1000 sw=10 sl=64	p50	1,080,343 us	1,068,701 us	1,009,354 us	+1.1%	+7.0%
bs=1000 sw=10 sl=64	p95	1,251,006 us	1,122,293 us	1,052,748 us	+11.5%	+18.8%
bs=1000 sw=10 sl=64	p99	1,251,006 us	1,122,293 us	1,081,982 us	+11.5%	+15.6%

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,548.12,200,128000,365,0.223,26417.82,35395.47,35395.47
1,100,10,64,20,2487.29,2000,1280000,804,0.491,120288.41,151946.71,151946.71
2,1000,10,64,20,21780.74,20000,12800000,918,0.560,1080342.54,1251006.00,1251006.00

codecov-commenter · 2026-09-04T12:16:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.01%. Comparing base (1cbe857) to head (d5e08b3).
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8403      +/-   ##
============================================
- Coverage     94.03%   94.01%   -0.02%     
+ Complexity     4821     4818       -3     
============================================
  Files          1204     1204              
  Lines         48991    48992       +1     
  Branches       5956     5957       +1     
============================================
- Hits          46067    46062       -5     
- Misses         1458     1461       +3     
- Partials       1466     1469       +3

Flag	Coverage Δ		*Carryforward flag
access-control-service	`81.00% <ø> (ø)`		Carriedforward from a9996d3
agent-service	`99.32% <ø> (ø)`		Carriedforward from a9996d3
amber	`89.88% <ø> (-0.04%)`	⬇️
computing-unit-managing-service	`73.67% <ø> (ø)`		Carriedforward from a9996d3
config-service	`87.12% <ø> (ø)`		Carriedforward from a9996d3
file-service	`87.91% <ø> (ø)`		Carriedforward from a9996d3
frontend	`96.79% <ø> (ø)`		Carriedforward from a9996d3
notebook-migration-service	`83.57% <ø> (ø)`		Carriedforward from a9996d3
pyamber	`98.47% <ø> (ø)`		Carriedforward from a9996d3
workflow-compiling-service	`77.19% <ø> (ø)`		Carriedforward from a9996d3

*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

🟢 Approval recommended

The change is a low-risk test-hygiene cleanup that removes cross-suite global-state leakage without altering test assertions or production behavior.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

mengw15

LGTM

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>

apache#8404) ### What changes were proposed in this PR? `FulltextSearchQueryUtils.usePgroonga` (`amber/src/main/scala/org/apache/texera/web/resource/dashboard/FulltextSearchQueryUtils.scala:32`) is a JVM-global `var` whose production default is `true`. It is read at exactly one site, `FulltextSearchQueryUtils.scala:52`, to choose between the pgroonga arm and the `to_tsvector`/`to_tsquery` fallback. `WorkflowResourceSpec.beforeAll` forces it `false` and never puts it back. amber declares no `Test / fork` — the two `Test / fork := true` settings a grep finds in `build.sbt` belong to `ComputingUnitManagingService` (`build.sbt:191`) and `FileService` (`build.sbt:227`), while the amber project (`WorkflowExecutionService`, `build.sbt:252`) declares neither `fork` nor `parallelExecution` — and suites are serialized by `Tags.limit(Tags.Test, 1)` (`amber/build.sbt:48`). One `WorkflowExecutionService/test` run is therefore a single JVM sharing a single flag: 193 completed suites in the run measured here. ``` one WorkflowExecutionService/test JVM -- unforked, suites serialized ... -> WorkflowResourceSpec -> ... beforeAll: usePgroonga = false afterAll : (before) nothing every later suite (after) usePgroonga = <captured> renders the fallback arm, not production's ``` This suite genuinely needs the `false` arm, so the write is restored rather than deleted: the embedded Postgres it runs against has no pgroonga extension, and with the flag left `true` 11 of its 78 tests fail with `ERROR: function pgroonga_condition(unknown, fuzzy_max_distance_ratio => numeric) does not exist`. The 11 are named under *How was this PR tested?*. The capture is taken **in `beforeAll`, immediately before the write**, and written back as the **first** statement of `afterAll`, ahead of `closeConnectionPool()`. That ordering is hygiene, not a fix for a live hazard: `MockTexeraDB.closeConnectionPool` (`common/dao/src/test/scala/org/apache/texera/dao/MockTexeraDB.scala:183-192`) already swallows any `Exception` itself (`catch { case e: Exception => e.printStackTrace() } finally { ... }`), so only an `Error` could escape it today. Putting the restore first means the ordering does not depend on that staying true. The alternative — capturing into a `val` at construction time — restores whatever the flag held when sbt happened to instantiate the class, which is a property of sbt's scheduling rather than of what the suite clobbered. Both forms were run against the same scenario: an earlier suite leaves the flag `false`, then this suite runs, then a third suite reads it. | capture site | value restored | scenario outcome | | --- | --- | --- | | construction time (`private val`) | `true` — the value at instantiation | 79/80, `true was not equal to false` | | write time (`private var`, this PR) | `false` — the value it clobbered | 80/80 | Honest qualification, since it cuts against this change: that scenario is built with a nested `org.scalatest.Suites`, which evaluates its nested suites as **constructor arguments** and therefore instantiates all of them before any of their tests run. Measured separately, sbt/ScalaTest does *not* do that for discovered suites — it constructs each one immediately before running it (order log: `ctor:Bb, beforeAll:Bb, test:Bb, afterAll:Bb, ctor:Aa, ...`). Nothing touches the flag between this suite's construction and its `beforeAll`, and there are zero `extends Suites` classes in `amber/src/test/scala`, so **the two forms restore the same value in a real `test` run today and are behaviourally indistinguishable there**. The write-time form is preferred only because its correctness does not depend on that measurement continuing to hold: a future `Test / fork`, a `OneInstancePerTest` mixin, or any nesting suite would each change the answer. The in-code comment carries that same qualification, so the file does not overstate the case either. The field is also initialised from the live value at its declaration. That is not decoration — `afterAll` was measured to run even when `beforeAll` throws, so the restore can execute on a path where the capture line never did, and the initialiser keeps it from writing back an invented default. **The restore is unpinned: nothing in the repo would go red if it were deleted again.** Pinning the arm `false` for an entire module run — a conservative superset of the leak's effect — leaves the failing-test identities byte-identical to baseline, so no suite in `WorkflowExecutionService` currently reads the flag in an arm-sensitive way. This PR removes latent cross-suite state leakage; it does not fix a currently-failing test, and no fragile ordering-dependent guard suite was added to manufacture a pin. **A comment elsewhere that this change falsifies.** `DatasetSearchQueryBuilderSpec`'s header comment (`:114-123`) explains why that suite's keyword assertions are deliberately arm-independent, and its parenthetical names both writers of the global: "`DatasetResourceSpec` or `WorkflowResourceSpec` ran earlier in this JVM and left the global `false` (both set it and neither restores it...)". After this commit that sentence is false — this suite does restore it. This PR deliberately does **not** edit that comment. Any narrowing written here ("only `DatasetResourceSpec` leaks") would itself become false the moment the sibling PR lands, so the correction belongs in exactly one place: the sibling (apache#8403) rewrites that paragraph into a form that names no suite at all and is therefore true whichever of the two merges first. If this PR merges first, the comment is stale until the sibling lands — and the guardrail it states, "anything added here must keep that property", stays valid in every one of the four states, so nothing that reads it is misled about what to do. Writers of the global today: | writer | restores? | | --- | --- | | `FulltextSearchQueryUtilsSpec:64,71,87` | yes — `after { ... }`, per test | | `WorkflowResourceSpec:179` | this PR | | `DatasetResourceSpec:99` | no — deleted outright by apache#8403, since that suite never reaches the flag read | Once both land, this spec is the only writer of the global outside `FulltextSearchQueryUtilsSpec`. With only **one** of the two applied the flag's end state differs — with only this PR, `DatasetResourceSpec` still writes `false` without restoring; with only the sibling, this suite still leaks `false` — and whichever of the two sbt schedules last decides it (sbt's order was measured to be neither alphabetical nor command-line order). *Inference, not a measurement:* since nothing in the module was found to be arm-sensitive, the failing-test identity set should not move in either partial state. The sibling-only tree was never built or run, so that half is reasoning from the arm-sensitivity measurement rather than an observation. **Scope.** One test file, +17 lines: the capture, the restore, and the comment explaining why the capture sits where it does. No `src/main` file is touched and no other test file is touched. No test is added, renamed or deleted; the suite's own 78 tests are unchanged and so are their outcomes. ### Any related issues, documentation, discussions? Closes apache#8400 ### How was this PR tested? Everything below is read out of `amber/target/test-reports/TEST-*.xml`, not the sbt console summary. All probe suites were deleted before committing; the branch is one commit. **1. The suite really does need the `false` write.** With `beforeAll` changed to leave the flag `true`, `WorkflowResourceSpec` alone reports 78 tests and **11 failures**, every one an `org.jooq.exception.DataAccessException` caused by `ERROR: function pgroonga_condition(unknown, fuzzy_max_distance_ratio => numeric) does not exist`. The suite passes keywords through a `getKeywordsArray` helper at 16 call sites (`:331-751`), one of them with the reserved-character string `"+-@()<>~*\""`, so it reaches the flag read for real instead of taking the empty-keywords early return at `FulltextSearchQueryUtils.scala:45-47`. ``` /search API should be able to search for resources by keyword /search API should be able to search for workflows in different columns in Workflow table /search API should be able to search text phrases /search API should be able to search with arbitrary number of keywords in different combinations /search API should filter results by different resourceType /search API should handle multiple keywords correctly /search API should handle reserved characters in the keywords /search API should not be able to search workflows from different user accounts /search API should not return resources that belong to a different user /search API should return multiple matching resources from a single resource type /search API should return resources that match any of all provided keywords ``` So the right fix here is a restore, not the deletion the sibling PR makes. **2. Red before / green after,** with an explicitly-ordered throwaway probe — `Suites(new WorkflowResourceSpec, new PgroongaProbeTailSpec)`, where the tail asserts the global still holds production's default. The nesting pins the ordering because a plain `testOnly` of two classes does not order them. Both suites in the **same** invocation, since separate invocations get fresh classloaders and reset the static: | tree | result | non-passing identity in the XML | | --- | --- | --- | | baseline `1cbe857007` | 78 passed, **1 failed** | `the JVM-global usePgroonga, after the preceding suite finished should still hold production's default` | | with this PR | **79 passed, 0 failed** | none | | control: probe alone, baseline | 1 passed | none | The control matters: the tail assertion is not unconditionally red, so its red above is caused by the leak. **3. Write-time vs construction-time capture** — the table in the first section. Same probe shape, with an added first nested suite that leaves the flag `false`. Both arms were measured on the committed content: the write-time arm as committed, the construction-time arm by changing only `var` to `val` and dropping the capture line from `beforeAll`. The construction-time form restores `true` and clobbers it (XML: `true was not equal to false` on `usePgroonga after WorkflowResourceSpec should still be the false the earlier suite left`); the write-time form restores `false` and the run is 80/80. As stated above, this reflects the eager instantiation that `Suites` nesting creates rather than sbt's own (measured lazy) behaviour, so it is a design argument, not a live bug. **4. Instantiation and lifecycle semantics,** measured with an append-only order log from two suites plus one whose `beforeAll` throws: ``` ctor:Bb flag=true <- Bb fully constructed, run and torn down beforeAll:Bb flag=true test:Bb flag=true afterAll:Bb ctor:Aa flag=true <- only now is Aa constructed beforeAll:Aa flag=true test:Aa sets flag=false afterAll:Aa throwspec:beforeAll entered, about to throw throwspec:afterAll RAN <- afterAll runs even when beforeAll throws ``` Four findings: construction is lazy, per suite, immediately before that suite runs; suites do not interleave; `afterAll` still runs when `beforeAll` throws (sbt reported `Suites: completed 2, aborted 1` and the throwing suite's test never logged a line, so the restore can execute on a path where the capture never did); and `Bb` ran before `Aa` although `Aa` was listed first on the `testOnly` command line and sorts first alphabetically. **5. Arm sensitivity, i.e. why the restore is unpinned.** Production default mutated `true` -> `false` in `src/main` for one whole module run, then reverted: 195 report files and **86 non-passing identities, byte-identical to the baseline list** (`diff` empty). That is a superset of the leak's effect — every one of the 193 suites ran on the fallback arm, not just the ones scheduled after this spec — so no suite outcome in this module depends on the arm. The mutation was reverted and verified: `git diff --name-only 1cbe857 -- '*/src/main/*'` is empty and line 32 reads `var usePgroonga: Boolean = true` again. **6. Regression,** widest scope that runs locally — `AMBER_TEST_FILTER=skip-integration sbt WorkflowExecutionService/test`, baseline (both touched files at their `1cbe857007` content) measured first. *Counting rule:* a non-passing identity is a `<testcase>` element in `amber/target/test-reports/TEST-*.xml` carrying a `<failure>`, `<error>` or `<skipped>` child, printed as `KIND \t suite \t test name` and sorted; the same rule is applied to both runs. | run | report files | sbt summary | non-passing identities | | --- | --- | --- | --- | | baseline `1cbe857007` | 195 | 2297 tests: 2214 succeeded, 83 failed, 1 canceled, 1 pending; 193 suites completed, 1 aborted | 86 | | with this PR | 195 | identical, line for line | 86 | `diff` of the two sorted identity lists is **empty**. The 86 break down as 84 `<failure>` + 1 `<error>` (the aborted suite's `SuiteSelector` pseudo-testcase) + 1 `<skipped>`, and the run's own reporter tally agrees: `Total 2301, Failed 84, Errors 1, Passed 2216, Canceled 1, Pending 1`. Excluding `<skipped>` the same runs read as 85 rows — the absolute number is method-dependent, which is why the rule is stated; the load-bearing fact is that the two lists are byte-identical, not the count. All 86 are pre-existing on this box, in 13 suites, none of them touched by this PR: | suite | rows | why it is red here | | --- | --- | --- | | ResultExportServiceSpec | 17 | `org.apache.iceberg.exceptions.RESTException` — REST catalog GET to `localhost:8181`, no Docker on this box | | DataProcessingSpec | 16 | same catalog GET, wrapped in `java.lang.Throwable` | | ExecutionStatsServiceSpec | 12 | same | | ExecutionResultServiceSpec | 11 | same | | SyncExecutionResourceSpec | 8 | same | | InputPortMaterializationReaderThreadSpec | 8 | same (an engine worker-manager suite, not a dashboard one) | | PveResourceSpec | 6 | python virtual environment: `Python executable not found for PVE` | | ReconfigurationSpec, PauseSpec | 2 + 2 | same catalog GET | | WorkflowExecutionServiceSpec | 1 | same | | DefaultCostEstimatorSpec | 1 | the aborted suite: `RESTException` from the same catalog GET at construction | | GitVersionControlLocalFileStorageSpec | 1 | local file-tree assertion, `testFileTreeRetrieval` | | NetworkOutputBufferSpec | 1 | the `<skipped>` row — a `pendingUntilFixed` test, not a failure | `WorkflowResourceSpec`, the only file this PR touches, is green in both wide runs at `tests="78" errors="0" failures="0"`; so is `DatasetSearchQueryBuilderSpec` (`tests="24"`), the spec whose header comment describes this leak. The change alters what the suite leaves behind, not what it asserts. **7. Lint** (CI gates): `WorkflowExecutionService/scalafmtCheck`, `WorkflowExecutionService/Test/scalafmtCheck` and `WorkflowExecutionService/scalafixAll --check` all report `[success]` on the committed tree. **Corrections made after review.** Four claims in an earlier draft of this description were wrong or unstated, and are corrected above rather than quietly dropped: (a) the restore-before-teardown ordering was justified as protecting against a teardown throw, but `closeConnectionPool` catches `Exception` itself, so the justification is hygiene only; (b) the non-passing identity count is counting-method dependent (the same runs read as 85 rows if `<skipped>` is excluded), so the rule is now stated; (c) the pre-existing local failures were described as Iceberg/Docker dashboard suites, which under-describes them — see the table above; and (d) this PR falsifies a sentence in `DatasetSearchQueryBuilderSpec`'s header comment, which the earlier draft cited as supporting evidence without disclosing that it goes stale; An earlier revision of this branch corrected that comment here; the edit has been reverted, because the narrowing it wrote would go false as soon as the sibling landed. The correction now lives only in the sibling PR, in an order-neutral form, and this PR discloses the staleness instead. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5)

Copilot AI lite review requested due to automatic review settings September 4, 2026 12:03

github-actions Bot assigned aglinxinyuan Sep 4, 2026

github-actions Bot added the engine label Sep 4, 2026

Copilot started reviewing on behalf of aglinxinyuan September 4, 2026 12:04 View session

aglinxinyuan mentioned this pull request Sep 4, 2026

test(amber): restore the pgroonga global WorkflowResourceSpec clobbers #8404

Merged

Copilot AI reviewed Sep 4, 2026

View reviewed changes

aglinxinyuan requested a review from mengw15 September 4, 2026 12:12

mengw15 requested a lite review from Copilot September 5, 2026 05:34

Copilot started reviewing on behalf of mengw15 September 5, 2026 05:35 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

Comment thread .../src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala Outdated

mengw15 approved these changes Sep 5, 2026

View reviewed changes

Refactor comments for clarity in DatasetSearchQueryBuilderSpec

d5e08b3

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>

aglinxinyuan enabled auto-merge September 5, 2026 05:52

aglinxinyuan added this pull request to the merge queue Sep 5, 2026

Merged via the queue into apache:main with commit 71da882 Sep 5, 2026
22 checks passed

aglinxinyuan deleted the test/dataset-resource-spec-drop-unused-pgroonga-override branch September 5, 2026 06:17

Navigation Menu

Uh oh!

test(amber): drop the unused pgroonga override from DatasetResourceSpec - #8403

test(amber): drop the unused pgroonga override from DatasetResourceSpec#8403
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:test/dataset-resource-spec-drop-unused-pgroonga-override

aglinxinyuan commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

mengw15 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

aglinxinyuan commented Sep 4, 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 4, 2026

Automated Reviewer Suggestions

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

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

Uh oh!

⚠️ Benchmark changes need a look

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

mengw15 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aglinxinyuan commented Sep 4, 2026 •

edited

Loading

github-actions Bot commented Sep 4, 2026 •

edited

Loading

codecov-commenter commented Sep 4, 2026 •

edited

Loading