This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8403-39e17fd1d1d4eb608300be19a31fc99a8cdc2d46 in repository https://gitbox.apache.org/repos/asf/texera.git
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) }
