The GitHub Actions job "Required Checks" on texera.git/gh-readonly-queue/main/pr-7927-c16e152f2f2b7b239a8660b77b9b1e539c8c2236 has succeeded. Run started by GitHub user xuang7 (triggered by xuang7).
Head commit for run: e321f0c717cf5873cebd3ce72885f41976b0282d / Eugene Gu <[email protected]> feat(amber): register EvaluatedValue in ControlReturn's sealed oneof (#7927) ### What changes were proposed in this PR? Register `EvaluatedValue` as a member of `ControlReturn`'s sealed oneof (`EvaluatedValue evaluatedValue = 53;` in the worker-response range of `controlreturns.proto`), add two Python test files that pin the fix and the underlying invariant, and add a Jackson mix-in in `JSONUtils.scala` that keeps ScalaPB sealed-oneof helper methods out of websocket JSON. The Jackson behavior is also covered directly in `common/workflow-core` by two `JSONUtilsSpec` tests. **Why this is a bug.** The two proto files disagree about `EvaluatePythonExpression`'s reply type: `workerservice.proto` declares the worker's reply as `EvaluatedValue`, but every worker reply must travel inside `ControlReturn`'s sealed oneof. That oneof registers only the coordinator-side wrapper `EvaluatePythonExpressionResponse` (the reply type of the coordinator's RPC, which contains `repeated EvaluatedValue`). `EvaluatedValue` itself is defined in the same file but never joined the oneof, so the declared worker reply has no wire slot. **The failure is silent.** In the Python engine, `set_one_of` assigns the snake_case field name derived from the type name. Assigning a name that is not a oneof field raises no error on a betterproto dataclass, and serialization ignores it, so the worker's reply is packed into an empty `ControlReturn`: `bytes(set_one_of(ControlReturn, EvaluatedValue(...)))` is `b''` and `get_one_of` returns `None`, with no exception or log. The Python worker's handler produces the correct `EvaluatedValue`, but it is lost at the packing step, so the coordinator's `Future.collect` over worker replies can never receive a real value. **Why fix it in the proto.** The bug lives in the contract, not in either engine's code. Both engines' packing and receiving logic assumes that every declared reply type is registered. Registering the type restores that assumption, and both engines regenerate their bindings from the shared proto (generated bindings are not checked in), so no handler code changes on either side. The alternative—changing `workerservice.proto` to reply with the wrapper type—would require handler changes in both engines for no additional benefit. **Follow-up: keep sealed-oneof helper methods out of websocket JSON.** Joining the sealed oneof makes the generated Scala `EvaluatedValue` extend the `ControlReturn` trait, which carries ScalaPB's `isEmpty` and `isDefined` helper methods. Jackson's getter scan then serializes them as `"empty"` and `"defined"` fields in websocket JSON (`EvaluatedValue` is embedded in the `PythonExpressionEvaluateResponse` websocket event). Deserialization rejects those fields because the constructor knows only `value` and `attributes`. This is the round-trip failure that `TexeraWebSocketEventSpec` caught on the first CI run of this PR. The fix adds a `GeneratedSealedOneofMixin` with `@JsonIgnore` on both methods and registers it on `JSONUtils.objectMapper` against the `scalapb.GeneratedSealedOneof` interface, covering every current and future sealed-oneof member. Alternatives were rejected: disabling `FAIL_ON_UNKNOWN_PROPERTIES` globally would hide real deserialization bugs and leave unwanted fields on the wire; loosening the spec would legitimize those fields; and a per-class mix-in would leave the next sealed-oneof member exposed to the same failure. **Not in scope.** The Scala worker's `evaluatePythonExpression` remains a `???` stub (`DataProcessorRPCHandlerInitializer.scala`), and the coordinator fans the request out to all workers of the operator. Evaluating against an operator with Scala workers therefore still fails on the stub; that is pre-existing behavior independent of this change. ### Any related issues, documentation, discussions? Fixes #7924 ### How was this PR tested? Two Python test files were added under `amber/src/test/python`: - `core/util/proto/test_set_one_of.py` contains regression tests showing that `EvaluatedValue` survives `set_one_of`/`get_one_of`, and that a full wire round-trip produces non-empty bytes that parse back to the original value. With the proto change removed and bindings regenerated from the original proto, both tests fail on the empty-bytes/`None` symptoms; with the change restored, they pass. - `core/architecture/rpc/test_reply_types_registered.py` contains invariant tests for the whole bug class. Every reply type declared by `WorkerServiceStub` (21 RPCs) and `CoordinatorServiceStub` (18 RPCs) must be a registered `ControlReturn` oneof member, and every registered member must survive a real `set_one_of`/`get_one_of` round-trip. Non-empty guards prevent reflection from passing silently if the generated-code layout changes. On the unfixed proto, the invariant test identifies `WorkerServiceStub.evaluate_python_expression -> EvaluatedValue` as the missing registration. For the Jackson mix-in, `TexeraWebSocketEventSpec` goes from 9/10 (failing during round-trip deserialization on the unrecognized `"empty"` field, matching this PR's first CI run) to 10/10. Removing only the mix-in reproduces the original failure. Two direct tests were also added to `common/workflow-core/src/test/scala/org/apache/texera/amber/util/JSONUtilsSpec.scala`: - A representative non-empty generated sealed-oneof member, `OpExecWithCode`, serializes without `empty` or `defined` and round-trips to the original Scala value. - The empty generated sealed-oneof value, `OpExecInitInfo.Empty`, serializes to an empty JSON object without helper properties. The direct `workflow-core` tests and formatting checks pass with: ```bash env JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk-17.jdk/Contents/Home sbt -Dsbt.log.noformat=true "WorkflowCore/testOnly org.apache.texera.amber.util.JSONUtilsSpec" env JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk-17.jdk/Contents/Home sbt -Dsbt.log.noformat=true "WorkflowCore/Test/scalafmtCheck" "WorkflowCore/Compile/scalafmtCheck" ``` The first command passes all 23 `JSONUtilsSpec` tests. `git diff --check` also passes. The Python suite (`pytest -m "not integration"`) passes with the two new files included; `ruff check` and `ruff format --check` pass. ScalaPB code generation and a full `sbt compile` on JDK 17 succeed, and the generated Scala `ControlReturn` gains the `SealedValue.EvaluatedValue` case, preserving the coordinator-side `Future[EvaluatedValue]` typing. The websocket serde specs pass after the mix-in, and `scalafix --check` passes. Manual reproduction before and after the proto change: ```python bytes(set_one_of(ControlReturn, EvaluatedValue(value=TypedValue(expression="1+1", value_str="2")))) ``` Before the change, this returns `b''`. After the change, it returns `b'\xaa\x03\n\n\x08\n\x031+1\x1a\x012'` (field 53), and `get_one_of` returns the full value. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Fable 5) and OpenAI Codex --------- Co-authored-by: Xuan Gu <[email protected]> Report URL: https://github.com/apache/texera/actions/runs/35549495614 With regards, GitHub Actions via GitBox
