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-8339-fbf6b2ca99fd2f75dadb642e0f0dbf9fa2715c7a in repository https://gitbox.apache.org/repos/asf/texera.git
commit 896426c40325f8664e1e1015cf759c6aadc66799 Author: Xinyuan Lin <[email protected]> AuthorDate: Fri Sep 25 04:11:16 2026 +0000 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) --- .../main/python/core/util/expression_evaluator.py | 14 +---------- .../apache/texera/amber/core/tuple/Attribute.java | 7 ------ .../user-dataset-file-renderer.component.ts | 27 ++++++---------------- .../user-dataset-version-creator.component.ts | 3 --- 4 files changed, 8 insertions(+), 43 deletions(-) diff --git a/amber/src/main/python/core/util/expression_evaluator.py b/amber/src/main/python/core/util/expression_evaluator.py index b13e6a0519..d0f9158fde 100644 --- a/amber/src/main/python/core/util/expression_evaluator.py +++ b/amber/src/main/python/core/util/expression_evaluator.py @@ -16,9 +16,8 @@ # under the License. import inspect -import re from collections.abc import Iterator, Mapping -from typing import Any, Dict, List, Optional, Pattern, Tuple +from typing import Any, Dict, List, Optional, Tuple from proto.org.apache.texera.amber.engine.architecture.rpc import ( EvaluatedValue, @@ -152,17 +151,6 @@ class ExpressionEvaluator: def _is_empty_container(obj) -> bool: return hasattr(obj, "__len__") and len(obj) == 0 - @staticmethod - def _contextualize_expression( - expression: str, context_replacements: Dict[Pattern[str], str] - ) -> str: - contextualized_expression = expression - for pattern, contextualized_pattern in context_replacements.items(): - contextualized_expression = re.sub( - pattern, contextualized_pattern, contextualized_expression - ) - return contextualized_expression - @staticmethod def _extract_container_items(value: Any) -> List[TypedValue]: return ExpressionEvaluator._to_typed_values( diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/tuple/Attribute.java b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/tuple/Attribute.java index fb434e0875..0d87ba9911 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/tuple/Attribute.java +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/tuple/Attribute.java @@ -80,13 +80,6 @@ public class Attribute implements Serializable { Attribute that = (Attribute) toCompare; - if (this.attributeName == null) { - return that.attributeName == null; - } - if (this.attributeType == null) { - return that.attributeType == null; - } - return this.attributeName.equalsIgnoreCase(that.attributeName) && this.attributeType.equals(that.attributeType); } diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-file-renderer/user-dataset-file-renderer.component.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-file-renderer/user-dataset-file-renderer.component.ts index 6f93c8ddb8..d6150ee633 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-file-renderer/user-dataset-file-renderer.component.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-file-renderer/user-dataset-file-renderer.component.ts @@ -362,26 +362,13 @@ export class UserDatasetFileRendererComponent implements OnInit, OnChanges, OnDe this.tableDataHeader = data[0]; // Process the rest of the rows - this.tableContent = data - .slice(1) - .map(row => { - // Normalize the row length to match the header length - while (row.length < this.tableDataHeader.length) { - row.push(""); - } - return row; - }) - .filter(row => { - // filter out all empty row - let areCellAllEmpty = true; - for (const cell in row) { - if (cell != "") { - areCellAllEmpty = false; - break; - } - } - return !areCellAllEmpty; - }); + this.tableContent = data.slice(1).map(row => { + // Normalize the row length to match the header length + while (row.length < this.tableDataHeader.length) { + row.push(""); + } + return row; + }); } } } diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-creator/user-dataset-version-creator.component.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-creator/user-dataset-version-creator.component.ts index 4606ff3baa..1eb40dc608 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-creator/user-dataset-version-creator.component.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/user-dataset-version-creator/user-dataset-version-creator.component.ts @@ -128,9 +128,6 @@ export class UserDatasetVersionCreatorComponent implements OnInit { }, ]; } - get formControlNames(): string[] { - return Object.keys(this.form.controls); - } datasetNameSanitization(datasetName: string): string { // Remove leading spaces
