This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 71da882192 test(amber): drop the unused pgroonga override from
DatasetResourceSpec (#8403)
71da882192 is described below
commit 71da8821926a1e506c3fc5ba18760c792da7d223
Author: Xinyuan Lin <[email protected]>
AuthorDate: Sat Sep 5 06:04:18 2026 +0000
test(amber): drop the unused pgroonga override from DatasetResourceSpec
(#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, #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)
---------
Signed-off-by: Xinyuan Lin <[email protected]>
Co-authored-by: Copilot Autofix powered by AI
<[email protected]>
---
.../dashboard/DatasetSearchQueryBuilderSpec.scala | 25 +++++++++++++---------
.../dashboard/file/DatasetResourceSpec.scala | 19 +++++++---------
2 files changed, 23 insertions(+), 21 deletions(-)
diff --git
a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala
b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala
index d0e3055966..51028942c0 100644
---
a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala
+++
b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala
@@ -111,16 +111,21 @@ import scala.jdk.CollectionConverters._
* `WorkflowExecutionService` a test->test dependency on `DAO` and `Auth`
only, so workflow-core's
* test tree is not on this module's test classpath.
*
- * The keyword tests RENDER a full-text predicate (they do not fetch one),
which does read the
- * JVM-global `FulltextSearchQueryUtils.usePgroonga` and emits whichever arm
it currently selects:
- * `pgroonga_condition(...)` when this suite runs alone, the
`to_tsvector`/`to_tsquery` arm if
- * `DatasetResourceSpec` or `WorkflowResourceSpec` ran earlier in this JVM
and left the global
- * `false` (both set it and neither restores it; amber has no `Test / fork`).
This suite therefore
- * neither touches nor restores that global, and every keyword assertion here
is deliberately
- * branch-independent: the tokens themselves and the `coalesce(...) || ' ' ||
coalesce(...)`
- * expression are built at `FulltextSearchQueryUtils:49-51`, *before* the `if
(usePgroonga)`.
- * Anything added here must keep that property — an assertion on
`pgroonga_condition` would pass
- * solo and fail in a full-module run.
+ * The keyword tests RENDER a full-text predicate (they do not fetch one),
and rendering reads the
+ * JVM-global `FulltextSearchQueryUtils.usePgroonga`: the
`pgroonga_condition(...)` arm while it
+ * holds `true`, the `to_tsvector`/`to_tsquery` arm while it holds `false`.
That flag is a plain
+ * mutable `var` and amber has no `Test / fork`, so its value here is
whatever the suites sharing
+ * this JVM have left it at — not something this spec controls or should
assume. This suite
+ * therefore neither touches nor restores it, and every keyword assertion
here is deliberately
+ * branch-independent. Two things reach both arms: the `coalesce(...) || ' '
|| coalesce(...)`
+ * expression, built in `FulltextSearchQueryUtils` as `combinedFields` before
the `if (usePgroonga)`
+ * branch and embedded verbatim by either arm, and each INDIVIDUAL keyword
token. Their JOINING does
+ * not — the `true` arm space-joins the full keyword list into one literal
(rendering
+ * `pgroonga_condition('alpha beta', ...)`), while the `false` arm emits one
predicate per keyword
+ * and joins the words *inside* a keyword with ` & ` (rendering
`to_tsquery('english', 'alpha & beta')`).
+ * `to_tsquery('english', 'alpha & beta')`). So assert on individual tokens —
never on a joined
+ * multi-token string, and never on one arm's own output; either would tie
this spec's result to
+ * whichever other suites wrote the flag first.
*
* The `record.into(USER).into(classOf[User]).getEmail` in
`VersionedResourceTables.hydrate` used to
* be executed but unobservable: the dataset schema left
`UnifiedResourceSchema`'s `userEmail` at its
diff --git
a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/DatasetResourceSpec.scala
b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/DatasetResourceSpec.scala
index 70b8252c09..7e58489556 100644
---
a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/DatasetResourceSpec.scala
+++
b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/DatasetResourceSpec.scala
@@ -25,7 +25,6 @@ import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum
import org.apache.texera.dao.jooq.generated.tables.pojos.User
import
org.apache.texera.web.resource.dashboard.DashboardResource.SearchQueryParams
import
org.apache.texera.web.resource.dashboard.user.dataset.DatasetResource.DashboardDataset
-import org.apache.texera.web.resource.dashboard.{FulltextSearchQueryUtils}
import org.apache.texera.web.resource.dashboard.DatasetSearchQueryBuilder
import org.scalatest.flatspec.AnyFlatSpec
import org.apache.texera.dao.jooq.generated.tables.daos.{UserDao, DatasetDao,
DatasetUserAccessDao}
@@ -33,7 +32,6 @@ import
org.apache.texera.dao.jooq.generated.enums.PrivilegeEnum
import org.apache.texera.dao.jooq.generated.tables.pojos.{Dataset,
DatasetUserAccess}
import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach}
import java.time.OffsetDateTime
-import java.util
import
org.apache.texera.web.resource.dashboard.SearchQueryBuilder.DATASET_RESOURCE_TYPE
class DatasetResourceSpec
@@ -96,7 +94,14 @@ class DatasetResourceSpec
override protected def beforeAll(): Unit = {
initializeDBAndReplaceDSLContext()
- FulltextSearchQueryUtils.usePgroonga = false // disable pgroonga
+ // `FulltextSearchQueryUtils.usePgroonga` is deliberately left at its
production default here:
+ // no test in this suite reaches the read, because both `constructQuery`
calls below pass no
+ // keywords and `getFullTextSearchFilter` returns before that read on an
empty keyword list.
+ // A keyword test added here would need the `to_tsvector` arm instead —
`MockTexeraDB` strips
+ // the full-text index block out of the DDL, so the embedded Postgres has
no pgroonga extension
+ // — and would have to set the flag and put it back: amber has no `Test /
fork`, so a value
+ // left behind here follows every suite scheduled after this one in the
same JVM.
+
// add test user directly
val userDao = new UserDao(getDSLContext.configuration())
userDao.insert(ownerUser)
@@ -124,14 +129,6 @@ class DatasetResourceSpec
shutdownDB()
}
- private def getKeywordsArray(keywords: String*): util.ArrayList[String] = {
- val keywordsList = new util.ArrayList[String]()
- for (keyword <- keywords) {
- keywordsList.add(keyword)
- }
- keywordsList
- }
-
private def assertSameDataset(a: Dataset, b: DashboardDataset): Unit = {
assert(a.getName == b.dataset.getName)
}
