eugenegujing opened a new pull request, #7927: URL: https://github.com/apache/texera/pull/7927
### What changes were proposed in this PR? Register `EvaluatedValue` as a member of `ControlReturn`'s sealed oneof (`EvaluatedValue evaluatedValue = 53;` in the worker-responses range of `controlreturns.proto`), and add two Python test files that pin the fix and the underlying invariant. **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, and that oneof only registers the coordinator-side wrapper `EvaluatePythonExpressionResponse` (the reply type of the *coordinator's* RPC, which is `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.** On the Python engine, `set_one_of` assigns by the snake_case field name derived from the type name; assigning a name that is not a oneof field raises nothing 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 and no log anywhere. The Python worker's handler does produce the correct `EvaluatedValue`; 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/receiving logic is correct under the assumption that the declared reply type is registered. Registering the type restores that assumption, both engines regenerate their bindings from the shared proto (generated bindings are not checked in), and no handler code changes on either side. The alternative (changing `workerservice.proto` to reply with the wrapper type) would touch handlers in both engines for no additional benefit. **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, so evaluating against an operator with Scala workers still fails on the stub; that is pre-existing behavior independent of this fix. ### Any related issues, documentation, discussions? Fixes #7924 ### How was this PR tested? Two new test files were added under `amber/src/test/python`: - `core/util/proto/test_set_one_of.py` — regression tests pinning this bug: `EvaluatedValue` survives `set_one_of`/`get_one_of`, and a full wire round-trip produces non-empty bytes that parse back to the original value. Rollback-verified: with the proto fix removed and bindings regenerated from the original proto, both tests fail exactly on the empty-bytes/`None` symptoms; with the fix restored they pass. - `core/architecture/rpc/test_reply_types_registered.py` — invariant tests covering the whole bug class: every reply type declared by `WorkerServiceStub` (21 RPCs) and `CoordinatorServiceStub` (18 RPCs) must be a registered `ControlReturn` oneof member (the assertion message names any missing type and points to `controlreturns.proto`), and every registered member must survive a real `set_one_of`/`get_one_of` round-trip, which also pins the field-naming convention `set_one_of` depends on; non-empty guards prevent the reflection from silently passing if the generated-code layout ever changes. Rollback-verified: on the unfixed proto the invariant test fails precisely on `WorkerServiceStub.evaluate_python_expression -> EvaluatedValue`, so this class of bug is caught by CI at the PR that introduces it. Full verification: 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 so the coordinator-side `Future[EvaluatedValue]` typing holds. Manual repro before/after: `bytes(set_one_of(ControlReturn, EvaluatedValue(value=TypedValue(expression="1+1", value_str="2"))))` returns `b''` before the fix and `b'\xaa\x03\n\n\x08\n\x031+1\x1a\x012'` (field 53) after, with `get_one_of` returning the full value. ### Was this PR authored or co-authored using generative AI tooling? Co-authored by: Claude Code (Claude Fable 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]
