aglinxinyuan opened a new pull request, #8403:
URL: https://github.com/apache/texera/pull/8403
### 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. 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
