aglinxinyuan opened a new pull request, #8339:
URL: https://github.com/apache/texera/pull/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)
--
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]
