The GitHub Actions job "Required Checks" on texera.git/main has failed. Run started by GitHub user github-merge-queue[bot] (triggered by github-merge-queue[bot]).
Head commit for run: 896426c40325f8664e1e1015cf759c6aadc66799 / Xinyuan Lin <[email protected]> refactor: remove four unreachable code paths (#8339) ### What changes were proposed in this PR? Removes four unreachable code paths. **4 files, +8/−43.** No test file is touched, and behaviour is preserved on every input the code can receive. | Site | Removed | |---|---| | `user-dataset-version-creator.component.ts` | `get formControlNames()` — 3 lines | | `expression_evaluator.py` | `ExpressionEvaluator._contextualize_expression`, plus the `re` import and `Pattern` type it alone used — 13 lines | | `Attribute.java` | two null guards in `equals` — 7 lines | | `user-dataset-file-renderer.component.ts` | the empty-row `.filter(...)` — net −14 | Two of these are worth more than their line count. **The second `Attribute.equals` guard was latently wrong, not just unreachable.** It returned `that.attributeType == null` and ignored the attribute names entirely, so two differently-named attributes both holding a null type compared **equal** — which would break the `Schema` lookups and `Set` semantics built on this class. Verified by forcing the field: pre-removal, two attributes with different names and null types did compare equal. **The file-renderer filter misled its reader.** Its comment says "filter out all empty row"; `for (const cell in row)` enumerates *keys*, so `cell != ""` is true on the first iteration and the predicate returns true. Rows of entirely empty strings were never filtered. ### Liveness, established per site rather than inferred A name grep is not sufficient in this repo, so each site was checked against Angular templates, Jackson, reflection, protobuf, jOOQ, service registries, trait mixins and the test tree. - **`formControlNames`** — occurs exactly once repo-wide, its own declaration; the fragment `ControlNames` occurs zero times, so no template can contain the binding text. The component's selector appears in **no** template at all — it exists only as `NzModal` `nzContent`, which rules out the parent-template route. Decisive check: a full **AOT `ng build`** compiles every template in the app and passes. `tsc --noEmit` and `ng test` would not have caught a template binding; an AOT build does. - **`_contextualize_expression`** — zero call sites; the only member any other module touches on that class is `evaluate`. No `__getattr__`, no registry, no getattr dispatch. - **`Attribute.equals` guards** — one constructor, `@JsonCreator`, `checkNotNull` on both params before either `putfield` (confirmed with `javap`). Fields are `private final`, no setter, no subclass. All six null-or-missing JSON shapes throw through a real `ObjectMapper` (`ValueInstantiationException` / "Missing required creator property"), and with no default constructor Jackson must route through that creator. `AmberKryoInitializer` registers no custom instantiator for this class. Corroborating: `hashCode()` already dereferences `attributeName` with no guard, so a null-field instance would NPE in any `HashMap`/`HashSet` — the very uses these guards purported to protect. - **The row filter** — the producers were checked, not just the predicate. Real `papaparse` over 17 inputs never yields a zero-key row (a blank line becomes `[""]`), and `read-excel-file`'s `getData.js` assigns `null` for every column index so rows are always dense, with splice-based trimming preserving density; driving the real `getData` over 8 synthetic sheets dropped 0 rows. ### Two claims of mine that the review corrected Worth recording, because both make the change look *less* trivially safe than I first described. **The filter was not an unconditional no-op.** I said it "cannot remove anything". It does drop a row with zero own enumerable keys — `[]`, `{}`, or a sparse `new Array(3)`. The sparse case is the interesting one, since it escapes the padding loop (`row.length >= header.length` pushes nothing) and reaches the filter intact. The removal is safe **because neither producer can emit such a row**, which is a fact about papaparse and read-excel-file rather than a property of the predicate. Measured old-vs-new: `data=[[],[]]` gave old `[]`, new `[[]]`. **`Attribute.equals` is not literally a no-op for every heap state.** For a receiver whose `attributeName` has been forced to null via reflection, `equals` previously returned `false` and now throws NPE. Unreachable through every code path in the repo, and such an instance is already unusable — `hashCode()` and `HashSet.add` NPE on it today — but it is not a no-op for *all* possible heaps, only for all constructible ones. ### Two candidates deliberately left in place - **`WorkflowExecution.scala`'s unreachable `forall(_ == READY)` arm** (#8148). It is unreachable — `ExecutionUtils.aggregateStates` maps an all-ready set to `RUNNING`, so no operator can report `READY` — but deleting it would erase the only signal that workflow-level `READY` is intended-but-broken. Whether the fix is to drop the arm or repair `aggregateStates` is a maintainer's decision, not a cleanup. - **`attribute_type.py`'s `Z`-suffix normalisation.** Dead on every interpreter CI runs — `datetime.fromisoformat` has accepted `Z` since 3.11 and the matrix is 3.11/3.12/3.13 — but `amber/pyproject.toml` declares **no `requires-python` floor**, so it is not provably dead for a 3.10 user. Removing it would be a behavioural bet. Also **not** done: "fixing" the row filter to iterate `Object.values(row)`. That would *start* filtering rows — a behaviour change and a product decision. Removing the no-op preserves today's behaviour exactly; if empty-row filtering is actually wanted, it deserves its own change. ### Verification - **`WorkflowCore`**: 683 succeeded, 0 failed. `AttributeSpec` 6/6, including "reject null constructor arguments" — the test that corroborates the guards were unreachable. `SchemaSpec` and `TupleSpec`, the real consumers of `Attribute.equals`/`hashCode`, also pass. Five suites abort with `IllegalStateException: Could not find a valid Docker environment` (Testcontainers, no local Docker) — read out of `target/test-reports/TEST-*.xml`, since sbt's log never names them; unrelated and pre-existing. - **pyamber**: 1292 passed, with the 12-entry non-passing set diffed **by identity** against the local baseline. Two of those failures are in `test_expression_evaluator.py` — the file this PR touches — so they were not taken on trust: restoring the pre-removal file reproduces both identically. Root cause is a Windows/CPython repr mismatch (`hex(id(g))` vs the zero-padded uppercase pointer). Pre-existing. - **frontend**: both affected specs pass (55/55 and 19/19), plus the AOT `ng build`. - **Lint**: `WorkflowCore` `scalafmtCheck` and `scalafix --check` (both configs) pass — scalafix matters here because deleting code can orphan an import. `ruff check` and `ruff format --check` pass on CI's scope. `yarn format:ci` passes. ### Any related issues, documentation, discussions? Closes #8149 Closes #8338 ### How was this PR tested? ``` sbt "WorkflowCore/testOnly org.apache.texera.amber.core.tuple.AttributeSpec" cd amber && python -m pytest -m "not integration" -q cd frontend && npx ng test --watch=false --include="**/user-dataset-file-renderer.component.spec.ts" --include="**/user-dataset-version-creator.component.spec.ts" && npx ng build ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) Report URL: https://github.com/apache/texera/actions/runs/36095505531 With regards, GitHub Actions via GitBox
