From bd3b2acecb0e26c5b112f9226af2aa3c047aa448 Mon Sep 17 00:00:00 2001 From: carloea2 Date: Fri, 7 Aug 2026 11:24:45 -0600 Subject: [PATCH 01/10] feat(engine): add TryCatch/Finally control blocks with in-band error handling Introduce block-level try/catch/finally semantics for workflows: - TryCatch logical operator (Try / Catch / Error Info ports) expanding to a splitter + catch gate; Finally logical operator (From Try / From Catch in, Try Result / Catch Result out) backed by a staging merger that releases exactly one branch's complete output through the port named for the winner. - Operator failures inside a frame become ordinary States with a reserved __error__ key, broadcast in-band; per-port drain contagion in both workers discards post-failure data without invoking executors and suppresses finish hooks while ports still complete, so the stream terminates. - Failures outside any frame keep the existing behavior (console error + pause, current input retriable): TryCatchFramePass bakes a per-operator guarded flag delivered via InitializeExecutorRequest. - TryCatchFramePass pairs frames, computes try/catch cones, synthesizes signal edges (SignalPartitioning drops tuples at the sender) with a dependee snapshot port for structural resolution timing, and validates the wiring rules (disjoint cones, Finally provenance, no Merger bypass, Error Info cannot feed its own try cone). - Fix multi-dependee port scheduling (sliding(2,1) fabricated dependency pairs) and guard IfOpExec against unknown States. - pyamber parity: error State emission, drain guards, signal partitioner, and a failed-cycle handshake fix in DataProcessor so a failing Python UDF terminates cleanly. - Docs pages for both operators; unit + integration test coverage. --- .../amber/bench/ArrowFlightActorBench.scala | 3 +- .../architecture/rpc/controlcommands.proto | 5 + .../sendsemantics/partitionings.proto | 9 + .../control/initialize_executor_handler.py | 3 + .../core/architecture/managers/context.py | 5 + .../architecture/packaging/output_manager.py | 3 + .../sendsemantics/signal_partitioner.py | 75 +++ amber/src/main/python/core/models/state.py | 33 ++ .../python/core/runnables/data_processor.py | 29 +- .../main/python/core/runnables/main_loop.py | 125 ++++- .../main/python/core/util/virtual_identity.py | 14 + .../messaginglayer/OutputManager.scala | 3 + .../ExpansionGreedyScheduleGenerator.scala | 36 +- .../scheduling/RegionExecutionManager.scala | 3 +- .../scheduling/config/LinkConfig.scala | 10 + .../partitioners/SignalPartitioner.scala | 41 ++ .../architecture/worker/DataProcessor.scala | 64 ++- .../promisehandlers/EndChannelHandler.scala | 27 +- .../InitializeExecutorHandler.scala | 1 + .../engine/e2e/TryCatchIntegrationSpec.scala | 414 +++++++++++++++ .../core/runnables/test_data_processor.py | 37 ++ .../python/core/runnables/test_main_loop.py | 193 +++++-- .../worker/DataProcessorSpec.scala | 152 +++++- .../architecture/worker/WorkerSpec.scala | 3 +- .../worker/WorkflowWorkerSpec.scala | 3 +- .../managers/SerializationManagerSpec.scala | 3 +- .../common/compiler/TryCatchFramePass.scala | 477 ++++++++++++++++++ .../common/compiler/WorkflowCompiler.scala | 6 +- .../compiler/TryCatchFramePassSpec.scala | 402 +++++++++++++++ .../texera/amber/core/state/State.scala | 31 ++ .../amber/core/workflow/PartitionInfo.scala | 10 + .../amber/core/workflow/PhysicalOp.scala | 33 +- .../core/workflow/PartitionInfoSpec.scala | 2 + .../amber/core/workflow/PhysicalOpSpec.scala | 29 ++ .../texera/amber/operator/LogicalOp.scala | 3 + .../amber/operator/ifStatement/IfOpExec.scala | 8 +- .../operator/trycatch/CatchGateConfig.scala | 34 ++ .../operator/trycatch/CatchGateOpExec.scala | 111 ++++ .../trycatch/FinallyMergerConfig.scala | 34 ++ .../trycatch/FinallyMergerOpExec.scala | 98 ++++ .../operator/trycatch/FinallyOpDesc.scala | 95 ++++ .../operator/trycatch/TryCatchOpDesc.scala | 145 ++++++ .../operator/trycatch/TrySplitterOpExec.scala | 43 ++ .../trycatch/CatchGateOpExecSpec.scala | 94 ++++ .../trycatch/FinallyMergerOpExecSpec.scala | 127 +++++ .../trycatch/TryCatchOpDescSpec.scala | 143 ++++++ .../operators/control-block/_index.md | 4 +- .../operators/control-block/finally.md | 79 +++ .../operators/control-block/try-catch.md | 105 ++++ .../src/assets/operator_images/Finally.png | Bin 0 -> 477 bytes .../src/assets/operator_images/TryCatch.png | Bin 0 -> 523 bytes 51 files changed, 3305 insertions(+), 102 deletions(-) create mode 100644 amber/src/main/python/core/architecture/sendsemantics/signal_partitioner.py create mode 100644 amber/src/main/scala/org/apache/texera/amber/engine/architecture/sendsemantics/partitioners/SignalPartitioner.scala create mode 100644 amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala create mode 100644 common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala create mode 100644 common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDesc.scala create mode 100644 common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TrySplitterOpExec.scala create mode 100644 common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala create mode 100644 common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala create mode 100644 common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala create mode 100644 docs/reference/operators/control-block/finally.md create mode 100644 docs/reference/operators/control-block/try-catch.md create mode 100644 frontend/src/assets/operator_images/Finally.png create mode 100644 frontend/src/assets/operator_images/TryCatch.png diff --git a/amber/src/bench/scala/org/apache/texera/amber/bench/ArrowFlightActorBench.scala b/amber/src/bench/scala/org/apache/texera/amber/bench/ArrowFlightActorBench.scala index e3b79ea8040..0a7c84b8f52 100644 --- a/amber/src/bench/scala/org/apache/texera/amber/bench/ArrowFlightActorBench.scala +++ b/amber/src/bench/scala/org/apache/texera/amber/bench/ArrowFlightActorBench.scala @@ -287,7 +287,8 @@ object ArrowFlightActorBench { 1, OpExecWithCode(IdentityPythonCode, "python"), isSource = false, - loopStartStateUris = Map.empty + loopStartStateUris = Map.empty, + guarded = false ), ctx, 0L diff --git a/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/rpc/controlcommands.proto b/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/rpc/controlcommands.proto index 8a6403a97e8..d61492b2840 100644 --- a/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/rpc/controlcommands.proto +++ b/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/rpc/controlcommands.proto @@ -260,6 +260,11 @@ message InitializeExecutorRequest { // the consumed StateFrame and writes the next-iteration state there. Empty // for plans without loops. map loopStartStateUris = 4; + // True iff this operator sits inside a try/catch frame's cone (set by + // TryCatchFramePass). A guarded worker turns its own executor failure into + // an in-band error State and drains; an unguarded worker keeps the default + // behavior: report and pause. + bool guarded = 5; } message UpdateExecutorRequest { diff --git a/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/sendsemantics/partitionings.proto b/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/sendsemantics/partitionings.proto index 813a4041b31..faa9338d880 100644 --- a/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/sendsemantics/partitionings.proto +++ b/amber/src/main/protobuf/org/apache/texera/amber/engine/architecture/sendsemantics/partitionings.proto @@ -35,6 +35,7 @@ message Partitioning{ HashBasedShufflePartitioning hashBasedShufflePartitioning = 3; RangeBasedShufflePartitioning rangeBasedShufflePartitioning = 4; BroadcastPartitioning broadcastPartitioning = 5; + SignalPartitioning signalPartitioning = 6; } } @@ -66,3 +67,11 @@ message BroadcastPartitioning{ int32 batchSize = 1; repeated core.ChannelIdentity channels = 2; } + +// Signal links (try/catch frame wiring): data tuples are dropped at the +// sender; only States, ECMs and END_CHANNEL traverse the link. Channels must +// still be declared so the buffers/channels exist for those control payloads. +message SignalPartitioning{ + int32 batchSize = 1; + repeated core.ChannelIdentity channels = 2; +} diff --git a/amber/src/main/python/core/architecture/handlers/control/initialize_executor_handler.py b/amber/src/main/python/core/architecture/handlers/control/initialize_executor_handler.py index 50fceab7feb..f845daf016e 100644 --- a/amber/src/main/python/core/architecture/handlers/control/initialize_executor_handler.py +++ b/amber/src/main/python/core/architecture/handlers/control/initialize_executor_handler.py @@ -32,4 +32,7 @@ async def initialize_executor(self, req: InitializeExecutorRequest) -> EmptyRetu ) # Loop-back write addresses; see the proto field doc on loopStartStateUris. self.context.loop_start_state_uris = dict(req.loop_start_state_uris) + # Frame membership decides the failure path: drain (guarded) vs the + # default report-and-pause (unguarded). + self.context.guarded = req.guarded return EmptyReturn() diff --git a/amber/src/main/python/core/architecture/managers/context.py b/amber/src/main/python/core/architecture/managers/context.py index dfcc30f9aa9..9871a66f188 100644 --- a/amber/src/main/python/core/architecture/managers/context.py +++ b/amber/src/main/python/core/architecture/managers/context.py @@ -90,6 +90,11 @@ def __init__(self, worker_id, input_queue): # Loop-back write addresses delivered at setup; see the proto field doc # on InitializeExecutorRequest.loopStartStateUris (controlcommands.proto). self.loop_start_state_uris: Dict[str, str] = {} + # True iff this operator sits inside a try/catch frame's cone (from + # InitializeExecutorRequest.guarded, set by TryCatchFramePass). + # Guarded: own executor failure becomes an in-band error State and the + # worker drains. Unguarded: default behavior — report and pause. + self.guarded: bool = False def report_exception(self, err: BaseException) -> None: """Route an operator-facing exception to the exception manager and diff --git a/amber/src/main/python/core/architecture/packaging/output_manager.py b/amber/src/main/python/core/architecture/packaging/output_manager.py index bc6829520eb..6e4b3918027 100644 --- a/amber/src/main/python/core/architecture/packaging/output_manager.py +++ b/amber/src/main/python/core/architecture/packaging/output_manager.py @@ -41,6 +41,7 @@ from core.architecture.sendsemantics.round_robin_partitioner import ( RoundRobinPartitioner, ) +from core.architecture.sendsemantics.signal_partitioner import SignalPartitioner from core.models import Tuple, Schema, StateFrame from core.models.payload import DataPayload, DataFrame from core.models.state import State @@ -66,6 +67,7 @@ RoundRobinPartitioning, RangeBasedShufflePartitioning, BroadcastPartitioning, + SignalPartitioning, ) @@ -81,6 +83,7 @@ def __init__(self, worker_id: str): HashBasedShufflePartitioning: HashBasedShufflePartitioner, RangeBasedShufflePartitioning: RangeBasedShufflePartitioner, BroadcastPartitioning: BroadcastPartitioner, + SignalPartitioning: SignalPartitioner, } self._ports: typing.Dict[PortIdentity, WorkerPort] = dict() self._channels: typing.Dict[ChannelIdentity, Channel] = dict() diff --git a/amber/src/main/python/core/architecture/sendsemantics/signal_partitioner.py b/amber/src/main/python/core/architecture/sendsemantics/signal_partitioner.py new file mode 100644 index 00000000000..4c1a78e0f2e --- /dev/null +++ b/amber/src/main/python/core/architecture/sendsemantics/signal_partitioner.py @@ -0,0 +1,75 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import typing +from overrides import overrides +from typing import Iterator + +from core.architecture.sendsemantics.partitioner import Partitioner +from core.models import Tuple +from core.models.state import State +from core.util import set_one_of +from proto.org.apache.texera.amber.core import ActorVirtualIdentity +from proto.org.apache.texera.amber.engine.architecture.rpc import EmbeddedControlMessage +from proto.org.apache.texera.amber.engine.architecture.sendsemantics import ( + Partitioning, + SignalPartitioning, +) + + +class SignalPartitioner(Partitioner): + """Partitioner for signal links (try/catch frame wiring). + + Data tuples are dropped at the sender -- never batched, serialized, or + networked -- while States and ECMs still travel, because a frame's gate + only needs the error signal and the end-of-stream marker. Mirror of the + Scala `SignalPartitioner`. + """ + + def __init__(self, partitioning: SignalPartitioning): + super().__init__(set_one_of(Partitioning, partitioning)) + self.receivers = list( + {channel.to_worker_id for channel in partitioning.channels} + ) + + @overrides + def add_tuple_to_batch( + self, tuple_: Tuple + ) -> Iterator[typing.Tuple[ActorVirtualIdentity, typing.List[Tuple]]]: + # tuples never traverse a signal link + return iter(()) + + @overrides + def flush( + self, to: ActorVirtualIdentity, ecm: EmbeddedControlMessage + ) -> Iterator[typing.Union[EmbeddedControlMessage, typing.List[Tuple]]]: + for receiver in self.receivers: + if receiver == to: + yield ecm + + @overrides + def flush_state( + self, state: State + ) -> Iterator[ + typing.Tuple[ActorVirtualIdentity, typing.Union[State, typing.List[Tuple]]] + ]: + for receiver in self.receivers: + yield receiver, state + + @overrides + def reset(self) -> None: + pass diff --git a/amber/src/main/python/core/models/state.py b/amber/src/main/python/core/models/state.py index 559d6eca45f..bacd357d875 100644 --- a/amber/src/main/python/core/models/state.py +++ b/amber/src/main/python/core/models/state.py @@ -24,6 +24,14 @@ class State(dict): + # Reserved key marking a State as an in-band error signal (an operator + # failure traveling forward as a dataflow event). Mirror of the Scala + # `State.ErrorKey` (core/state/State.scala): the value is an envelope with + # operatorId / workerId / errorType / message. Recognition and port + # poisoning live in the worker runtime (MainLoop), not in operators. + ERROR_KEY = "__error__" + _ERR_OPERATOR_ID = "operatorId" + CONTENT = "content" # Loop-control bookkeeping owned by the worker runtime, NOT user state -- it # never appears in the content JSON. In memory it rides on the StateFrame @@ -78,6 +86,31 @@ def from_json(cls, payload: str) -> "State": def from_tuple(cls, row: Tuple) -> "State": return cls.from_json(row[cls.CONTENT]) + @classmethod + def error(cls, operator_id: str, worker_id: str, err: BaseException) -> "State": + """Build the in-band error signal for a failed operator.""" + return cls( + { + cls.ERROR_KEY: { + cls._ERR_OPERATOR_ID: operator_id, + "workerId": worker_id, + "errorType": type(err).__name__, + "message": str(err), + } + } + ) + + def is_error(self) -> bool: + return State.ERROR_KEY in self + + def error_operator_id(self) -> Any: + """The failing operator's canonical physical id, or None if not an + error State. Used by try/catch frames for own-cone attribution.""" + envelope = self.get(State.ERROR_KEY) + if isinstance(envelope, dict): + return envelope.get(State._ERR_OPERATOR_ID) + return None + _TYPE_MARKER = "__texera_type__" _PAYLOAD_MARKER = "payload" diff --git a/amber/src/main/python/core/runnables/data_processor.py b/amber/src/main/python/core/runnables/data_processor.py index 22e7058f27d..7461fe01240 100644 --- a/amber/src/main/python/core/runnables/data_processor.py +++ b/amber/src/main/python/core/runnables/data_processor.py @@ -65,6 +65,15 @@ def run(self) -> None: else: self.process_tuple() + # NOTE on the failure path: `_executor_session` sets `finished_current` + # in its except branch, BEFORE the final context switch. The loop above + # re-checks the input slots only after MainLoop queued the next input and + # notified -- which is exactly the invariant the assertion enforces. If + # the cycle were marked finished only after that switch (on this thread's + # next wake-up), MainLoop's cycle loop would see an unfinished cycle, + # wake this thread once more WITHOUT queuing anything, the assertion + # would kill this thread, and MainLoop's next switch would wait forever. + def process_internal_marker(self, internal_marker: InternalMarker) -> None: with self._executor_session() as (executor, port_id): if isinstance(internal_marker, StartChannel): @@ -102,8 +111,18 @@ def _executor_session(self): and queue the stack trace as a console message, and always switch back to MainLoop on exit. Reporting must happen *before* the switch: MainLoop's post-switch hook flushes console messages and - then enters EXCEPTION_PAUSE, so anything queued after the switch - would arrive at the coordinator only after the worker resumes. + broadcasts the error State, so anything queued after the switch + would reach the coordinator late. + + On an exception, `_set_output_tuple` never ran, so the cycle is + finished HERE, before the final switch — the same ordering the + normal path has (`_set_output_tuple` sets `finished_current` before + its last switch). MainLoop's cycle loop then ends the cycle on its + first check, exactly like a normal cycle that yielded its trailing + None, and this thread parks in the switch until the next input is + queued. (For unguarded workers the failure ping-pong is parked by + EXCEPTION_PAUSE instead; guarded workers drain — MainLoop's drain + guard skips the executor for the rest of the poisoned port.) """ try: executor = self._context.executor_manager.executor @@ -115,6 +134,12 @@ def _executor_session(self): yield executor, port_id except Exception as err: self._context.report_exception(err) + if self._context.guarded: + # Drain semantics: end the cycle now. Unguarded workers leave + # the cycle OPEN instead — MainLoop pauses on its side of the + # switch and the current input stays retriable + # (RetryCurrentTuple), exactly the pause-and-retry flow. + self._context.tuple_processing_manager.finished_current.set() finally: self._switch_context() diff --git a/amber/src/main/python/core/runnables/main_loop.py b/amber/src/main/python/core/runnables/main_loop.py index fe44b36044d..426a670cd8e 100644 --- a/amber/src/main/python/core/runnables/main_loop.py +++ b/amber/src/main/python/core/runnables/main_loop.py @@ -46,7 +46,7 @@ from core.util import StoppableQueueBlockingRunnable, get_one_of from core.util.console_message.timestamp import current_time_in_local_timezone from core.util.customized_queue.queue_base import QueueElement -from core.util.virtual_identity import get_logical_op_id +from core.util.virtual_identity import get_logical_op_id, get_physical_op_id_string from proto.org.apache.texera.amber.core import ( ActorVirtualIdentity, PortIdentity, @@ -93,6 +93,15 @@ def __init__( # same iteration's state arrives once per branch. Workers are recreated # on each region re-execution, so this instance flag is per iteration. self._loop_state_consumed: bool = False + # Per-port drain contagion, mirroring the Scala worker: a port is + # poisoned when an `__error__` State arrives on + # it; `_self_failed` poisons every port at once when this worker's own + # executor throws. Data on a poisoned port is discarded without invoking + # the executor and its finish hooks are suppressed -- so no + # post-failure side effects run in user code -- while ports still + # complete normally so the stream terminates instead of hanging. + self._poisoned_ports: typing.Set[PortIdentity] = set() + self._self_failed: bool = False self.context = Context(worker_id, input_queue) self._async_rpc_server = AsyncRPCServer(output_queue, context=self.context) @@ -136,7 +145,14 @@ def complete(self) -> None: self._check_and_report_console_messages(force_flush=True) coordinator_interface = self._async_rpc_client.coordinator_stub() executor = self.context.executor_manager.executor - if isinstance(executor, LoopEndOperator): + if isinstance(executor, LoopEndOperator) and self._self_failed: + # This LoopEnd's own executor failed: do not evaluate condition() or + # take the back-edge -- a failed iteration must not re-iterate on + # partial loop state. The error State already went downstream, so an + # enclosing try/catch frame (or the console error) reports it; the + # worker still completes so the stream terminates. + pass + elif isinstance(executor, LoopEndOperator): # condition() evaluates a user-supplied expression, and the # loop-back edge writes state to iceberg after the jump DCM -- # both on this main loop thread, outside DataProcessor's guarded @@ -336,6 +352,19 @@ def _process_dcm(self, dcm_element: DCMElement) -> None: self.context.statistics_manager.update_total_execution_time(end_time) def _process_tuple(self, tuple_: Tuple) -> None: + port_id = self.context.tuple_processing_manager.current_input_port_id + if self._is_port_poisoned(port_id): + # This data belongs to an already-failed attempt: consume and + # discard without invoking the executor (no post-failure side + # effects in user code). No control check here -- unlike real + # processing, a discard is O(1), and _check_and_process_control + # blocks while data is disabled; control messages are still handled + # between batches and on markers. + if port_id is not None: + self.context.statistics_manager.increase_input_statistics( + port_id, tuple_.in_mem_size() + ) + return self.context.tuple_processing_manager.current_input_tuple = tuple_ self.process_input_tuple() self._check_and_process_control() @@ -402,6 +431,13 @@ def _process_state_frame(self, frame: StateFrame) -> None: output_loop_counter=in_counter, output_loop_start_id=frame.loop_start_id, ) + # Poison AFTER delivery: the executor sees the error State first (frame + # operators react to it; the default pass-through forwards it), then the + # port drains from the next data tuple on. + if isinstance(state, State) and state.is_error(): + port_id = self._current_input_port_id() + if port_id is not None: + self._poisoned_ports.add(port_id) self._check_and_process_control() def _process_start_channel(self) -> None: @@ -411,19 +447,14 @@ def _process_start_channel(self) -> None: self.process_input_state() def _process_end_channel(self) -> None: - self.process_input_state() - if self.context.exception_manager.has_exception(): - # A state-emission error was reported on the main loop thread (see - # _emit_and_save_state). Hold the region: skip port_completed and - # complete() so the coordinator does not mark the region complete - # (region completion is port-based) with partial, single-iteration - # results. The reported error surfaces instead of a false success. - return - self.process_input_tuple() - - input_port_id = self.context.input_manager.get_port_id( - self.context.current_input_channel_id - ) + input_port_id = self._current_input_port_id() + # A poisoned port belongs to a failed attempt: suppress the executor's + # finish hooks (produce_state_on_finish / on_finish) so no post-failure + # side effects or junk final emissions happen. Port completion below + # still runs, so the stream terminates normally instead of hanging. + if not self._is_port_poisoned(input_port_id): + self.process_input_state() + self.process_input_tuple() if input_port_id is not None: self._async_rpc_client.coordinator_stub().port_completed( @@ -628,9 +659,71 @@ def _check_and_report_debug_event(self) -> None: self.context.pause_manager.pause(PauseType.DEBUG_PAUSE) def _check_exception(self) -> None: + """Route a reported operator exception down the failure path. + + Mirror of the Scala worker's `handleExecutorException`: the console + message always reports the error first. Then, GUARDED workers (inside + a try/catch frame's cone) broadcast an + `__error__` State so downstream workers drain and the frame can react, + and drain their own remaining input — ports still complete, the run + terminates. UNGUARDED workers keep the default product behavior: + EXCEPTION_PAUSE, leaving the worker inspectable and the current input + retriable (RetryCurrentTuple). + """ if self.context.exception_manager.has_exception(): self._check_and_report_console_messages(force_flush=True) - self.context.pause_manager.pause(PauseType.EXCEPTION_PAUSE) + if not self.context.guarded: + self.context.pause_manager.pause(PauseType.EXCEPTION_PAUSE) + elif not self._self_failed: + self._self_failed = True + self._emit_error_state() + + def _emit_error_state(self) -> None: + """Broadcast the error State for this worker's own failure.""" + # Read the field directly: get_exc_info() CONSUMES the exception (it + # nulls exc_info for the replay/debug path), and clearing it here would + # make the worker look healthy to every later has_exception() check. + exc_info = self.context.exception_manager.exc_info + err = exc_info[1] if exc_info else RuntimeError("operator failed") + try: + operator_id = get_physical_op_id_string(self.context.worker_id) + except ValueError: + # A malformed worker id must not raise on the failure path -- a + # secondary error here would lose the signal entirely. Fall back to + # the raw id: frame attribution then treats the error as foreign, + # which escalates outward (the safe direction) instead of a frame + # wrongly claiming it. + operator_id = self.context.worker_id + error_state = State.error( + operator_id, + self.context.worker_id, + err, + ) + # Emit directly: _emit_and_save_state funnels back into _check_exception + # on failure, and the storage write is irrelevant for a failure signal. + try: + self._emit_batches(self.context.output_manager.emit_state(error_state)) + except Exception as emit_err: # pragma: no cover - defensive + logger.exception(emit_err) + + def _current_input_port_id(self) -> Optional[PortIdentity]: + """The input port the current channel belongs to, or None when the + channel is not registered (only reachable outside a normal data path).""" + channel_id = self.context.current_input_channel_id + if channel_id is None: + return None + try: + return self.context.input_manager.get_port_id(channel_id) + except KeyError: + return None + + def _is_port_poisoned(self, port_id: Optional[PortIdentity]) -> bool: + """Whether this port belongs to an already-failed attempt: data on it is + discarded without invoking the executor, and the executor's finish hooks + for it are suppressed (per-port drain contagion).""" + return self._self_failed or ( + port_id is not None and port_id in self._poisoned_ports + ) def _check_and_report_console_messages(self, force_flush=False) -> None: for msg in self.context.console_message_manager.get_messages(force_flush): diff --git a/amber/src/main/python/core/util/virtual_identity.py b/amber/src/main/python/core/util/virtual_identity.py index 0a9de0ccc48..43dc1fe3f47 100644 --- a/amber/src/main/python/core/util/virtual_identity.py +++ b/amber/src/main/python/core/util/virtual_identity.py @@ -57,6 +57,20 @@ def get_logical_op_id(worker_id: str) -> str: raise ValueError(f"Invalid worker ID format: {worker_id}") +def get_physical_op_id_string(worker_id: str) -> str: + """ + The canonical ``/`` physical-operator id string for + a worker actor name. Must stay byte-identical to the Scala side's + ``s"${opId.logicalOpId.id}/${opId.layerName}"`` (DataProcessor's error-State + envelope), because try/catch frames compare these strings against the + compile-time cone sets baked into their gate/merger configs. + """ + match = worker_name_pattern.fullmatch(worker_id) + if match: + return f"{match.group(2)}/{match.group(3)}" + raise ValueError(f"Invalid worker ID format: {worker_id}") + + def serialize_global_port_identity(obj: GlobalPortIdentity) -> str: """ Serialize GlobalPortIdentity into a custom human-readable string. diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/messaginglayer/OutputManager.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/messaginglayer/OutputManager.scala index b15e8992546..853c47fd62f 100644 --- a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/messaginglayer/OutputManager.scala +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/messaginglayer/OutputManager.scala @@ -57,6 +57,8 @@ object OutputManager { RangeBasedShufflePartitioner(rangeBasedShufflePartitioning) case broadcastPartitioning: BroadcastPartitioning => BroadcastPartitioner(broadcastPartitioning) + case signalPartitioning: SignalPartitioning => + SignalPartitioner(signalPartitioning) case _ => throw new RuntimeException(s"partitioning $partitioning not supported") } partitioner @@ -69,6 +71,7 @@ object OutputManager { case p: HashBasedShufflePartitioning => p.batchSize case p: RangeBasedShufflePartitioning => p.batchSize case p: BroadcastPartitioning => p.batchSize + case p: SignalPartitioning => p.batchSize case _ => throw new RuntimeException(s"partitioning $partitioning not supported") } } diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/ExpansionGreedyScheduleGenerator.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/ExpansionGreedyScheduleGenerator.scala index 304e1496f8a..abdebf052b5 100644 --- a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/ExpansionGreedyScheduleGenerator.scala +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/ExpansionGreedyScheduleGenerator.scala @@ -199,13 +199,15 @@ class ExpansionGreedyScheduleGenerator( physicalOpId: PhysicalOpIdentity, regionDAG: DirectedAcyclicGraph[Region, RegionLink] ): Option[Set[PhysicalLink]] = { - // For operators like HashJoin's Probe that have dependencies between their input ports + // For operators like HashJoin's Probe that have dependencies between their input ports. + // Iterate the ACTUAL declared (dependee, depender) pairs — a port may depend + // on N ports (e.g. a snapshot port depending on many signal ports), so + // consecutive positions in the topological processing order are not pairs. physicalPlan .getOperator(physicalOpId) - .getInputPortDependencyPairs - .sliding(2, 1) + .getInputPortDependencyEdges .foreach { - case List(dependeePort, dependerPort) => + case (dependeePort, dependerPort) => // Create edges between regions val dependeeEdges = physicalPlan @@ -216,14 +218,18 @@ class ExpansionGreedyScheduleGenerator( .getUpstreamPhysicalLinks(physicalOpId) .filter(l => l.toPortId == dependerPort) - if (dependerEdges.nonEmpty) { - // The depender port is connected to some edges of this same region - val regionOrderPairs = - toRegionOrderPairs( - dependeeEdges.head.fromOpId, - dependerEdges.head.fromOpId, - regionDAG - ) + if (dependeeEdges.isEmpty) { + // The dependee port reads only from materialization: ordering is + // already enforced by storage; nothing to add for this pair. + } else if (dependerEdges.nonEmpty) { + // The depender port is connected to some edges of this same region. + // A dependee/depender port may have multiple incoming edges (fan-in): + // every dependee-side region must precede every depender-side region. + val regionOrderPairs = for { + dependeeEdge <- dependeeEdges + dependerEdge <- dependerEdges + pair <- toRegionOrderPairs(dependeeEdge.fromOpId, dependerEdge.fromOpId, regionDAG) + } yield pair // Attempt to add these depender edges to regionDAG try { regionOrderPairs.foreach { @@ -242,8 +248,9 @@ class ExpansionGreedyScheduleGenerator( } else { // The depender port is not connected to any edges (due to materializations) try { - // Any region that the dependee port belongs to needs to run first. - val dependeeRegions = getRegions(dependeeEdges.head.fromOpId, regionDAG) + // Any region that a dependee edge belongs to needs to run first. + val dependeeRegions = + dependeeEdges.flatMap(edge => getRegions(edge.fromOpId, regionDAG)).toSet // Any region that this depender port belongs to need to run after those dependee regions. val dependerRegion = getRegions(physicalOpId, regionDAG) .filter(region => @@ -270,7 +277,6 @@ class ExpansionGreedyScheduleGenerator( ) } } - case _ => } None } diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/RegionExecutionManager.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/RegionExecutionManager.scala index 0b32b74101a..6557f65597a 100644 --- a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/RegionExecutionManager.scala +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/RegionExecutionManager.scala @@ -452,7 +452,8 @@ class RegionExecutionManager( workerConfigs.length, physicalOp.opExecInitInfo, physicalOp.isSourceOperator, - loopStartStateUris + loopStartStateUris, + guarded = physicalOp.isGuarded ), asyncRPCClient.mkContext(workerId) ) diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/config/LinkConfig.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/config/LinkConfig.scala index ef92afef593..f0e288757ec 100644 --- a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/config/LinkConfig.scala +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/config/LinkConfig.scala @@ -77,6 +77,16 @@ case object LinkConfig { ) ) + case SignalPartition() => + // signal links (try/catch frame wiring): channels exist for States, + // ECMs and END_CHANNEL, but data tuples are dropped at the sender + SignalPartitioning( + dataTransferBatchSize, + fromWorkerIds.flatMap(from => + toWorkerIds.map(to => ChannelIdentity(from, to, isControl = false)) + ) + ) + case UnknownPartition() => RoundRobinPartitioning( dataTransferBatchSize, diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/sendsemantics/partitioners/SignalPartitioner.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/sendsemantics/partitioners/SignalPartitioner.scala new file mode 100644 index 00000000000..dfb1c31e0b6 --- /dev/null +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/sendsemantics/partitioners/SignalPartitioner.scala @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.engine.architecture.sendsemantics.partitioners + +import org.apache.texera.amber.core.tuple.Tuple +import org.apache.texera.amber.core.virtualidentity.ActorVirtualIdentity +import org.apache.texera.amber.engine.architecture.sendsemantics.partitionings.SignalPartitioning + +/** + * Partitioner for signal links (try/catch frame wiring): every data tuple is + * dropped at the sender — never serialized, never networked — while States, + * ECMs and END_CHANNEL still traverse the link, because those ride the + * broadcast/channel path (`OutputManager.emitState`, `sendECMToDataChannels`) + * rather than `getBucketIndex`. Receivers must still be declared so the + * network buffers and channels exist for that control traffic. + */ +case class SignalPartitioner(partitioning: SignalPartitioning) extends Partitioner { + + private val receivers = partitioning.channels.map(_.toWorkerId).distinct + + override def getBucketIndex(tuple: Tuple): Iterator[Int] = Iterator.empty + + override def allReceivers: Seq[ActorVirtualIdentity] = receivers +} diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessor.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessor.scala index 618fa73427a..2489de1f13c 100644 --- a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessor.scala +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessor.scala @@ -59,8 +59,10 @@ import org.apache.texera.amber.engine.common.ambermessage._ import org.apache.texera.amber.engine.common.statetransition.WorkerStateManager import org.apache.texera.amber.engine.common.virtualidentity.util.COORDINATOR import org.apache.texera.amber.error.ErrorUtils.{mkConsoleMessage, safely} +import org.apache.texera.amber.util.VirtualIdentityUtils import java.util.concurrent.LinkedBlockingQueue +import scala.collection.mutable class DataProcessor( actorId: ActorVirtualIdentity, @@ -91,6 +93,24 @@ class DataProcessor( inputGateway.getChannel(channelId).getQueuedCredit } + // Per-port drain contagion: a port is + // poisoned when an error State arrives on it, or all ports at once when this + // worker's own executor throws. Data tuples on a poisoned port are discarded + // without invoking the executor, and the executor's onFinish for that port is + // suppressed; ports still complete normally so END_CHANNEL propagation and + // FinalizePort behave exactly as on a healthy run. + private val poisonedPorts = mutable.HashSet[PortIdentity]() + private var selfFailed = false + + // True iff this operator sits inside a try/catch frame's cone (from + // InitializeExecutorRequest.guarded, set by TryCatchFramePass). Guarded: + // own executor failure becomes an in-band error State and the worker + // drains. Unguarded: the default behavior — report and pause. + var guarded: Boolean = false + + def isPortPoisoned(portId: PortIdentity): Boolean = + selfFailed || poisonedPorts.contains(portId) + /** * provide API for actor to get stats of this operator */ @@ -102,9 +122,15 @@ class DataProcessor( * this function is only called by the DP thread. */ private[this] def processInputTuple(tuple: Tuple): Unit = { + val portIdentity: PortIdentity = + this.inputGateway.getChannel(inputManager.currentChannelId).getPortId + if (isPortPoisoned(portIdentity)) { + // the attempt this data belongs to has already failed: consume and + // discard without invoking the executor + statisticsManager.increaseInputStatistics(portIdentity, tuple.inMemSize) + return + } try { - val portIdentity: PortIdentity = - this.inputGateway.getChannel(inputManager.currentChannelId).getPortId outputManager.outputIterator.setTupleOutput( executor.processTupleMultiPort( tuple, @@ -116,19 +142,19 @@ class DataProcessor( } catch safely { case e => - // forward input tuple to the user and pause DP thread + // report the error and fail this attempt in-band handleExecutorException(e) } } private[this] def processInputState( state: State, - port: Int, + portId: PortIdentity, loopCounter: Long, loopStartId: String ): Unit = { try { - val outputState = executor.processState(state, port) + val outputState = executor.processState(state, portId.id) if (outputState.isDefined) { // Carry the incoming loop envelope through unchanged: loop operators // are Python-only, so a JVM operator inside a loop body only ever @@ -139,6 +165,13 @@ class DataProcessor( } catch safely { case e => handleExecutorException(e) + } finally { + // poison AFTER delivery: the executor sees the error State (frame + // operators react to it; the default pass-through forwards it), then + // the port drains from the next data tuple on + if (State.isError(state)) { + poisonedPorts.add(portId) + } } } @@ -231,7 +264,7 @@ class DataProcessor( inputManager.initBatch(channelId, tuples) processInputTuple(inputManager.getNextTuple) case StateFrame(state, loopCounter, loopStartId) => - processInputState(state, portId.id, loopCounter, loopStartId) + processInputState(state, portId, loopCounter, loopStartId) } statisticsManager.increaseDataProcessingTime(System.nanoTime() - dataProcessingStartTime) } @@ -315,7 +348,22 @@ class DataProcessor( asyncRPCClient.mkContext(COORDINATOR) ) logger.warn(e.getLocalizedMessage + "\n" + e.getStackTrace.mkString("\n")) - // invoke a pause in-place - pauseManager.pause(OperatorLogicPause) + if (!guarded) { + // Outside any try/catch frame, keep the default product behavior: + // report and pause, leaving the current input retriable. + pauseManager.pause(OperatorLogicPause) + return + } + // Inside a frame, failure becomes a dataflow event: broadcast an error + // State on all output ports so downstream workers drain and try/catch + // frames can react, then drain this worker's own remaining input. Ports + // still complete normally — no stall, no kill. + if (!selfFailed) { + selfFailed = true + val opId = VirtualIdentityUtils.getPhysicalOpId(actorId) + outputManager.emitState( + State.errorState(s"${opId.logicalOpId.id}/${opId.layerName}", actorId.name, e) + ) + } } } diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/EndChannelHandler.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/EndChannelHandler.scala index 7794342690b..2740a7f282d 100644 --- a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/EndChannelHandler.scala +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/EndChannelHandler.scala @@ -40,18 +40,23 @@ trait EndChannelHandler { val portId = dp.inputGateway.getChannel(channelId).getPortId dp.inputManager.getPort(portId).completed = true dp.inputManager.initBatch(channelId, Array.empty) - try { - val outputState = dp.executor.produceStateOnFinish(portId.id) - if (outputState.isDefined) { - dp.outputManager.emitState(outputState.get) + // A poisoned port belongs to a failed attempt: suppress the executor's + // finish hooks (no post-failure side effects, no junk final emissions). + // Port completion below still runs so the stream terminates normally. + if (!dp.isPortPoisoned(portId)) { + try { + val outputState = dp.executor.produceStateOnFinish(portId.id) + if (outputState.isDefined) { + dp.outputManager.emitState(outputState.get) + } + dp.outputManager.outputIterator.setTupleOutput( + dp.executor.onFinishMultiPort(portId.id) + ) + } catch safely { + case e => + // report the error and fail this attempt in-band + dp.handleExecutorException(e) } - dp.outputManager.outputIterator.setTupleOutput( - dp.executor.onFinishMultiPort(portId.id) - ) - } catch safely { - case e => - // forward input tuple to the user and pause DP thread - dp.handleExecutorException(e) } dp.outputManager.outputIterator.appendSpecialTupleToEnd( diff --git a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/InitializeExecutorHandler.scala b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/InitializeExecutorHandler.scala index 969b466a1b2..41137c46eef 100644 --- a/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/InitializeExecutorHandler.scala +++ b/amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/promisehandlers/InitializeExecutorHandler.scala @@ -44,6 +44,7 @@ trait InitializeExecutorHandler { ) ) cachedTotalWorkerCount = req.totalWorkerCount + dp.guarded = req.guarded setupExecutor(req.opExecInitInfo, workerIdx, cachedTotalWorkerCount) EmptyReturn() } diff --git a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala new file mode 100644 index 00000000000..7f958c0dc99 --- /dev/null +++ b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala @@ -0,0 +1,414 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.engine.e2e + +import com.twitter.util.Duration +import org.apache.pekko.actor.{ActorSystem, Props} +import org.apache.pekko.testkit.{ImplicitSender, TestKit} +import org.apache.pekko.util.Timeout +import org.apache.texera.amber.clustering.SingleNodeListener +import org.apache.texera.amber.core.storage.DocumentFactory +import org.apache.texera.amber.core.storage.model.VirtualDocument +import org.apache.texera.amber.core.tuple.Tuple +import org.apache.texera.amber.core.virtualidentity.{ExecutionIdentity, OperatorIdentity} +import org.apache.texera.amber.core.workflow.PortIdentity +import org.apache.texera.amber.engine.common.AmberRuntime +import org.apache.texera.amber.engine.e2e.TestUtils.{ + buildWorkflow, + cleanupWorkflowExecutionData, + initiateTexeraDBForTestCases, + runWorkflowAndReadResults, + setUpWorkflowExecutionData, + workflowContext +} +import org.apache.texera.amber.operator.LogicalOp +import org.apache.texera.amber.operator.filter.{ + ComparisonType, + FilterPredicate, + SpecializedFilterOpDesc +} +import org.apache.texera.amber.operator.limit.LimitOpDesc +import org.apache.texera.amber.operator.source.scan.text.TextInputSourceOpDesc +import org.apache.texera.amber.operator.trycatch.{FinallyOpDesc, TryCatchOpDesc} +import org.apache.texera.amber.operator.udf.python.PythonUDFOpDescV2 +import org.apache.texera.amber.tags.IntegrationTest +import org.apache.texera.common.compiler.model.LogicalLink +import org.apache.texera.web.resource.dashboard.user.workflow.WorkflowExecutionsResource.getResultUriByLogicalPortId +import org.scalatest.flatspec.AnyFlatSpecLike +import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach, Outcome, Retries} + +import scala.concurrent.duration.DurationInt + +/** + * End-to-end try/catch frame tests: run real + * TextInput -> TryCatch -> {try, catch} -> Finally workflows through the + * engine and assert the Finally's materialized results — the all-or-nothing + * contract means exactly one branch's complete output comes out, and it comes + * out of the port named for the winner (`Try Result` / `Catch Result`). + * + * The failing operator is a Filter whose predicate references a nonexistent + * attribute: it compiles cleanly (Filter does no compile-time predicate + * validation) and throws on the first tuple at runtime, exercising the whole + * failure path — error State emission, per-port drain, signal edges into the + * CatchGate, snapshot replay through the catch subgraph, and the Merger's + * release decision. + * + * All operators are JVM-based, so no Python workers are involved. + */ +@IntegrationTest +class TryCatchIntegrationSpec + extends TestKit(ActorSystem("TryCatchIntegrationSpec", AmberRuntime.pekkoConfig)) + with ImplicitSender + with AnyFlatSpecLike + with BeforeAndAfterAll + with BeforeAndAfterEach + with Retries { + + override def withFixture(test: NoArgTest): Outcome = + withRetry { super.withFixture(test) } + + implicit val timeout: Timeout = Timeout(5.seconds) + + // Unique per-suite id (1-6 are taken by the other e2e/integration suites). + private val specId = 7 + + override protected def beforeEach(): Unit = setUpWorkflowExecutionData(specId) + + override protected def afterEach(): Unit = cleanupWorkflowExecutionData(specId) + + override def beforeAll(): Unit = { + system.actorOf(Props[SingleNodeListener](), "cluster-info") + Class.forName("org.postgresql.Driver") + initiateTexeraDBForTestCases() + } + + override def afterAll(): Unit = { + TestKit.shutdownActorSystem(system) + } + + private def runAndGetMaterializedRowCounts( + operators: List[LogicalOp], + links: List[LogicalLink] + ): Map[OperatorIdentity, Long] = + runWorkflowAndReadResults( + system, + buildWorkflow(operators, links, workflowContext(specId)), + operators.map(_.operatorIdentifier), + _.getCount, + Duration.fromSeconds(90) + ) + + /** + * Row count of one specific materialized output port, read after the run + * completed (the harness's map only covers port 0). Finally routes the + * winner by port — `Try Result` (0) on success, `Catch Result` (1) on + * failure — so the frame tests assert both ports' counts. + */ + private def portRowCount(op: LogicalOp, port: PortIdentity): Long = + getResultUriByLogicalPortId(ExecutionIdentity(specId.toLong), op.operatorIdentifier, port) + .map(uri => + DocumentFactory.openDocument(uri)._1.asInstanceOf[VirtualDocument[Tuple]].getCount + ) + .getOrElse(fail(s"no materialized result for '${op.operatorIdentifier.id}' port ${port.id}")) + + private def textInput(text: String): TextInputSourceOpDesc = { + val op = new TextInputSourceOpDesc() + op.textInput = text + op + } + + private def limit(n: Int): LimitOpDesc = { + val op = new LimitOpDesc() + op.limit = n + op + } + + /** a filter that compiles cleanly but throws on the first tuple at runtime */ + private def failingFilter(): SpecializedFilterOpDesc = { + val op = new SpecializedFilterOpDesc() + op.predicates = List(new FilterPredicate("no_such_attribute", ComparisonType.EQUAL_TO, "x")) + op + } + + private def link( + from: LogicalOp, + fromPort: PortIdentity, + to: LogicalOp, + toPort: PortIdentity + ): LogicalLink = + LogicalLink(from.operatorIdentifier, fromPort, to.operatorIdentifier, toPort) + + private val port0 = PortIdentity() + private val port1 = PortIdentity(1) + + /** src -> TryCatch; Try -> tryBranch -> Finally.FromTry; Catch -> catchBranch -> Finally.FromCatch */ + private def frameWorkflow( + tryBranch: LogicalOp, + catchBranch: LogicalOp + ): (List[LogicalOp], List[LogicalLink], FinallyOpDesc) = { + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val fin = new FinallyOpDesc() + val operators = List(src, tryCatch, tryBranch, catchBranch, fin) + val links = List( + link(src, port0, tryCatch, port0), + link(tryCatch, port0, tryBranch, port0), // Try + link(tryCatch, port1, catchBranch, port0), // Catch + link(tryBranch, port0, fin, port0), // From Try + link(catchBranch, port0, fin, port1) // From Catch + ) + (operators, links, fin) + } + + "Engine" should "emit the try branch's results through Finally when the attempt succeeds" in { + val (operators, links, fin) = frameWorkflow(tryBranch = limit(3), catchBranch = limit(2)) + val materialized = runAndGetMaterializedRowCounts(operators, links) + // try side saw all 3 rows and succeeded => Finally emits exactly those, + // out of Try Result (port 0) — the harness map reads port 0; the catch + // subgraph ran empty (gate dropped the snapshot) and Catch Result is empty + assert(materialized(fin.operatorIdentifier) == 3) + assert(portRowCount(fin, port1) == 0) + } + + it should "replay the input through the catch branch when the try branch fails" in { + val (operators, links, fin) = frameWorkflow( + tryBranch = failingFilter(), + catchBranch = limit(2) + ) + val materialized = runAndGetMaterializedRowCounts(operators, links) + // the failing filter poisons the try side; the gate releases the snapshot + // (all 3 input rows) into the catch branch, whose limit(2) passes 2 rows; + // the Merger flushes the catch side only — never a mix — and routes it out + // of Catch Result (port 1), leaving Try Result (port 0) empty + assert(materialized(fin.operatorIdentifier) == 0) + assert(portRowCount(fin, port1) == 2) + } + + it should "continue downstream from BOTH result ports (the loser's subgraph completes empty)" in { + // The seal on the split: each result port is a real continuation point. + // Downstream of the loser port must run on zero rows and complete (the + // If-operator untaken-branch pattern), never hang; downstream of the + // winner sees the full release. Failure path: winner = Catch Result. + val (operators, links, fin) = frameWorkflow( + tryBranch = failingFilter(), + catchBranch = limit(2) + ) + val onSuccess = limit(10) // downstream of Try Result: must complete empty + val onRecovery = limit(10) // downstream of Catch Result: gets the release + val materialized = runAndGetMaterializedRowCounts( + operators ++ List(onSuccess, onRecovery), + links ++ List( + link(fin, port0, onSuccess, port0), + link(fin, port1, onRecovery, port0) + ) + ) + assert(materialized(onSuccess.operatorIdentifier) == 0) + assert(materialized(onRecovery.operatorIdentifier) == 2) + } + + it should "complete a frame whose Catch port is unconnected when the attempt succeeds" in { + // A frame without a catch subgraph (and without a Finally) is legal: it + // guards a pipeline that ends in its own result table. On success the + // guarded tail materializes normally. + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val tail = limit(3) + val materialized = runAndGetMaterializedRowCounts( + List(src, tryCatch, tail), + List(link(src, port0, tryCatch, port0), link(tryCatch, port0, tail, port0)) + ) + assert(materialized(tail.operatorIdentifier) == 3) + } + + it should "drain and terminate (not hang) when a guarded operator fails with no catch wired" in { + // A frame need not have a catch subgraph to be useful: an error inside it + // becomes an in-band error State and the execution terminates — the + // guarded tail produces nothing and the run completes (an unguarded + // failure would pause the worker instead). + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val boom = failingFilter() + val materialized = runAndGetMaterializedRowCounts( + List(src, tryCatch, boom), + List(link(src, port0, tryCatch, port0), link(tryCatch, port0, boom, port0)) + ) + assert(materialized(boom.operatorIdentifier) == 0) + } + + it should "handle an inner frame's failure without disturbing the outer frame" in { + // outer.Try -> inner TryCatch -> {inner try fails, inner catch recovers} + // -> inner Finally -> outer Finally.FromTry + // The inner frame catches its own failure, so the outer frame must see a + // clean try side and emit the inner Finally's (catch-side) results. + val src = textInput("1\n2\n3") + val outer = new TryCatchOpDesc() + val inner = new TryCatchOpDesc() + val innerTry = failingFilter() + val innerCatch = limit(2) + val innerFin = new FinallyOpDesc() + val outerCatch = limit(1) + val outerFin = new FinallyOpDesc() + + val operators = + List(src, outer, inner, innerTry, innerCatch, innerFin, outerCatch, outerFin) + val links = List( + link(src, port0, outer, port0), + link(outer, port0, inner, port0), // outer Try -> inner frame + link(inner, port0, innerTry, port0), // inner Try + link(inner, port1, innerCatch, port0), // inner Catch + link(innerTry, port0, innerFin, port0), + link(innerCatch, port0, innerFin, port1), + // outer From Try: union BOTH inner result ports — "whatever the inner + // frame produced continues", whichever side won + link(innerFin, port0, outerFin, port0), + link(innerFin, port1, outerFin, port0), + link(outer, port1, outerCatch, port0), // outer Catch + link(outerCatch, port0, outerFin, port1) // outer From Catch + ) + val materialized = runAndGetMaterializedRowCounts(operators, links) + // inner catch recovered 2 rows (inner Catch Result); the outer frame's try + // side is clean, so the outer Finally emits those 2 rows out its own + // Try Result (NOT the outer catch's 1 row) + assert(materialized(outerFin.operatorIdentifier) == 2) + assert(portRowCount(outerFin, port1) == 0) + } + + it should "escalate to the outer catch when both inner branches fail" in { + // Same shape as above, but the inner CATCH branch also fails: the inner + // frame cannot recover, so the failure escalates and the outer catch runs. + val src = textInput("1\n2\n3") + val outer = new TryCatchOpDesc() + val inner = new TryCatchOpDesc() + val innerTry = failingFilter() + val innerCatch = failingFilter() + val innerFin = new FinallyOpDesc() + val outerCatch = limit(1) + val outerFin = new FinallyOpDesc() + + val operators = + List(src, outer, inner, innerTry, innerCatch, innerFin, outerCatch, outerFin) + val links = List( + link(src, port0, outer, port0), + link(outer, port0, inner, port0), + link(inner, port0, innerTry, port0), + link(inner, port1, innerCatch, port0), + link(innerTry, port0, innerFin, port0), + link(innerCatch, port0, innerFin, port1), + link(innerFin, port0, outerFin, port0), + link(innerFin, port1, outerFin, port0), + link(outer, port1, outerCatch, port0), + link(outerCatch, port0, outerFin, port1) + ) + val materialized = runAndGetMaterializedRowCounts(operators, links) + // double inner failure => the outer catch's limit(1) result wins, and the + // recovery is visible as rows on the outer Catch Result port + assert(materialized(outerFin.operatorIdentifier) == 0) + assert(portRowCount(outerFin, port1) == 1) + } + + it should "route the catch branch by error type: catch(SpecificError) via Error Info + If" in { + // Simulates `catch (SpecificError e)`: a classifier UDF consumes the + // frame's Error Info rows and emits a boolean State; an If routes the + // Catch replay to the specific handler when the error matches, and to the + // generic handler otherwise. The failing filter's message mentions the + // missing attribute, so the specific branch (limit 2) must win over the + // generic one (limit 1). + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val classifier = new PythonUDFOpDescV2() + classifier.code = """ +from pytexera import * + +class ProcessTupleOperator(UDFOperatorV2): + matched = False + + @overrides + def process_tuple(self, tuple_: Tuple, port: int) -> Iterator[Optional[TupleLike]]: + if "no_such_attribute" in str(tuple_["message"]): + self.matched = True + yield + + def produce_state_on_finish(self, port: int): + return {"isSpecific": self.matched} +""" + classifier.workers = 1 + val ifOp = new org.apache.texera.amber.operator.ifStatement.IfOpDesc() + ifOp.conditionName = "isSpecific" + val specificHandler = limit(2) // runs on isSpecific == true (If port 1) + val genericHandler = limit(1) // runs otherwise (If port 0) + val fin = new FinallyOpDesc() + val failing = failingFilter() + + val operators = + List(src, tryCatch, failing, classifier, ifOp, specificHandler, genericHandler, fin) + val links = List( + link(src, port0, tryCatch, port0), + link(tryCatch, port0, failing, port0), // Try: fails on first tuple + link(failing, port0, fin, port0), // From Try (poisoned, discarded) + link(tryCatch, PortIdentity(2), classifier, port0), // Error Info + link(classifier, port0, ifOp, port0), // If condition (dependee) + link(tryCatch, port1, ifOp, port1), // If data = Catch replay + link(ifOp, port1, specificHandler, port0), // True branch + link(ifOp, port0, genericHandler, port0), // False branch + link(specificHandler, port0, fin, port1), // From Catch (union) + link(genericHandler, port0, fin, port1) + ) + val materialized = runAndGetMaterializedRowCounts(operators, links) + // specific handler passed 2 of the 3 replayed rows; generic saw none; + // the recovery leaves Finally through Catch Result (port 1) + assert(materialized(fin.operatorIdentifier) == 0) + assert(portRowCount(fin, port1) == 2) + } + + it should "guard a Python UDF failure and fall back to the catch branch" in { + // The common real-world case: user code in a Python UDF raises. This + // exercises the pyamber side of the design (error State emission + + // per-port drain in MainLoop) end to end. + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val udf = new PythonUDFOpDescV2() + // pass the input schema through, so the Finally's two branches match + udf.retainInputColumns = true + udf.code = """ +from pytexera import * + +class ProcessTupleOperator(UDFOperatorV2): + @overrides + def process_tuple(self, tuple_: Tuple, port: int) -> Iterator[Optional[TupleLike]]: + raise ValueError("boom from python udf") + yield +""" + udf.workers = 1 + val catchBranch = limit(2) + val fin = new FinallyOpDesc() + val materialized = runAndGetMaterializedRowCounts( + List(src, tryCatch, udf, catchBranch, fin), + List( + link(src, port0, tryCatch, port0), + link(tryCatch, port0, udf, port0), + link(tryCatch, port1, catchBranch, port0), + link(udf, port0, fin, port0), + link(catchBranch, port0, fin, port1) + ) + ) + assert(materialized(fin.operatorIdentifier) == 0) + assert(portRowCount(fin, port1) == 2) + } +} diff --git a/amber/src/test/python/core/runnables/test_data_processor.py b/amber/src/test/python/core/runnables/test_data_processor.py index 61482363a15..80c1ffdb575 100644 --- a/amber/src/test/python/core/runnables/test_data_processor.py +++ b/amber/src/test/python/core/runnables/test_data_processor.py @@ -154,6 +154,43 @@ def capturing_switch(): # Exit always switches back to MainLoop, even on the failure path. assert data_processor.switch_calls == 1 + @pytest.mark.timeout(2) + def test_guarded_failure_finishes_the_cycle_before_the_switch( + self, context, data_processor + ): + # Drain semantics (inside a try/catch frame): the failed cycle must be + # marked finished BEFORE the final switch, so MainLoop's cycle loop + # ends on its first check and never wakes this thread again without + # queuing an input — run()'s slot invariant would kill the thread and + # MainLoop's next switch would wait on it forever. + context.guarded = True + seen_at_switch = [] + + def capturing_switch(): + seen_at_switch.append( + context.tuple_processing_manager.finished_current.is_set() + ) + data_processor.switch_calls += 1 + + data_processor._switch_context = capturing_switch + + with data_processor._executor_session(): + raise RuntimeError("boom-guarded") + + assert seen_at_switch == [True] + + @pytest.mark.timeout(2) + def test_unguarded_failure_leaves_the_cycle_open_for_retry( + self, context, data_processor + ): + # Pre-frame behavior (no try/catch anywhere): MainLoop pauses on its + # side of the switch and the current input stays retriable + # (RetryCurrentTuple), so the failed cycle must NOT be force-finished. + with data_processor._executor_session(): + raise RuntimeError("boom-unguarded") + + assert not context.tuple_processing_manager.finished_current.is_set() + @pytest.mark.timeout(2) def test_clean_session_does_not_record_an_exception(self, context, data_processor): with data_processor._executor_session(): diff --git a/amber/src/test/python/core/runnables/test_main_loop.py b/amber/src/test/python/core/runnables/test_main_loop.py index 73f3957559b..40733cf9dea 100644 --- a/amber/src/test/python/core/runnables/test_main_loop.py +++ b/amber/src/test/python/core/runnables/test_main_loop.py @@ -28,6 +28,7 @@ from core.models import ( DataFrame, InternalQueue, + Schema, State, StateFrame, Tuple, @@ -2082,17 +2083,17 @@ def test_empty_on_finish_after_tuples_completes_worker( reraise() @pytest.mark.timeout(2) - def test_console_message_rpc_fires_before_exception_pause( - self, main_loop, monkeypatch - ): - # Pin the coordinator-facing contract: when DataProcessor raises - # during an executor call, the stack-trace ConsoleMessage must - # reach the coordinator *before* the worker enters EXCEPTION_PAUSE - # — otherwise the UI sees a paused worker with no error to show - # until the user resumes. The DataProcessor side queues the - # message before the switch (covered by - # test_data_processor.TestExecutorSession); this test pins the - # MainLoop side: post-switch hook flushes RPCs first, pauses last. + def test_console_message_rpc_fires_before_error_state(self, main_loop, monkeypatch): + # Pin the coordinator-facing contract: when DataProcessor raises during + # an executor call, the stack-trace ConsoleMessage must reach the + # coordinator *before* the failure travels on as an in-band error State + # — otherwise the UI could see a drained/completed operator with no + # error to show. The DataProcessor side queues the message before the + # switch (covered by test_data_processor.TestExecutorSession); this test + # pins the MainLoop side: post-switch hook flushes RPCs first, emits the + # error State last, and does NOT pause (a GUARDED operator — inside a + # try/catch frame — drains rather than stalling). + main_loop.context.guarded = True events = [] monkeypatch.setattr( @@ -2105,6 +2106,11 @@ def test_console_message_rpc_fires_before_exception_pause( "pause", lambda pause_type, change_state=True: events.append(("pause", pause_type)), ) + monkeypatch.setattr( + main_loop, + "_emit_batches", + lambda batches: events.append(("emit", list(batches))), + ) try: raise RuntimeError("boom-from-executor") @@ -2125,13 +2131,109 @@ def test_console_message_rpc_fires_before_exception_pause( main_loop._post_switch_context_checks() kinds = [e[0] for e in events] - assert kinds == ["rpc", "pause"], ( - "console message must reach coordinator before pause; " - f"observed order: {kinds}" + assert kinds == ["rpc", "emit"], ( + "console message must reach coordinator before the error State, " + f"and the worker must not pause; observed order: {kinds}" ) assert events[0][1].msg_type == ConsoleMessageType.ERROR assert "boom-from-executor" in events[0][1].title - assert events[1][1] is PauseType.EXCEPTION_PAUSE + assert main_loop._self_failed + + @pytest.mark.timeout(2) + def test_unguarded_failure_pauses_like_before_frames(self, main_loop, monkeypatch): + # An operator OUTSIDE any try/catch frame keeps the default product + # behavior on failure: console error reaches the coordinator, then the + # worker enters EXCEPTION_PAUSE — inspectable, current input retriable + # — and NO error State travels, nothing drains. + events = [] + monkeypatch.setattr( + main_loop, + "_send_console_message", + lambda msg: events.append(("rpc", msg)), + ) + monkeypatch.setattr( + main_loop.context.pause_manager, + "pause", + lambda pause_type, change_state=True: events.append(("pause", pause_type)), + ) + monkeypatch.setattr( + main_loop, + "_emit_batches", + lambda batches: events.append(("emit", list(batches))), + ) + + try: + raise RuntimeError("boom-unframed") + except RuntimeError: + exc_info = sys.exc_info() + main_loop.context.exception_manager.set_exception_info(exc_info) + main_loop.context.console_message_manager.put_message( + ConsoleMessage( + worker_id="dummy_worker_id", + timestamp=current_time_in_local_timezone(), + msg_type=ConsoleMessageType.ERROR, + source="test:_capture_exc_info:0", + title="RuntimeError: boom-unframed", + message="RuntimeError: boom-unframed", + ) + ) + + main_loop._check_exception() + + kinds = [e[0] for e in events] + assert kinds == ["rpc", "pause"], ( + "unguarded failure must report the console error and then pause — " + f"no error State; observed order: {kinds}" + ) + assert events[1][1] == PauseType.EXCEPTION_PAUSE + assert not main_loop._self_failed + assert not main_loop._is_port_poisoned(PortIdentity(id=0)) + + @pytest.mark.timeout(2) + def test_failed_worker_drains_instead_of_invoking_the_executor( + self, main_loop, monkeypatch + ): + # After its own failure, a GUARDED worker must consume remaining input + # without calling the executor again — that is what keeps side-effecting + # UDFs from running on a doomed attempt, and what lets the stream + # terminate instead of hanging. + main_loop.context.guarded = True + monkeypatch.setattr(main_loop, "_send_console_message", lambda msg: None) + monkeypatch.setattr(main_loop, "_emit_batches", lambda batches: None) + invoked = [] + monkeypatch.setattr( + main_loop, "process_input_tuple", lambda: invoked.append("tuple") + ) + + try: + raise RuntimeError("boom") + except RuntimeError: + exc_info = sys.exc_info() + main_loop.context.exception_manager.set_exception_info(exc_info) + main_loop._check_exception() + assert main_loop._self_failed + + main_loop._process_tuple(Tuple({"test-1": 10})) + assert invoked == [], "executor must not be invoked on a drained port" + + @pytest.mark.timeout(2) + def test_incoming_error_state_poisons_only_its_own_port( + self, main_loop, monkeypatch + ): + # Per-port drain contagion: an error State arriving on one input port + # drains that port; other ports keep working (a Finally's From Catch + # side must survive its From Try side failing). + monkeypatch.setattr(main_loop, "_send_console_message", lambda msg: None) + error_state = State.error("someop/main", "worker-9", RuntimeError("upstream")) + port_a = PortIdentity(id=0) + port_b = PortIdentity(id=1) + + main_loop._poisoned_ports.add(port_a) + + assert main_loop._is_port_poisoned(port_a) + assert not main_loop._is_port_poisoned(port_b) + assert error_state.is_error() + assert error_state.error_operator_id() == "someop/main" @pytest.mark.timeout(2) def test_complete_reports_loopend_condition_error_instead_of_crashing( @@ -2143,8 +2245,8 @@ def test_complete_reports_loopend_condition_error_instead_of_crashing( # session. A typo or undefined name in the condition would otherwise # propagate through run()'s @logger.catch(reraise=True) and kill the # worker thread silently. The guard must report it like a UDF error - # (record on the exception manager + ERROR console message + - # EXCEPTION_PAUSE) and skip both the loop-back edge and completion. + # (record on the exception manager + ERROR console message + in-band + # error State) and skip the loop-back edge and completion. class _BoomLoopEnd(LoopEndOperator): def __init__(self): super().__init__() @@ -2158,6 +2260,7 @@ def close(self): executor = _BoomLoopEnd() main_loop.context.executor_manager.executor = executor + main_loop.context.guarded = True # loop bodies run inside frames' semantics console_msgs = [] pauses = [] @@ -2173,6 +2276,7 @@ def close(self): monkeypatch.setattr( main_loop, "_jump_to_loop_start", lambda *args: jumped.append(True) ) + monkeypatch.setattr(main_loop, "_emit_batches", lambda batches: None) # Must not raise: a bad condition is reported, not propagated. main_loop.complete() @@ -2180,7 +2284,10 @@ def close(self): assert jumped == [], "must not take the loop-back edge on a failed condition" assert not executor.closed, "must return before completing the worker" assert main_loop.context.exception_manager.has_exception() - assert pauses == [PauseType.EXCEPTION_PAUSE] + # Failure travels as an in-band error State (guarded worker); the + # worker drains rather than stalling in EXCEPTION_PAUSE. + assert pauses == [] + assert main_loop._self_failed error_msgs = [m for m in console_msgs if m.msg_type == ConsoleMessageType.ERROR] assert len(error_msgs) == 1 assert "ValueError" in error_msgs[0].title @@ -2194,8 +2301,8 @@ def test_complete_reports_loopback_write_error_instead_of_crashing( # write in _jump_to_loop_start runs after the jump DCM, on the main # loop thread, outside DataProcessor's guarded executor session. A # put_one/close failure must be reported the same way as a condition - # error (exception manager + ERROR console message + EXCEPTION_PAUSE) - # and skip completion, not propagate and kill the worker thread. + # error (exception manager + ERROR console message + in-band error + # State) and skip completion, not propagate and kill the worker thread. class _JumpingLoopEnd(LoopEndOperator): def __init__(self): super().__init__() @@ -2210,6 +2317,7 @@ def close(self): executor = _JumpingLoopEnd() executor.state = State({"i": 1}) main_loop.context.executor_manager.executor = executor + main_loop.context.guarded = True main_loop._loop_start_id = "loop-start-1" main_loop.context.loop_start_state_uris = {"loop-start-1": "vfs:///x/state"} @@ -2239,13 +2347,15 @@ def writer(self, name): "core.runnables.main_loop.DocumentFactory.create_document", lambda uri, schema: _Doc(), ) + monkeypatch.setattr(main_loop, "_emit_batches", lambda batches: None) # Must not raise: a failed back-edge write is reported, not propagated. main_loop.complete() assert not executor.closed, "must return before completing the worker" assert main_loop.context.exception_manager.has_exception() - assert pauses == [PauseType.EXCEPTION_PAUSE] + assert pauses == [] + assert main_loop._self_failed error_msgs = [m for m in console_msgs if m.msg_type == ConsoleMessageType.ERROR] assert len(error_msgs) == 1 assert "iceberg commit failed" in error_msgs[0].title @@ -2261,7 +2371,8 @@ def test_emit_and_save_state_reports_error_instead_of_killing_thread( # propagates through run()'s @logger.catch(reraise=True) and kills the # thread, hanging the workflow with no operator-facing error. It must be # reported like a UDF error (exception manager + ERROR console message + - # EXCEPTION_PAUSE) instead. + # in-band error State) instead. + main_loop.context.guarded = True console_msgs = [] pauses = [] monkeypatch.setattr( @@ -2289,20 +2400,25 @@ def _boom(*args, **kwargs): main_loop._emit_and_save_state(State({"weights": 1}), 0, "") assert main_loop.context.exception_manager.has_exception() - assert pauses == [PauseType.EXCEPTION_PAUSE] + assert pauses == [] + assert main_loop._self_failed error_msgs = [m for m in console_msgs if m.msg_type == ConsoleMessageType.ERROR] assert len(error_msgs) == 1 assert "not JSON serializable" in error_msgs[0].title @pytest.mark.timeout(2) - def test_end_channel_holds_region_when_state_emit_fails( + def test_end_channel_completes_ports_after_a_failure_instead_of_hanging( self, main_loop, monkeypatch ): - # When a state-emission error is reported during _process_end_channel, - # the worker must NOT go on to send port_completed / complete(): those - # RPCs would let the coordinator mark the region complete despite the - # reported error (port-based region completion). The guard holds the - # region so the reported error is not a false success. + # Superseded behavior: this used to HOLD the region (skip + # port_completed/complete) after a reported state-emission error, so an + # operator failure stalled the workflow forever with no terminal state. + # Failure is now an in-band dataflow event: the error State went + # downstream (any enclosing try/catch frame reacts, and the console + # error is already reported), so the worker must still complete its + # ports and finish — the stream terminates instead of hanging, and the + # execution ends as failed-with-errors rather than never ending. + main_loop.context.guarded = True completed = [] port_completed_calls = [] @@ -2317,6 +2433,7 @@ def _boom_process_input_state(*args, **kwargs): monkeypatch.setattr(main_loop, "process_input_state", _boom_process_input_state) monkeypatch.setattr(main_loop, "process_input_tuple", lambda: None) monkeypatch.setattr(main_loop, "complete", lambda: completed.append(True)) + monkeypatch.setattr(main_loop, "_emit_batches", lambda batches: None) class _Coordinator: def port_completed(self, request): @@ -2332,12 +2449,24 @@ def port_completed(self, request): ) monkeypatch.setattr(main_loop, "_send_console_message", lambda msg: None) + # register the channel so the end-channel path resolves a real port id + channel_id = ChannelIdentity( + ActorVirtualIdentity("upstream"), + ActorVirtualIdentity("dummy_worker_id"), + False, + ) + main_loop.context.current_input_channel_id = channel_id + main_loop.context.input_manager.add_input_port( + PortIdentity(id=0), Schema(raw_schema={"test-1": "INTEGER"}), [], [] + ) + main_loop.context.input_manager.register_input(channel_id, PortIdentity(id=0)) + main_loop._process_end_channel() - assert port_completed_calls == [], ( - "must not complete ports after a reported error" + assert main_loop.context.exception_manager.has_exception() + assert port_completed_calls != [], ( + "ports must still complete after a failure so the stream terminates" ) - assert completed == [], "must not complete the worker after a reported error" # -- Loop counter is runtime-owned (relocated from test_loop_operators) --- # diff --git a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala index 53f167c4eaf..5074e3fc882 100644 --- a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala +++ b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala @@ -374,8 +374,15 @@ class DataProcessorSpec extends AnyFlatSpec with MockFactory with Matchers with "data processor" should "handle an exception thrown while processing a state frame" in { val dp = mkDataProcessor dp.executor = executor + dp.guarded = true // inside a try/catch frame: failure drains, not pauses dp.stateManager.transitTo(READY) - (outputHandler.apply _).expects(*).anyNumberOfTimes() + val emitted = scala.collection.mutable.ArrayBuffer[WorkflowFIFOMessage]() + (outputHandler.apply _) + .expects(*) + .onCall { (m: Either[MainThreadDelegateMessage, WorkflowFIFOMessage]) => + m.foreach(emitted += _); () + } + .anyNumberOfTimes() val inputState = State(Map("field1" -> 3)) ( ( @@ -390,21 +397,43 @@ class DataProcessorSpec extends AnyFlatSpec with MockFactory with Matchers with .getChannel(ChannelIdentity(senderWorkerId, testWorkerId, isControl = false)) .setPortId(inputPortId) dp.outputManager.addPort(outputPortId, schema, None) + dp.outputManager.addPartitionerWithPartitioning( + PhysicalLink(testOpId, outputPortId, upstreamOpId, inputPortId), + OneToOnePartitioning(1, Seq(ChannelIdentity(testWorkerId, senderWorkerId, isControl = false))) + ) noException should be thrownBy { dp.processDataPayload( ChannelIdentity(senderWorkerId, testWorkerId, isControl = false), StateFrame(inputState) ) } - // handleExecutorException must engage an operator-logic pause. - dp.pauseManager.isPaused shouldBe true + // Failure is an in-band dataflow event now: no stall, an error State goes + // downstream, and this worker drains (executor never invoked again — no + // further expectations are set on the mock). + dp.pauseManager.isPaused shouldBe false + val errorFrames = emitted.map(_.payload).collect { case sf: StateFrame => sf.frame } + assert(errorFrames.exists(State.isError), s"expected an error State, got: $errorFrames") + dp.isPortPoisoned(inputPortId) shouldBe true + noException should be thrownBy { + dp.processDataPayload( + ChannelIdentity(senderWorkerId, testWorkerId, isControl = false), + DataFrame(Array(tuples.head)) + ) + } } "data processor" should "handle an exception thrown while processing an input tuple" in { val dp = mkDataProcessor dp.executor = executor + dp.guarded = true dp.stateManager.transitTo(READY) - (outputHandler.apply _).expects(*).anyNumberOfTimes() + val emitted = scala.collection.mutable.ArrayBuffer[WorkflowFIFOMessage]() + (outputHandler.apply _) + .expects(*) + .onCall { (m: Either[MainThreadDelegateMessage, WorkflowFIFOMessage]) => + m.foreach(emitted += _); () + } + .anyNumberOfTimes() ( ( tuple: Tuple, @@ -419,19 +448,76 @@ class DataProcessorSpec extends AnyFlatSpec with MockFactory with Matchers with .getChannel(ChannelIdentity(senderWorkerId, testWorkerId, isControl = false)) .setPortId(inputPortId) dp.outputManager.addPort(outputPortId, schema, None) + dp.outputManager.addPartitionerWithPartitioning( + PhysicalLink(testOpId, outputPortId, upstreamOpId, inputPortId), + OneToOnePartitioning(1, Seq(ChannelIdentity(testWorkerId, senderWorkerId, isControl = false))) + ) + noException should be thrownBy { + dp.processDataPayload( + ChannelIdentity(senderWorkerId, testWorkerId, isControl = false), + DataFrame(Array(tuples.head, tuples(1))) + ) + } + // No stall: an error State is broadcast and the worker self-drains — the + // second tuple in the batch must NOT reach the executor (the mock only + // expects the first, throwing call). + dp.pauseManager.isPaused shouldBe false + val errorFrames = emitted.map(_.payload).collect { case sf: StateFrame => sf.frame } + assert(errorFrames.exists(State.isError), s"expected an error State, got: $errorFrames") + dp.isPortPoisoned(inputPortId) shouldBe true + noException should be thrownBy { + dp.continueDataProcessing() + } + } + + "data processor" should "pause on failure when unguarded (no try/catch frame), preserving the default report-and-pause behavior" in { + // An operator OUTSIDE any try/catch frame keeps the old product behavior: + // the console error is reported, the worker pauses (Python-UDF debugging / + // retry-current-tuple flows), and NO error State travels — nothing drains. + val dp = mkDataProcessor + dp.executor = executor // guarded stays false: the default + dp.stateManager.transitTo(READY) + val emitted = scala.collection.mutable.ArrayBuffer[WorkflowFIFOMessage]() + (outputHandler.apply _) + .expects(*) + .onCall { (m: Either[MainThreadDelegateMessage, WorkflowFIFOMessage]) => + m.foreach(emitted += _); () + } + .anyNumberOfTimes() + ( + ( + tuple: Tuple, + input: Int + ) => executor.processTupleMultiPort(tuple, input) + ) + .expects(tuples.head, 0) + .throwing(new RuntimeException("boom unguarded")) + (adaptiveBatchingMonitor.startAdaptiveBatching _).expects().anyNumberOfTimes() + dp.inputManager.addPort(inputPortId, schema, List.empty, List.empty) + dp.inputGateway + .getChannel(ChannelIdentity(senderWorkerId, testWorkerId, isControl = false)) + .setPortId(inputPortId) + dp.outputManager.addPort(outputPortId, schema, None) + dp.outputManager.addPartitionerWithPartitioning( + PhysicalLink(testOpId, outputPortId, upstreamOpId, inputPortId), + OneToOnePartitioning(1, Seq(ChannelIdentity(testWorkerId, senderWorkerId, isControl = false))) + ) noException should be thrownBy { dp.processDataPayload( ChannelIdentity(senderWorkerId, testWorkerId, isControl = false), DataFrame(Array(tuples.head)) ) } - // handleExecutorException must engage an operator-logic pause. dp.pauseManager.isPaused shouldBe true + val errorFrames = emitted.map(_.payload).collect { case sf: StateFrame => sf.frame } + assert(!errorFrames.exists(State.isError), s"unguarded failure must not emit an error State") + dp.isPortPoisoned(inputPortId) shouldBe false } "data processor" should "handle an exception thrown while advancing the output iterator" in { val dp = mkDataProcessor dp.executor = executor + dp.guarded = true dp.stateManager.transitTo(READY) (outputHandler.apply _).expects(*).anyNumberOfTimes() (adaptiveBatchingMonitor.startAdaptiveBatching _).expects().anyNumberOfTimes() @@ -453,9 +539,61 @@ class DataProcessorSpec extends AnyFlatSpec with MockFactory with Matchers with noException should be thrownBy { dp.continueDataProcessing() } - // handleExecutorException must pause the operator and reset the output iterator to empty. - dp.pauseManager.isPaused shouldBe true + // No stall: the output iterator is reset and the worker drains in place. + dp.pauseManager.isPaused shouldBe false dp.outputManager.hasUnfinishedOutput shouldBe false + dp.isPortPoisoned(inputPortId) shouldBe true + } + + "data processor" should "deliver an error State to the executor, forward it, then poison the port" in { + val dp = mkDataProcessor + dp.executor = executor + dp.stateManager.transitTo(READY) + val emitted = scala.collection.mutable.ArrayBuffer[WorkflowFIFOMessage]() + (outputHandler.apply _) + .expects(*) + .onCall { (m: Either[MainThreadDelegateMessage, WorkflowFIFOMessage]) => + m.foreach(emitted += _); () + } + .anyNumberOfTimes() + val errorState = State.errorState("upstreamOp/main", "some-worker", new RuntimeException("boom")) + // the executor SEES the error State (frame operators react to it); the + // default pass-through contract forwards it downstream + ( + ( + state: State, + port: Int + ) => executor.processState(state, port) + ) + .expects(errorState, 0) + .returning(Some(errorState)) + dp.inputManager.addPort(inputPortId, schema, List.empty, List.empty) + dp.inputGateway + .getChannel(ChannelIdentity(senderWorkerId, testWorkerId, isControl = false)) + .setPortId(inputPortId) + dp.outputManager.addPort(outputPortId, schema, None) + dp.outputManager.addPartitionerWithPartitioning( + PhysicalLink(testOpId, outputPortId, upstreamOpId, inputPortId), + OneToOnePartitioning(1, Seq(ChannelIdentity(testWorkerId, senderWorkerId, isControl = false))) + ) + dp.processDataPayload( + ChannelIdentity(senderWorkerId, testWorkerId, isControl = false), + StateFrame(errorState) + ) + // forwarded downstream (drain contagion travels), and the port is poisoned + val forwarded = emitted.map(_.payload).collect { case sf: StateFrame => sf.frame } + assert(forwarded.exists(State.isError), s"expected forwarded error State, got: $forwarded") + dp.isPortPoisoned(inputPortId) shouldBe true + // data arriving on the poisoned port is discarded without invoking the + // executor (no processTupleMultiPort expectation is set on the mock) + noException should be thrownBy { + dp.processDataPayload( + ChannelIdentity(senderWorkerId, testWorkerId, isControl = false), + DataFrame(Array(tuples.head)) + ) + } + // the worker itself did not fail: no self-poison of other ports + dp.pauseManager.isPaused shouldBe false } } diff --git a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkerSpec.scala b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkerSpec.scala index 7093dbfe8ef..d429ba81eba 100644 --- a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkerSpec.scala +++ b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkerSpec.scala @@ -201,7 +201,8 @@ class WorkerSpec "org.apache.texera.amber.engine.architecture.worker.DummyOperatorExecutor" ), isSource = false, - loopStartStateUris = Map.empty + loopStartStateUris = Map.empty, + guarded = false ), AsyncRPCContext(COORDINATOR, identifier1), 4 diff --git a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkflowWorkerSpec.scala b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkflowWorkerSpec.scala index a5b31b48015..cbcf8ee5507 100644 --- a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkflowWorkerSpec.scala +++ b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/WorkflowWorkerSpec.scala @@ -119,7 +119,8 @@ class WorkflowWorkerSpec "org.apache.texera.amber.engine.architecture.worker.DummyOperatorExecutor" ), isSource = false, - loopStartStateUris = Map.empty + loopStartStateUris = Map.empty, + guarded = false ) ) diff --git a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/managers/SerializationManagerSpec.scala b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/managers/SerializationManagerSpec.scala index 30f91ddf795..a5b62cb9346 100644 --- a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/managers/SerializationManagerSpec.scala +++ b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/managers/SerializationManagerSpec.scala @@ -57,7 +57,8 @@ class SerializationManagerSpec extends AnyFlatSpec { totalWorkerCount = totalWorkers, opExecInitInfo = info, isSource = false, - loopStartStateUris = Map.empty + loopStartStateUris = Map.empty, + guarded = false ) "SerializationManager.restoreExecutorState" should diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala new file mode 100644 index 00000000000..46d08616906 --- /dev/null +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala @@ -0,0 +1,477 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.common.compiler + +import org.apache.texera.amber.core.executor.OpExecWithClassName +import org.apache.texera.amber.core.virtualidentity.{OperatorIdentity, PhysicalOpIdentity} +import org.apache.texera.amber.core.workflow._ +import org.apache.texera.amber.operator.trycatch.{ + CatchGateConfig, + FinallyMergerConfig, + TryCatchOpDesc +} +import org.apache.texera.amber.util.JSONUtils.objectMapper + +import scala.collection.mutable +import scala.collection.mutable.ArrayBuffer + +/** + * Compile-time expansion of try/catch frames. + * + * For every TryCatch frame on the physical plan, this pass: + * 1. pairs each Finally with its TryCatch (structurally, via reachability); + * 2. computes the frame's try/catch cones (inclusive of nested frames) and + * bakes them into the gate/merger executor configs for error attribution; + * 3. synthesizes signal edges — one per try-cone tail output port — into the + * gate's signal ports, with `SignalPartition` requirements so the links + * carry only States/completion (tuples dropped at the sender); + * 4. declares the gate's snapshot port dependent on all signal ports, which + * is what sequences the catch side after the try side resolves; + * 5. validates the frame wiring rules (disjoint cones, Finally provenance, + * connected ports). + * + * The pass only reads/writes the physical plan: no scheduler or coordinator + * involvement. Frames appear to the rest of compilation as ordinary operators. + */ +object TryCatchFramePass { + + private val SplitterClassName = + "org.apache.texera.amber.operator.trycatch.TrySplitterOpExec" + private val GateClassName = + "org.apache.texera.amber.operator.trycatch.CatchGateOpExec" + private val MergerClassName = + "org.apache.texera.amber.operator.trycatch.FinallyMergerOpExec" + + private val TRY_PORT = PortIdentity() + private val CATCH_PORT = PortIdentity(1) + private val ERROR_INFO_PORT = TryCatchOpDesc.ERROR_INFO_PORT + private val FROM_TRY = PortIdentity() + private val FROM_CATCH = PortIdentity(1) + + private case class Frame( + splitter: PhysicalOp, + gate: PhysicalOp, + merger: Option[PhysicalOp], + tryCone: Set[PhysicalOpIdentity], + catchCone: Set[PhysicalOpIdentity] + ) { + def logicalOpId: OperatorIdentity = splitter.id.logicalOpId + def cone: Set[PhysicalOpIdentity] = tryCone ++ catchCone + def coneIdStrings: List[String] = + cone.toList.map(id => s"${id.logicalOpId.id}/${id.layerName}").sorted + } + + def run( + plan: PhysicalPlan, + errorList: Option[ArrayBuffer[(OperatorIdentity, Throwable)]] + ): PhysicalPlan = { + val splitters = opsWithClassName(plan, SplitterClassName) + if (splitters.isEmpty) { + return plan // no frames: zero-cost for ordinary workflows + } + try { + expandFrames(plan) + } catch { + case err: Throwable => + errorList match { + case Some(list) => + // attribute to the first frame's logical operator for UI display + list.append((splitters.head.id.logicalOpId, err)) + plan + case None => throw err + } + } + } + + private def opsWithClassName(plan: PhysicalPlan, className: String): List[PhysicalOp] = + plan.operators.toList + .filter(op => + op.opExecInitInfo match { + case OpExecWithClassName(cls, _) => cls == className + case _ => false + } + ) + .sortBy(_.id.logicalOpId.id) + + private def expandFrames(plan: PhysicalPlan): PhysicalPlan = { + val splitters = opsWithClassName(plan, SplitterClassName) + val mergers = opsWithClassName(plan, MergerClassName) + + // ---- 1. pairing: Finally F belongs to TryCatch T iff F's From Try is fed + // from T's try side and F's From Catch from T's catch side. The pairing + // traversal does not cross frame-internal (snapshot) links, which is what + // makes the pairing unique under nesting. + val gates = splitters.map(s => s.id.logicalOpId -> gateOf(plan, s)).toMap + + val mergerToFrame: Map[PhysicalOpIdentity, OperatorIdentity] = mergers.flatMap { merger => + val fromTrySources = sourcesInto(plan, merger.id, FROM_TRY) + val fromCatchSources = sourcesInto(plan, merger.id, FROM_CATCH) + val candidates = splitters.filter { s => + val tryReach = reach(plan, s.id, TRY_PORT, crossInternal = false, stopAt = Set.empty) + val catchReach = + reach(plan, gates(s.id.logicalOpId).id, CATCH_PORT, crossInternal = false, Set.empty) + fromTrySources.exists(src => tryReach.contains(src) || src == s.id) && + fromCatchSources.exists(src => catchReach.contains(src)) + } + candidates match { + case single :: Nil => Some(merger.id -> single.id.logicalOpId) + case Nil => + throw new IllegalArgumentException( + s"Finally '${merger.id.logicalOpId.id}' is not wired to any TryCatch: " + + "its From Try / From Catch inputs must come from the same frame's try / catch subgraphs" + ) + case multiple => + throw new IllegalArgumentException( + s"Finally '${merger.id.logicalOpId.id}' matches multiple TryCatch frames: " + + multiple.map(_.id.logicalOpId.id).mkString(", ") + ) + } + }.toMap + + // ---- 2. cones (ownership; inclusive of nested frames, cut at own merger) + val frames: List[Frame] = splitters.map { s => + val gate = gates(s.id.logicalOpId) + val merger = + mergerToFrame.collectFirst { + case (mergerId, frameId) if frameId == s.id.logicalOpId => plan.getOperator(mergerId) + } + val stop = merger.map(_.id).toSet + val tryCone = reach(plan, s.id, TRY_PORT, crossInternal = true, stopAt = stop) + val catchCone = reach(plan, gate.id, CATCH_PORT, crossInternal = true, stopAt = stop) + Frame(s, gate, merger, tryCone, catchCone) + } + + // ---- 3. validations + frames.foreach(validateFrame(plan, _)) + + // ---- 4. per-op innermost-frame assignment (smallest containing cone) + def innermostFrameOf(opId: PhysicalOpIdentity): Option[Frame] = + frames.filter(_.tryCone.contains(opId)).sortBy(_.tryCone.size).headOption + + // tail ports of a frame's try cone: no outgoing links, or feeding the + // paired merger's From Try — owned by the innermost frame only + val signalSources: Map[OperatorIdentity, List[(PhysicalOpIdentity, PortIdentity)]] = + frames + .map { frame => + val tails = frame.tryCone.toList + .filter(opId => innermostFrameOf(opId).exists(_.logicalOpId == frame.logicalOpId)) + .flatMap { opId => + val op = plan.getOperator(opId) + op.outputPorts.keys + .filterNot(_.internal) + .filter { portId => + val outLinks = plan.links.filter(l => l.fromOpId == opId && l.fromPortId == portId) + outLinks.isEmpty || + frame.merger.exists(m => + outLinks.exists(l => l.toOpId == m.id && l.toPortId == FROM_TRY) + ) + } + .map(portId => (opId, portId)) + } + .sortBy { case (opId, portId) => (opId.logicalOpId.id, opId.layerName, portId.id) } + frame.logicalOpId -> tails + } + .toMap + + // escalation: catch-cone TERMINAL leaves signal the innermost enclosing + // frame's gate, so catch-side failures escalate even when their branch + // never reaches the Merger. Applies to every frame: leaves feeding the + // Merger's From Catch are excluded below (they have outgoing links; their + // errors escalate through the Merger's forwarding), but a failing terminal + // FORK of a catch branch would otherwise die at its result table while the + // frame reported a clean recovery. A catch failure is an error this frame + // did NOT handle — it must reach the enclosing frame (or, with none, the + // console/failed-with-errors path), never the own gate (that edge would be + // the cycle gate -> catch cone -> gate). + val escalationSources: Map[OperatorIdentity, List[(PhysicalOpIdentity, PortIdentity)]] = + frames + .flatMap { frame => + frames + .filter(f => + f.logicalOpId != frame.logicalOpId && f.tryCone.contains(frame.splitter.id) + ) + .sortBy(_.tryCone.size) + .headOption + .map { enclosing => + val tails = frame.catchCone.toList.flatMap { opId => + val op = plan.getOperator(opId) + op.outputPorts.keys + .filterNot(_.internal) + .filter(portId => + plan.links.forall(l => !(l.fromOpId == opId && l.fromPortId == portId)) + ) + .map(portId => (opId, portId)) + } + enclosing.logicalOpId -> tails + } + } + .groupBy(_._1) + .view + .mapValues(_.flatMap(_._2).toList) + .toMap + + // ---- 5. rebuild the plan with reconfigured gates/mergers + signal links + val gateSignals: Map[PhysicalOpIdentity, List[(PhysicalOpIdentity, PortIdentity)]] = + frames.map { frame => + val signals = signalSources(frame.logicalOpId) ++ + escalationSources.getOrElse(frame.logicalOpId, List.empty) + frame.gate.id -> signals + }.toMap + + // guarded = drain semantics on own failure (error State in-band). Marks + // every cone operator plus the frame apparatus itself; everything outside + // any frame keeps the default report-and-pause behavior. + val guardedOps: Set[PhysicalOpIdentity] = + frames.flatMap(f => f.cone + f.splitter.id + f.gate.id ++ f.merger.map(_.id)).toSet + + val rebuiltOps: Map[PhysicalOpIdentity, PhysicalOp] = plan.operators.map { op => + val rebuilt = frames.find(_.gate.id == op.id) match { + case Some(frame) => + val signals = gateSignals(frame.gate.id) + val signalPorts = signals.zipWithIndex.map { + case (_, idx) => InputPort(PortIdentity(idx + 1, internal = true), s"signal-${idx + 1}") + } + val snapshotPort = InputPort( + TryCatchOpDesc.SNAPSHOT_IN, + "snapshot", + dependencies = signalPorts.map(_.id) + ) + val config = new CatchGateConfig() + config.ownConeOpIds = frame.coneIdStrings + op.withInputPorts(snapshotPort :: signalPorts) + .withPartitionRequirement( + List(None) ++ signalPorts.map(_ => Some(SignalPartition())) + ) + .copy(opExecInitInfo = + OpExecWithClassName(GateClassName, objectMapper.writeValueAsString(config)) + ) + case None => + frames.find(_.merger.exists(_.id == op.id)) match { + case Some(frame) => + val config = new FinallyMergerConfig() + config.ownConeOpIds = frame.coneIdStrings + op.copy(opExecInitInfo = + OpExecWithClassName(MergerClassName, objectMapper.writeValueAsString(config)) + ) + case None => op + } + } + rebuilt.id -> rebuilt.withGuarded(guardedOps.contains(rebuilt.id)) + }.toMap + + val signalLinks: Set[PhysicalLink] = gateSignals.flatMap { + case (gateId, signals) => + signals.zipWithIndex.map { + case ((srcOpId, srcPortId), idx) => + PhysicalLink(srcOpId, srcPortId, gateId, PortIdentity(idx + 1, internal = true)) + } + }.toSet + + rebuildPlan(rebuiltOps, plan.links ++ signalLinks) + } + + /** + * Rebuild the plan from scratch: ops stripped of link bookkeeping and + * re-linked in topological order so `addLink`'s schema propagation always + * sees a resolved source schema (mirrors `WorkflowCompiler.expandLogicalPlan`). + * + * The order must come from the NEW edge set, not the original plan's: a + * synthesized signal edge runs from a try-cone leaf (late in the original + * order) back to that frame's gate (early), so ordering by the original + * index would add the gate's OUTGOING links before its incoming signal + * edge — propagating an unresolved schema into the catch subgraph, which + * later surfaces as `SchemaNotAvailableException` from the compiler's + * strict schema check. + */ + private def rebuildPlan( + ops: Map[PhysicalOpIdentity, PhysicalOp], + links: Set[PhysicalLink] + ): PhysicalPlan = { + val topoIndex = topologicalIndex(ops.keySet, links) + + var plan = PhysicalPlan(operators = Set.empty, links = Set.empty) + ops.values.toList.sortBy(op => topoIndex(op.id)).foreach { op => + val stripped = op + .withInputPorts(op.inputPorts.values.map(_._1).toList) + .withOutputPorts(op.outputPorts.values.map(_._1).toList) + plan = plan.addOperator(stripped.propagateSchema()) + } + links.toList + .sortBy(link => (topoIndex(link.fromOpId), topoIndex(link.toOpId), link.toPortId.id)) + .foreach(link => plan = plan.addLink(link)) + plan + } + + /** Kahn topological order over the given operators and links (the graph is + * acyclic by construction: frames only add leaf->gate and escalation edges, + * and try/catch cones are validated disjoint). Any operator left in a cycle + * is appended last so a malformed graph fails in the compiler's own checks + * rather than here. */ + private def topologicalIndex( + opIds: Set[PhysicalOpIdentity], + links: Set[PhysicalLink] + ): Map[PhysicalOpIdentity, Int] = { + val outgoing = links.groupBy(_.fromOpId).view.mapValues(_.toList.map(_.toOpId)).toMap + val inDegree = mutable.Map(opIds.toList.map(_ -> 0): _*) + links.foreach(link => inDegree(link.toOpId) = inDegree(link.toOpId) + 1) + + // deterministic tie-breaking: sort ready operators by id + val ready = mutable.SortedSet[PhysicalOpIdentity]()( + Ordering.by((id: PhysicalOpIdentity) => (id.logicalOpId.id, id.layerName)) + ) + inDegree.filter(_._2 == 0).keys.foreach(ready.add) + + val order = mutable.ArrayBuffer[PhysicalOpIdentity]() + while (ready.nonEmpty) { + val next = ready.head + ready.remove(next) + order.append(next) + outgoing.getOrElse(next, Nil).foreach { downstream => + inDegree(downstream) = inDegree(downstream) - 1 + if (inDegree(downstream) == 0) ready.add(downstream) + } + } + val remaining = opIds.diff(order.toSet).toList.sortBy(id => id.logicalOpId.id) + (order.toList ++ remaining).zipWithIndex.toMap + } + + private def gateOf(plan: PhysicalPlan, splitter: PhysicalOp): PhysicalOp = { + val gateId = PhysicalOpIdentity(splitter.id.logicalOpId, TryCatchOpDesc.GATE_LAYER) + plan.getOperator(gateId) + } + + private def sourcesInto( + plan: PhysicalPlan, + opId: PhysicalOpIdentity, + portId: PortIdentity + ): List[PhysicalOpIdentity] = + plan.links.filter(l => l.toOpId == opId && l.toPortId == portId).map(_.fromOpId).toList + + /** + * BFS over the plan's links starting from one operator's output port. + * `crossInternal = false` refuses to traverse links landing on internal + * ports (frame-internal snapshot wiring) — used for pairing, where crossing + * into a nested frame's catch side would break uniqueness. `stopAt` ops are + * neither included nor expanded (the frame's own merger). + */ + private def reach( + plan: PhysicalPlan, + fromOpId: PhysicalOpIdentity, + fromPortId: PortIdentity, + crossInternal: Boolean, + stopAt: Set[PhysicalOpIdentity] + ): Set[PhysicalOpIdentity] = { + val visited = mutable.Set[PhysicalOpIdentity]() + val frontier = mutable.Queue[PhysicalOpIdentity]() + + def linkAllowed(link: PhysicalLink): Boolean = + crossInternal || !link.toPortId.internal + + plan.links + .filter(l => l.fromOpId == fromOpId && l.fromPortId == fromPortId) + .filter(linkAllowed) + .map(_.toOpId) + .filterNot(stopAt.contains) + .foreach(op => if (visited.add(op)) frontier.enqueue(op)) + + while (frontier.nonEmpty) { + val current = frontier.dequeue() + plan.links + .filter(_.fromOpId == current) + .filter(linkAllowed) + .map(_.toOpId) + .filterNot(stopAt.contains) + .foreach(op => if (visited.add(op)) frontier.enqueue(op)) + } + visited.toSet + } + + private def validateFrame(plan: PhysicalPlan, frame: Frame): Unit = { + val name = frame.logicalOpId.id + if (frame.tryCone.isEmpty) { + throw new IllegalArgumentException( + s"TryCatch '$name': the Try port must be connected to the subgraph to guard" + ) + } + val overlap = frame.tryCone.intersect(frame.catchCone) + if (overlap.nonEmpty) { + throw new IllegalArgumentException( + s"TryCatch '$name': the try and catch subgraphs must be disjoint; shared operators: " + + overlap.map(_.logicalOpId.id).mkString(", ") + ) + } + // Error Info feeding back into the try subgraph is a structural cycle + // (reporter -> cone op -> tails -> signal edges -> gate) and a temporal + // paradox (the report exists only once the attempt resolved). Catch-cone + // consumers are fine: they become part of the catch branch. + val errorInfoReach = + reach(plan, frame.gate.id, ERROR_INFO_PORT, crossInternal = true, stopAt = Set.empty) + val feedback = errorInfoReach.intersect(frame.tryCone) + if (feedback.nonEmpty) { + throw new IllegalArgumentException( + s"TryCatch '$name': Error Info cannot feed back into the frame's own try subgraph; " + + s"offending: ${feedback.map(_.logicalOpId.id).mkString(", ")}" + ) + } + frame.merger.foreach { merger => + val fromTrySources = sourcesInto(plan, merger.id, FROM_TRY).toSet + val fromCatchSources = sourcesInto(plan, merger.id, FROM_CATCH).toSet + val badTry = fromTrySources.diff(frame.tryCone + frame.splitter.id) + if (badTry.nonEmpty) { + throw new IllegalArgumentException( + s"Finally of '$name': every From Try input must come from the frame's try subgraph; " + + s"offending: ${badTry.map(_.logicalOpId.id).mkString(", ")}" + ) + } + val badCatch = fromCatchSources.diff(frame.catchCone + frame.gate.id) + if (badCatch.nonEmpty) { + throw new IllegalArgumentException( + s"Finally of '$name': every From Catch input must come from the frame's catch subgraph; " + + s"offending: ${badCatch.map(_.logicalOpId.id).mkString(", ")}" + ) + } + val catchConnected = + plan.links.exists(l => l.fromOpId == frame.gate.id && l.fromPortId == CATCH_PORT) + if (!catchConnected) { + throw new IllegalArgumentException( + s"TryCatch '$name': a paired Finally requires the Catch port to be connected" + ) + } + // The Merger is the frame's only exit: a cone operator wired around it + // into its downstream would join a branch's raw (possibly failed) + // output with the released winner, and the synthesized signal edges + // would close a cycle (merger -> join -> tail -> gate -> catch -> + // merger). Reject the bypass here with the rule's own words instead of + // letting the cycle surface later as a schema error. + val mergerReach = merger.outputPorts.keys + .filterNot(_.internal) + .flatMap(portId => reach(plan, merger.id, portId, crossInternal = true, stopAt = Set.empty)) + .toSet + val bypass = mergerReach.intersect(frame.cone + frame.splitter.id + frame.gate.id) + if (bypass.nonEmpty) { + throw new IllegalArgumentException( + s"TryCatch '$name': the try/catch subgraphs may reach the region after the Finally " + + s"only through the Finally itself; offending: " + + bypass.map(_.logicalOpId.id).mkString(", ") + ) + } + } + } +} diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/WorkflowCompiler.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/WorkflowCompiler.scala index 65d13e41794..fbd69f4ef32 100644 --- a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/WorkflowCompiler.scala +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/WorkflowCompiler.scala @@ -253,9 +253,13 @@ class WorkflowCompiler( logicalPlan.resolveScanSourceOpFileName(errorList) // 3. expand the logical plan to the physical plan, and get the output ports that need storage - val (physicalPlan, outputPortsNeedingStorage) = + val (rawPhysicalPlan, outputPortsNeedingStorage) = expandLogicalPlan(logicalPlan, logicalPlanPojo.opsToViewResult, errorList) + // 3.5 expand try/catch frames: cones, attribution configs, signal edges, + // gate port dependencies (no-op when the workflow has no TryCatch) + val physicalPlan = TryCatchFramePass.run(rawPhysicalPlan, errorList) + // 4. collect the output schema for each logical op // even if an error is encountered during logical => physical expansion, we still want to // collect the output schemas of the remaining no-error operators. In Lenient mode diff --git a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala new file mode 100644 index 00000000000..49b9c2978f8 --- /dev/null +++ b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala @@ -0,0 +1,402 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.common.compiler + +import org.apache.texera.amber.core.executor.OpExecWithClassName +import org.apache.texera.amber.core.tuple.{AttributeType, Schema} +import org.apache.texera.amber.core.virtualidentity.{ + ExecutionIdentity, + OperatorIdentity, + PhysicalOpIdentity, + WorkflowIdentity +} +import org.apache.texera.amber.core.workflow._ +import org.apache.texera.amber.operator.trycatch.{ + CatchGateConfig, + FinallyOpDesc, + TryCatchOpDesc +} +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { + + private val wid = WorkflowIdentity(1L) + private val eid = ExecutionIdentity(1L) + private val schema: Schema = Schema().add("field1", AttributeType.INTEGER) + + /** an ordinary single-input single-output pass-through physical op */ + private def simpleOp(name: String): PhysicalOp = + PhysicalOp + .oneToOnePhysicalOp( + PhysicalOpIdentity(OperatorIdentity(name), "main"), + wid, + eid, + OpExecWithClassName(s"test.$name", "") + ) + .withInputPorts(List(InputPort(PortIdentity()))) + .withOutputPorts(List(OutputPort(PortIdentity()))) + .withPropagateSchema( + SchemaPropagationFunc(in => Map(PortIdentity() -> in(PortIdentity()))) + ) + + private def sourceOp(name: String): PhysicalOp = + PhysicalOp + .sourcePhysicalOp( + PhysicalOpIdentity(OperatorIdentity(name), "main"), + wid, + eid, + OpExecWithClassName(s"test.$name", "") + ) + .withInputPorts(List.empty) + .withOutputPorts(List(OutputPort(PortIdentity()))) + .withPropagateSchema(SchemaPropagationFunc(_ => Map(PortIdentity() -> schema))) + + /** + * Consumes anything, emits the standard test schema — stands in for a + * user op that turns Error Info rows back into branch-shaped data (a + * classifier/formatter), so tests can wire the gate's Error Info port + * into data subgraphs without tripping schema propagation. + */ + private def fixedSchemaOp(name: String): PhysicalOp = + PhysicalOp + .oneToOnePhysicalOp( + PhysicalOpIdentity(OperatorIdentity(name), "main"), + wid, + eid, + OpExecWithClassName(s"test.$name", "") + ) + .withInputPorts(List(InputPort(PortIdentity()))) + .withOutputPorts(List(OutputPort(PortIdentity()))) + .withPropagateSchema(SchemaPropagationFunc(_ => Map(PortIdentity() -> schema))) + + private def opId(name: String, layer: String = "main"): PhysicalOpIdentity = + PhysicalOpIdentity(OperatorIdentity(name), layer) + + private def gateConfig(gate: PhysicalOp): CatchGateConfig = { + val descString = gate.opExecInitInfo match { + case OpExecWithClassName(_, desc) => desc + } + objectMapper.readValue(descString, classOf[CatchGateConfig]) + } + + private def link( + from: PhysicalOpIdentity, + fromPort: PortIdentity, + to: PhysicalOpIdentity, + toPort: PortIdentity + ): PhysicalLink = PhysicalLink(from, fromPort, to, toPort) + + private val out0 = PortIdentity() + private val in0 = PortIdentity() + private val catchPort = PortIdentity(1) + + /** + * source -> TryCatch(Data); Try -> tryOp -> Finally.FromTry; + * Catch -> catchOp -> Finally.FromCatch; Finally -> downstream + */ + private def buildFramePlan(): PhysicalPlan = { + val tcDesc = new TryCatchOpDesc() + tcDesc.setOperatorId("tc") + val fDesc = new FinallyOpDesc() + fDesc.setOperatorId("fin") + + val tcPlan = tcDesc.getPhysicalPlan(wid, eid) + val merger = fDesc.getPhysicalOp(wid, eid) + + val src = sourceOp("src") + val tryOp = simpleOp("tryOp") + val catchOp = simpleOp("catchOp") + val down = simpleOp("down") + + val splitterId = opId("tc", TryCatchOpDesc.SPLITTER_LAYER) + val gateId = opId("tc", TryCatchOpDesc.GATE_LAYER) + + var plan = PhysicalPlan(operators = Set.empty, links = Set.empty) + (Set(src, tryOp, catchOp, down, merger) ++ tcPlan.operators).foreach { op => + plan = plan.addOperator(op.propagateSchema()) + } + List( + link(opId("src"), out0, splitterId, in0), + tcPlan.links.head, // splitter -> gate snapshot + link(splitterId, out0, opId("tryOp"), in0), + link(gateId, catchPort, opId("catchOp"), in0), + link(opId("tryOp"), out0, merger.id, PortIdentity()), + link(opId("catchOp"), out0, merger.id, PortIdentity(1)), + link(merger.id, out0, opId("down"), in0) + ).foreach(l => plan = plan.addLink(l)) + plan + } + + "TryCatchFramePass" should "leave plans without frames untouched" in { + var plan = PhysicalPlan(operators = Set.empty, links = Set.empty) + plan = plan.addOperator(sourceOp("src").propagateSchema()) + plan = plan.addOperator(simpleOp("down").propagateSchema()) + plan = plan.addLink(link(opId("src"), out0, opId("down"), in0)) + val result = TryCatchFramePass.run(plan, None) + result should be theSameInstanceAs plan + } + + it should "synthesize signal edges from try-cone tails into the gate with dependee snapshot" in { + val result = TryCatchFramePass.run(buildFramePlan(), None) + val gate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + + // one signal port (tryOp's output feeds the merger => it is a tail) + val signalPorts = gate.inputPorts.keys.filter(p => p.internal && p.id > 0) + signalPorts should have size 1 + val signalPort = signalPorts.head + + // snapshot depends on all signal ports + val snapshotInputPort = gate.inputPorts(TryCatchOpDesc.SNAPSHOT_IN)._1 + snapshotInputPort.dependencies should contain theSameElementsAs signalPorts + + // the signal link originates at the tail's output port + val signalLinks = result.links.filter(l => l.toOpId == gate.id && l.toPortId == signalPort) + signalLinks.map(_.fromOpId) shouldBe Set(opId("tryOp")) + + // signal ports carry the SignalPartition requirement (indexed by port id) + gate.partitionRequirement.lift(signalPort.id).flatten shouldBe Some(SignalPartition()) + + // cone attribution baked into the gate's executor config + val descString = gate.opExecInitInfo match { + case OpExecWithClassName(_, desc) => desc + } + val config = objectMapper.readValue(descString, classOf[CatchGateConfig]) + config.ownConeOpIds should contain("tryOp/main") + config.ownConeOpIds should contain("catchOp/main") + config.ownConeOpIds should not contain "src/main" + config.ownConeOpIds should not contain "down/main" + + // frame semantics only inside the frame: cones + apparatus get the + // guarded (fail-by-draining) flag; everything outside keeps the + // default report-and-pause behavior + result.getOperator(opId("tryOp")).isGuarded shouldBe true + result.getOperator(opId("catchOp")).isGuarded shouldBe true + result.getOperator(opId("tc", TryCatchOpDesc.SPLITTER_LAYER)).isGuarded shouldBe true + gate.isGuarded shouldBe true + result.getOperator(opId("fin")).isGuarded shouldBe true + result.getOperator(opId("src")).isGuarded shouldBe false + result.getOperator(opId("down")).isGuarded shouldBe false + } + + it should "collect exactly one signal port when the try cone has a single tail wired to Finally" in { + // The common shape: one linear try branch ending at Finally. Only the + // user's chosen tail becomes a signal source — intermediate ports (whose + // output flows on to another cone operator) must not. + var plan = buildFramePlan() + // extend the try branch: tryOp -> mid -> Finally (tryOp becomes intermediate) + val mid = simpleOp("mid") + plan = plan.addOperator(mid.propagateSchema()) + plan = plan.removeLink( + link(opId("tryOp"), out0, opId("fin"), PortIdentity()) + ) + plan = plan.addLink(link(opId("tryOp"), out0, opId("mid"), in0)) + plan = plan.addLink(link(opId("mid"), out0, opId("fin"), PortIdentity())) + + val result = TryCatchFramePass.run(plan, None) + val gate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + val signalPorts = gate.inputPorts.keys.filter(p => p.internal && p.id > 0) + signalPorts should have size 1 + val signalLinks = result.links.filter(l => l.toOpId == gate.id && l.toPortId.id > 0) + signalLinks.map(_.fromOpId) shouldBe Set(opId("mid")) + } + + it should "collect every ending of a forked try cone, not just the one wired to Finally" in { + // A fork where one tail goes to Finally and the other ends in its own + // result table: BOTH must signal the gate, or a failure in the second fork + // would be invisible and the frame could declare success while it is still + // running. + var plan = buildFramePlan() + val sideTail = simpleOp("sideTail") + plan = plan.addOperator(sideTail.propagateSchema()) + // the splitter's Try port also feeds a second branch that ends nowhere + plan = plan.addLink( + link(opId("tc", TryCatchOpDesc.SPLITTER_LAYER), out0, opId("sideTail"), in0) + ) + + val result = TryCatchFramePass.run(plan, None) + val gate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + val signalPorts = gate.inputPorts.keys.filter(p => p.internal && p.id > 0) + signalPorts should have size 2 + val signalLinks = result.links.filter(l => l.toOpId == gate.id && l.toPortId.id > 0) + signalLinks.map(_.fromOpId) shouldBe Set(opId("tryOp"), opId("sideTail")) + // every signal port carries the tuple-dropping requirement + signalPorts.foreach(p => + gate.partitionRequirement.lift(p.id).flatten shouldBe Some(SignalPartition()) + ) + } + + it should "not signal-partition the user's data link into Finally" in { + // Partitioning is PER-LINK: the tail wired to Finally keeps two links out + // of the same output port -- the user's data link (normal partitioning, so + // results actually reach Finally) and the synthesized signal link + // (tuple-dropping). Signal-partitioning the data link would silently empty + // the frame's output. + val result = TryCatchFramePass.run(buildFramePlan(), None) + val merger = result.getOperator(opId("fin")) + val gate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + + // the tail feeds BOTH the merger's From Try and a gate signal port + val fromTail = result.links.filter(l => l.fromOpId == opId("tryOp")) + fromTail.map(l => (l.toOpId, l.toPortId)) shouldBe Set( + (merger.id, PortIdentity()), + (gate.id, PortIdentity(1, internal = true)) + ) + + // the merger declares no signal requirement on any input port, so its + // incoming data links resolve to ordinary partitioning + merger.inputPorts.keys.foreach(portId => + merger.partitionRequirement.lift(portId.id).flatten should not be Some( + SignalPartition() + ) + ) + } + + it should "reject a cross-cone edge (try and catch subgraphs must be disjoint)" in { + var plan = buildFramePlan() + // wire tryOp also into catchOp: catch cone now overlaps the try cone + plan = plan.addLink(link(opId("tryOp"), out0, opId("catchOp"), in0)) + assertThrows[IllegalArgumentException] { + TryCatchFramePass.run(plan, None) + } + } + + it should "reject a try-cone edge that joins the region after the Finally (Merger bypass)" in { + // The Merger is the frame's only exit. tryOp -> down while merger -> down + // would join the raw (possibly failed) attempt with the released winner — + // and close a cycle through the synthesized signal edges. Must be rejected + // with the rule's own words, not surface later as a schema error. + var plan = buildFramePlan() + plan = plan.addLink(link(opId("tryOp"), out0, opId("down"), in0)) + val err = intercept[IllegalArgumentException] { + TryCatchFramePass.run(plan, None) + } + err.getMessage should include("only through the Finally") + } + + it should "reject Error Info feeding back into the frame's own try subgraph" in { + // reporter -> cone op -> tails -> signal edges -> gate is a structural + // cycle and a temporal paradox (the report exists only once the attempt + // resolved) — validated with a clear message. + var plan = buildFramePlan() + val err = fixedSchemaOp("err") + plan = plan.addOperator(err.propagateSchema()) + plan = plan.addLink( + link(opId("tc", TryCatchOpDesc.GATE_LAYER), TryCatchOpDesc.ERROR_INFO_PORT, opId("err"), in0) + ) + plan = plan.addLink(link(opId("err"), out0, opId("tryOp"), in0)) + val rejected = intercept[IllegalArgumentException] { + TryCatchFramePass.run(plan, None) + } + rejected.getMessage should include("Error Info") + } + + it should "allow Error Info to feed the catch subgraph (catch(SpecificError) wiring)" in { + // The error is known exactly when the snapshot releases, so joining the + // report with catch data is causally sound; the consumer stays an external + // upstream of the catch cone, not a frame member. + var plan = buildFramePlan() + val err = fixedSchemaOp("err") + plan = plan.addOperator(err.propagateSchema()) + plan = plan.addLink( + link(opId("tc", TryCatchOpDesc.GATE_LAYER), TryCatchOpDesc.ERROR_INFO_PORT, opId("err"), in0) + ) + plan = plan.addLink(link(opId("err"), out0, opId("catchOp"), in0)) + + val result = TryCatchFramePass.run(plan, None) + val config = gateConfig(result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER))) + config.ownConeOpIds should contain("catchOp/main") + config.ownConeOpIds should not contain "err/main" + } + + it should "allow Error Info consumers downstream of the frame (audit lane)" in { + // The green 6->8 edge: Error Info flows past the Finally and may join the + // post-frame region — without becoming a signal source or a cone member. + var plan = buildFramePlan() + val audit = fixedSchemaOp("audit") + plan = plan.addOperator(audit.propagateSchema()) + plan = plan.addLink( + link( + opId("tc", TryCatchOpDesc.GATE_LAYER), + TryCatchOpDesc.ERROR_INFO_PORT, + opId("audit"), + in0 + ) + ) + plan = plan.addLink(link(opId("audit"), out0, opId("down"), in0)) + + val result = TryCatchFramePass.run(plan, None) + val gate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + val signalPorts = gate.inputPorts.keys.filter(p => p.internal && p.id > 0) + signalPorts should have size 1 // audit's tail did NOT become a signal source + gateConfig(gate).ownConeOpIds should not contain "audit/main" + } + + it should "allow external upstreams to join either cone without joining the frame" in { + // The green edges into the cones: attribution follows the FAILING + // operator, not the data's destination — a side input is not guarded by + // the frame it happens to feed. + var plan = buildFramePlan() + val side = sourceOp("side") + plan = plan.addOperator(side.propagateSchema()) + plan = plan.addLink(link(opId("side"), out0, opId("tryOp"), in0)) + plan = plan.addLink(link(opId("side"), out0, opId("catchOp"), in0)) + + val result = TryCatchFramePass.run(plan, None) + val config = gateConfig(result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER))) + config.ownConeOpIds should contain("tryOp/main") + config.ownConeOpIds should contain("catchOp/main") + config.ownConeOpIds should not contain "side/main" + } + + it should "reject a Finally whose From Try comes from outside the frame" in { + val tcDesc = new TryCatchOpDesc() + tcDesc.setOperatorId("tc") + val fDesc = new FinallyOpDesc() + fDesc.setOperatorId("fin") + val tcPlan = tcDesc.getPhysicalPlan(wid, eid) + val merger = fDesc.getPhysicalOp(wid, eid) + val src = sourceOp("src") + val stranger = sourceOp("stranger") + val tryOp = simpleOp("tryOp") + val catchOp = simpleOp("catchOp") + val splitterId = opId("tc", TryCatchOpDesc.SPLITTER_LAYER) + val gateId = opId("tc", TryCatchOpDesc.GATE_LAYER) + + var plan = PhysicalPlan(operators = Set.empty, links = Set.empty) + (Set(src, stranger, tryOp, catchOp, merger) ++ tcPlan.operators).foreach { op => + plan = plan.addOperator(op.propagateSchema()) + } + List( + link(opId("src"), out0, splitterId, in0), + tcPlan.links.head, + link(splitterId, out0, opId("tryOp"), in0), + link(gateId, catchPort, opId("catchOp"), in0), + link(opId("tryOp"), out0, merger.id, PortIdentity()), + link(opId("stranger"), out0, merger.id, PortIdentity()), // outside the frame! + link(opId("catchOp"), out0, merger.id, PortIdentity(1)) + ).foreach(l => plan = plan.addLink(l)) + + assertThrows[IllegalArgumentException] { + TryCatchFramePass.run(plan, None) + } + } +} diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/state/State.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/state/State.scala index aa88ce16605..c1bf03d019d 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/state/State.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/state/State.scala @@ -42,6 +42,37 @@ final case class State(values: Map[String, Any]) { } object State { + + // Reserved key marking a State as an in-band error signal (an operator + // failure traveling forward as a dataflow event). The value is an envelope + // map: {operatorId, workerId, errorType, message}. Recognition and port + // poisoning happen in the worker (DataProcessor); operators are oblivious + // unless they explicitly consume it (try/catch frame operators do). + val ErrorKey = "__error__" + private val ErrOperatorId = "operatorId" + + def errorState(operatorId: String, workerId: String, e: Throwable): State = + State( + Map( + ErrorKey -> Map( + ErrOperatorId -> operatorId, + "workerId" -> workerId, + "errorType" -> e.getClass.getName, + "message" -> Option(e.getMessage).getOrElse("") + ) + ) + ) + + def isError(state: State): Boolean = state.values.contains(ErrorKey) + + /** The canonical physical-operator id of the failing operator, if this is an error State. */ + def errorOperatorId(state: State): Option[String] = + state.values.get(ErrorKey) match { + case Some(envelope: Map[_, _]) => + envelope.asInstanceOf[Map[String, Any]].get(ErrOperatorId).map(_.toString) + case _ => None + } + private val Content = "content" // loop-control bookkeeping owned by the (Python) worker runtime; not user // state and never in the content JSON. Materialized as its own columns, diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PartitionInfo.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PartitionInfo.scala index b5e6a02dab6..b0b824691c1 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PartitionInfo.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PartitionInfo.scala @@ -33,6 +33,7 @@ import com.fasterxml.jackson.annotation.{JsonSubTypes, JsonTypeInfo} new Type(value = classOf[SinglePartition], name = "single"), new Type(value = classOf[OneToOnePartition], name = "oneToOne"), new Type(value = classOf[BroadcastPartition], name = "broadcast"), + new Type(value = classOf[SignalPartition], name = "signal"), new Type(value = classOf[UnknownPartition], name = "none") ) ) @@ -102,6 +103,15 @@ final case class OneToOnePartition() extends PartitionInfo {} */ final case class BroadcastPartition() extends PartitionInfo {} +/** + * Signal links (try/catch frame wiring): data tuples are dropped at the + * sender; only States, ECMs and END_CHANNEL traverse the link. No producer + * partition ever satisfies it (default `satisfies`), so the allocator always + * resolves such links to the signal requirement itself — which maps to + * SignalPartitioning (tuples dropped at the sender). + */ +final case class SignalPartition() extends PartitionInfo {} + /** * Represents there is no specific partitioning scheme of the input stream. */ diff --git a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PhysicalOp.scala b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PhysicalOp.scala index a1e93050665..8d97af84b35 100644 --- a/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PhysicalOp.scala +++ b/common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PhysicalOp.scala @@ -214,7 +214,12 @@ case class PhysicalOp( // hint for number of workers suggestedWorkerNum: Option[Int] = None, // name of the PVE to execute within - pveName: String = "" + pveName: String = "", + // True iff this operator sits inside a try/catch frame's cone (set by + // TryCatchFramePass). A guarded worker turns its own executor failure into + // an in-band error State and drains; an unguarded worker keeps the + // default behavior: report and pause. + isGuarded: Boolean = false ) extends LazyLogging { // all the "dependee" links are also blocking @@ -323,6 +328,14 @@ case class PhysicalOp( def withParallelizable(parallelizable: Boolean): PhysicalOp = this.copy(parallelizable = parallelizable) + /** + * Mark this operator as inside a try/catch frame's cone: its workers turn + * their own executor failures into in-band error States and drain, instead + * of the unguarded report-and-pause behavior. + */ + def withGuarded(guarded: Boolean): PhysicalOp = + this.copy(isGuarded = guarded) + /** * creates a copy with the specified property that whether this operator is one-to-many */ @@ -530,6 +543,24 @@ case class PhysicalOp( this.outputPorts(link.fromPortId)._1.blocking } + /** + * The actual (dependee, depender) input-port dependency pairs declared on this + * operator, one entry per dependency edge. Unlike `getInputPortDependencyPairs` + * (a flat topological processing order), this stays correct when one port + * depends on N ports — consecutive positions in a topological order are NOT + * dependency pairs once a port has multiple independent dependees. + * Sorted for determinism (inputPorts is an unordered Map). + */ + @JsonIgnore + def getInputPortDependencyEdges: List[(PortIdentity, PortIdentity)] = { + inputPorts.values + .flatMap { + case (port, _, _) => port.dependencies.map(dependee => dependee -> port.id) + } + .toList + .sortBy { case (dependee, depender) => (depender.id, depender.internal, dependee.id) } + } + /** * Some operators process their inputs in a particular order. Eg: 2 phase hash join first * processes the build input, then the probe input. diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PartitionInfoSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PartitionInfoSpec.scala index a105d9d5922..c756e7a36b9 100644 --- a/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PartitionInfoSpec.scala +++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PartitionInfoSpec.scala @@ -182,6 +182,7 @@ class PartitionInfoSpec extends AnyFlatSpec { "SinglePartition", "OneToOnePartition", "BroadcastPartition", + "SignalPartition", "UnknownPartition" ) ) @@ -268,6 +269,7 @@ class PartitionInfoSpec extends AnyFlatSpec { "single" -> SinglePartition(), "oneToOne" -> OneToOnePartition(), "broadcast" -> BroadcastPartition(), + "signal" -> SignalPartition(), "none" -> UnknownPartition() ) diff --git a/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PhysicalOpSpec.scala b/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PhysicalOpSpec.scala index 1e8d247e452..c7088403fc6 100644 --- a/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PhysicalOpSpec.scala +++ b/common/workflow-core/src/test/scala/org/apache/texera/amber/core/workflow/PhysicalOpSpec.scala @@ -203,6 +203,35 @@ class PhysicalOpSpec extends AnyFlatSpec { assert(op.getInputPortDependencyPairs == Nil) } + "PhysicalOp.getInputPortDependencyEdges" should "enumerate one pair per declared dependency, including N dependees on one port" in { + // a port depending on many independent ports (e.g. a snapshot port + // depending on N signal ports) — consecutive topological positions are + // NOT pairs here, the actual edges are + val op = newOp("g").withInputPorts( + List( + InputPort(PortIdentity(0)), + InputPort(PortIdentity(1)), + InputPort(PortIdentity(2)), + InputPort( + PortIdentity(3), + dependencies = Seq(PortIdentity(0), PortIdentity(1), PortIdentity(2)) + ) + ) + ) + assert( + op.getInputPortDependencyEdges == List( + PortIdentity(0) -> PortIdentity(3), + PortIdentity(1) -> PortIdentity(3), + PortIdentity(2) -> PortIdentity(3) + ) + ) + } + + it should "be empty when no port declares dependencies" in { + val op = newOp("a").withInputPorts(List(InputPort(PortIdentity(0)))) + assert(op.getInputPortDependencyEdges == Nil) + } + // ----- addOutputLink guards ----- "PhysicalOp.addOutputLink" should "reject links from other operators or undeclared ports" in { diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/LogicalOp.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/LogicalOp.scala index efa46144180..649315bee61 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/LogicalOp.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/LogicalOp.scala @@ -50,6 +50,7 @@ import org.apache.texera.amber.operator.huggingFace.{ HuggingFaceTextSummarizationOpDesc } import org.apache.texera.amber.operator.ifStatement.IfOpDesc +import org.apache.texera.amber.operator.trycatch.{FinallyOpDesc, TryCatchOpDesc} import org.apache.texera.amber.operator.intersect.IntersectOpDesc import org.apache.texera.amber.operator.intervalJoin.IntervalJoinOpDesc import org.apache.texera.amber.operator.keywordSearch.KeywordSearchOpDesc @@ -166,6 +167,8 @@ trait StateTransferFunc @JsonSubTypes( Array( new Type(value = classOf[IfOpDesc], name = "If"), + new Type(value = classOf[TryCatchOpDesc], name = "TryCatch"), + new Type(value = classOf[FinallyOpDesc], name = "Finally"), new Type(value = classOf[SankeyDiagramOpDesc], name = "SankeyDiagram"), new Type(value = classOf[IcicleChartOpDesc], name = "IcicleChart"), new Type(value = classOf[FileListerSourceOpDesc], name = "FileLister"), diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala index 4634ad1c18c..de5bf3f75e7 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala @@ -32,10 +32,12 @@ class IfOpExec(descString: String) extends OperatorExecutor { //This function can handle one or more states. //The state can have mutiple key-value pairs. Keys are not identified by conditionName will be ignored. //It can accept any value that can be converted to a boolean. For example, Int 1 will be converted to true. + //States that do not carry conditionName at all (e.g. error States, loop envelopes) + //are passed through without touching the routing decision. override def processState(state: State, port: Int): Option[State] = { - outputPort = - if (state.values(desc.conditionName).asInstanceOf[Boolean]) PortIdentity(1) - else PortIdentity() + state.values.get(desc.conditionName).foreach { value => + outputPort = if (value.asInstanceOf[Boolean]) PortIdentity(1) else PortIdentity() + } Some(state) } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala new file mode 100644 index 00000000000..9c342813214 --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import com.fasterxml.jackson.annotation.JsonProperty + +/** + * Executor config for the CatchGate physical operator — not a user-facing + * descriptor. The compiler pass fills `ownConeOpIds` (canonical physical-op id + * strings, "logicalOpId/layerName") once the frame's cones are known, so the + * gate can attribute error States: own-cone error => trigger the catch; + * foreign error => not ours to handle. + */ +class CatchGateConfig { + @JsonProperty + var ownConeOpIds: List[String] = List() +} diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala new file mode 100644 index 00000000000..0503ee0e7ab --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala @@ -0,0 +1,111 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.executor.OperatorExecutor +import org.apache.texera.amber.core.state.State +import org.apache.texera.amber.core.tuple.{Tuple, TupleLike} +import org.apache.texera.amber.core.workflow.PortIdentity +import org.apache.texera.amber.util.JSONUtils.objectMapper + +import scala.collection.mutable + +/** + * An If generalized to N conditions (the frame's outcome gate). + * + * Ports: internal port 0 = the materialized input snapshot (its InputPort + * declares dependencies on all signal ports, so two-phase execution resolves + * the try side fully before the first snapshot tuple); internal ports 1..N = + * signal ports, one per try-cone leaf, carrying only States and completion + * (their links use SignalPartitioning — tuples dropped at the sender). + * + * Decision: an own-cone `__error__` State seen on a signal port => the attempt + * failed => forward the snapshot into the catch subgraph. Clean completion, or + * a foreign error (failure upstream of the frame — not ours to handle) => + * drop the snapshot; the catch subgraph runs empty and finalizes cleanly. + * + * State handling: signal-port States are absorbed (a caught error must not + * leak; forwarding foreign errors is the snapshot lane's job, since a foreign + * State always also traveled through the splitter). Snapshot-lane States + * (loop envelopes, foreign errors) pass through into the catch subgraph. + */ +class CatchGateOpExec(descString: String) extends OperatorExecutor { + + private val config: CatchGateConfig = + objectMapper.readValue(descString, classOf[CatchGateConfig]) + private val ownCone: Set[String] = config.ownConeOpIds.toSet + private val snapshotPortId: Int = TryCatchOpDesc.SNAPSHOT_IN.id + private val catchPort = PortIdentity(1) + private val errorInfoPort = TryCatchOpDesc.ERROR_INFO_PORT + + private var failed = false + // one report per failure event: drain guards make each worker emit at most + // one error State per execution, so (operatorId, workerId) identifies the + // event however many signal ports (fan-out) it arrived on + private val reportedErrors = mutable.LinkedHashMap[(String, String), TupleLike]() + + override def processState(state: State, port: Int): Option[State] = { + if (port == snapshotPortId) { + // snapshot lane: pass through (loop envelopes, foreign errors) + Some(state) + } else { + // signal lane: record own-cone failures, absorb everything + if (State.isError(state) && State.errorOperatorId(state).exists(ownCone.contains)) { + failed = true + recordError(state) + } + None + } + } + + private def recordError(state: State): Unit = { + val envelope = state.values.get(State.ErrorKey) match { + case Some(m: Map[_, _]) => m.asInstanceOf[Map[String, Any]] + case _ => Map.empty[String, Any] + } + def field(key: String): String = envelope.get(key).map(_.toString).getOrElse("") + val key = (field("operatorId"), field("workerId")) + if (!reportedErrors.contains(key)) { + reportedErrors(key) = + TupleLike(field("errorType"), field("message"), field("operatorId"), field("workerId")) + } + } + + // two output ports: every emission must be port-targeted + override def processTupleMultiPort( + tuple: Tuple, + port: Int + ): Iterator[(TupleLike, Option[PortIdentity])] = { + if (port == snapshotPortId && failed) Iterator((tuple, Some(catchPort))) + else Iterator.empty + } + + override def onFinishMultiPort(port: Int): Iterator[(TupleLike, Option[PortIdentity])] = { + // emitted at snapshot-lane completion (phase 2, output ports assigned), + // which also covers the empty-input case + if (port == snapshotPortId) { + reportedErrors.values.iterator.map(row => (row, Some(errorInfoPort))) + } else { + Iterator.empty + } + } + + override def processTuple(tuple: Tuple, port: Int): Iterator[TupleLike] = ??? +} diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala new file mode 100644 index 00000000000..0c137f500ff --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import com.fasterxml.jackson.annotation.JsonProperty + +/** + * Executor config for the Finally Merger physical operator — not a user-facing + * descriptor. The compiler pass fills `ownConeOpIds` with the paired frame's + * cone (canonical physical-op id strings) so the Merger can absorb *caught* + * try-side error States (they must not leak past the frame) while forwarding + * catch-side and foreign ones (escalation). + */ +class FinallyMergerConfig { + @JsonProperty + var ownConeOpIds: List[String] = List() +} diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala new file mode 100644 index 00000000000..1054c413981 --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.executor.OperatorExecutor +import org.apache.texera.amber.core.state.State +import org.apache.texera.amber.core.tuple.{Tuple, TupleLike} +import org.apache.texera.amber.core.workflow.PortIdentity +import org.apache.texera.amber.util.JSONUtils.objectMapper + +import scala.collection.mutable.ArrayBuffer + +/** + * The Finally physical operator: stages both sides, flushes the winner out of + * the output port named after it (`Try Result` on success, `Catch Result` on + * failure) — the outcome is part of the output, not just the rows. + * + * Port 0 (`From Try`) is consumed first (it is the dependee of `From Catch`) + * during the dependee phase, in which this operator has no output ports yet — + * so the try-side results are buffered and both release decisions happen at + * `onFinishMultiPort(From Catch)` in the second phase: + * - try side completed cleanly => flush the staged try results to + * `Try Result` (the catch subgraph received nothing from its gate and + * stays empty); + * - try side failed => flush the staged catch results to + * `Catch Result`. + * + * Failure handling costs no code here: an own-cone error State on `From Try` + * clears the staged attempt and is absorbed (a caught failure must not leak + * past the frame); per-port drain in the worker discards later tuples and + * suppresses `onFinish` on the poisoned port. Catch-side error States are + * forwarded (default pass-through) — that is escalation to any enclosing + * frame; with both flushes suppressed, nothing is emitted (double failure). + */ +class FinallyMergerOpExec(descString: String) extends OperatorExecutor { + + private val config: FinallyMergerConfig = + objectMapper.readValue(descString, classOf[FinallyMergerConfig]) + private val ownCone: Set[String] = config.ownConeOpIds.toSet + + private val FROM_TRY = 0 + private val FROM_CATCH = 1 + + private val stagedTry = new ArrayBuffer[Tuple]() + private val stagedCatch = new ArrayBuffer[Tuple]() + private var trySideClean = false + + override def processTuple(tuple: Tuple, port: Int): Iterator[TupleLike] = { + if (port == FROM_TRY) stagedTry.append(tuple) else stagedCatch.append(tuple) + Iterator.empty + } + + override def processState(state: State, port: Int): Option[State] = { + if ( + port == FROM_TRY && State.isError(state) && + State.errorOperatorId(state).exists(ownCone.contains) + ) { + // caught: discard the failed attempt's staged output and absorb the + // error — it must not leak past the frame + stagedTry.clear() + None + } else { + // catch-side errors and foreign errors travel on (escalation); + // ordinary States (loop envelope, user States) pass through + Some(state) + } + } + + override def onFinishMultiPort(port: Int): Iterator[(TupleLike, Option[PortIdentity])] = { + if (port == FROM_TRY) { + // dependee phase: no output ports exist yet; just record the outcome + // (this callback is suppressed by the worker if the port was poisoned) + trySideClean = true + Iterator.empty + } else if (trySideClean) { + stagedTry.iterator.map(t => (t, Some(FinallyOpDesc.TRY_RESULT))) + } else { + stagedCatch.iterator.map(t => (t, Some(FinallyOpDesc.CATCH_RESULT))) + } + } +} diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala new file mode 100644 index 00000000000..d44d1dde8d9 --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.executor.OpExecWithClassName +import org.apache.texera.amber.core.virtualidentity.{ExecutionIdentity, WorkflowIdentity} +import org.apache.texera.amber.core.workflow._ +import org.apache.texera.amber.operator.LogicalOp +import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, OperatorInfo} +import org.apache.texera.amber.util.JSONUtils.objectMapper + +/** + * Reconvergence point of a TryCatch frame: emits exactly one branch's complete + * results — the try side's when the attempt succeeded, the catch side's when it + * failed (all-or-nothing; never a mix). `From Catch` depends on `From Try`, so + * the try side resolves fully before the catch side is consumed — the release + * decision is deterministic, never a race. + * + * The winner leaves through the port named after it: `Try Result` on success, + * `Catch Result` on failure (both carry the branches' common schema). The + * outcome is therefore observable downstream — connect only one port to react + * to that outcome, or connect both to the same downstream input (a Union) to + * get "the winner, whichever it was". + */ +class FinallyOpDesc extends LogicalOp { + + override def getPhysicalOp( + workflowId: WorkflowIdentity, + executionId: ExecutionIdentity + ): PhysicalOp = { + PhysicalOp + .oneToOnePhysicalOp( + workflowId, + executionId, + operatorIdentifier, + OpExecWithClassName( + "org.apache.texera.amber.operator.trycatch.FinallyMergerOpExec", + objectMapper.writeValueAsString(new FinallyMergerConfig()) + ) + ) + .withInputPorts(operatorInfo.inputPorts) + .withOutputPorts(operatorInfo.outputPorts) + .withPropagateSchema( + SchemaPropagationFunc(inputSchemas => { + val trySchema = inputSchemas(operatorInfo.inputPorts.head.id) + val catchSchema = inputSchemas(operatorInfo.inputPorts.last.id) + require( + trySchema == catchSchema, + s"Finally requires the try and catch branches to produce the same schema (got $trySchema vs $catchSchema)" + ) + Map( + FinallyOpDesc.TRY_RESULT -> trySchema, + FinallyOpDesc.CATCH_RESULT -> trySchema + ) + }) + ) + } + + override def operatorInfo: OperatorInfo = + OperatorInfo( + "Finally", + "Emit the winning branch of a Try Catch frame: try results on Try Result when the attempt succeeds, catch results on Catch Result when it fails", + OperatorGroupConstants.CONTROL_GROUP, + inputPorts = List( + InputPort(PortIdentity(), "From Try"), + InputPort(PortIdentity(1), "From Catch", dependencies = List(PortIdentity())) + ), + outputPorts = List( + OutputPort(FinallyOpDesc.TRY_RESULT, "Try Result"), + OutputPort(FinallyOpDesc.CATCH_RESULT, "Catch Result") + ) + ) +} + +object FinallyOpDesc { + val TRY_RESULT: PortIdentity = PortIdentity() + val CATCH_RESULT: PortIdentity = PortIdentity(1) +} diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDesc.scala new file mode 100644 index 00000000000..07e57a6e971 --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDesc.scala @@ -0,0 +1,145 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.executor.OpExecWithClassName +import org.apache.texera.amber.core.tuple.{AttributeType, Schema} +import org.apache.texera.amber.core.virtualidentity.{ + ExecutionIdentity, + PhysicalOpIdentity, + WorkflowIdentity +} +import org.apache.texera.amber.core.workflow._ +import org.apache.texera.amber.operator.LogicalOp +import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, OperatorInfo} +import org.apache.texera.amber.util.JSONUtils.objectMapper + +/** + * Block-level try/catch: the subgraph fed by the + * Try port is one attempt; if any operator in it fails, the same input is + * replayed from a snapshot through the Catch port into the fallback subgraph. + * Pair with a Finally operator to reconverge the winning branch's results. + * + * Expands to two physical operators: + * - splitter: tees the input — live stream out the Try port, eager snapshot + * out an internal port toward the gate. + * - gate: an If generalized to N conditions. Signal ports (one per try-cone + * leaf output port, added by the compiler pass) collect error States; the + * snapshot port depends on all of them, so it is consumed only after the + * try side fully resolved. Own-cone error seen => forward the snapshot into + * the catch subgraph; clean completion or foreign error => drop it. + */ +class TryCatchOpDesc extends LogicalOp { + + override def getPhysicalPlan( + workflowId: WorkflowIdentity, + executionId: ExecutionIdentity + ): PhysicalPlan = { + val dataInput = operatorInfo.inputPorts.head // Data (0) + val tryOutput = operatorInfo.outputPorts.head // Try (0, external) + val catchOutput = operatorInfo.outputPorts(1) // Catch (1, external, on the gate) + val errorInfoOutput = operatorInfo.outputPorts(2) // Error Info (2, on the gate) + + val snapshotOut = OutputPort(TryCatchOpDesc.SNAPSHOT_OUT, "snapshot") + val snapshotIn = InputPort(TryCatchOpDesc.SNAPSHOT_IN, "snapshot") + + val splitter = PhysicalOp + .oneToOnePhysicalOp( + PhysicalOpIdentity(operatorIdentifier, TryCatchOpDesc.SPLITTER_LAYER), + workflowId, + executionId, + OpExecWithClassName( + "org.apache.texera.amber.operator.trycatch.TrySplitterOpExec", + // non-empty: ExecFactory routes empty descStrings to a legacy + // (int, int) constructor signature + "{}" + ) + ) + .withInputPorts(List(dataInput)) + .withOutputPorts(List(tryOutput, snapshotOut)) + .withPropagateSchema( + SchemaPropagationFunc(inputSchemas => { + val inputSchema = inputSchemas(dataInput.id) + Map(tryOutput.id -> inputSchema, snapshotOut.id -> inputSchema) + }) + ) + + // Signal ports and the snapshot port's dependencies on them are added by + // the compiler pass once the try cone is known (they are one-per-leaf). + val gate = PhysicalOp + .oneToOnePhysicalOp( + PhysicalOpIdentity(operatorIdentifier, TryCatchOpDesc.GATE_LAYER), + workflowId, + executionId, + OpExecWithClassName( + "org.apache.texera.amber.operator.trycatch.CatchGateOpExec", + objectMapper.writeValueAsString(new CatchGateConfig()) + ) + ) + .withInputPorts(List(snapshotIn)) + .withOutputPorts(List(catchOutput, errorInfoOutput)) + .withPropagateSchema( + SchemaPropagationFunc(inputSchemas => + Map( + catchOutput.id -> inputSchemas(snapshotIn.id), + errorInfoOutput.id -> TryCatchOpDesc.ERROR_INFO_SCHEMA + ) + ) + ) + + PhysicalPlan( + operators = Set(splitter, gate), + links = Set(PhysicalLink(splitter.id, snapshotOut.id, gate.id, snapshotIn.id)) + ) + } + + override def operatorInfo: OperatorInfo = + OperatorInfo( + "Try Catch", + "Run the Try subgraph as one attempt; on any failure, replay the same input through the Catch subgraph", + OperatorGroupConstants.CONTROL_GROUP, + inputPorts = List(InputPort(PortIdentity(), "Data")), + outputPorts = List( + OutputPort(PortIdentity(), "Try"), + OutputPort(PortIdentity(1), "Catch"), + OutputPort(PortIdentity(2), "Error Info") + ) + ) +} + +object TryCatchOpDesc { + val SPLITTER_LAYER = "splitter" + val GATE_LAYER = "gate" + + // splitter-side snapshot output port + val SNAPSHOT_OUT: PortIdentity = PortIdentity(1, internal = true) + // gate-side snapshot input port; signal ports use internal ids 1..N + val SNAPSHOT_IN: PortIdentity = PortIdentity(0, internal = true) + // gate-side error report output port (one row per caught own-cone failure, + // deduplicated on (operatorId, workerId); catch-cone failures escalate to + // the enclosing frame's report instead) + val ERROR_INFO_PORT: PortIdentity = PortIdentity(2) + + val ERROR_INFO_SCHEMA: Schema = Schema() + .add("errorType", AttributeType.STRING) + .add("message", AttributeType.STRING) + .add("operatorId", AttributeType.STRING) + .add("workerId", AttributeType.STRING) +} diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TrySplitterOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TrySplitterOpExec.scala new file mode 100644 index 00000000000..24f10216b30 --- /dev/null +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/TrySplitterOpExec.scala @@ -0,0 +1,43 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.executor.OperatorExecutor +import org.apache.texera.amber.core.tuple.{Tuple, TupleLike} +import org.apache.texera.amber.core.workflow.PortIdentity + +/** + * Physical entry of a TryCatch frame: tees every input tuple to the live Try + * port and to the snapshot port (whose edge toward the gate is materialized by + * the scheduler, providing the replay data for the catch subgraph). + */ +class TrySplitterOpExec(descString: String) extends OperatorExecutor { + + private val tryPort = PortIdentity() + private val snapshotPort = TryCatchOpDesc.SNAPSHOT_OUT + + override def processTupleMultiPort( + tuple: Tuple, + port: Int + ): Iterator[(TupleLike, Option[PortIdentity])] = + Iterator((tuple, Some(tryPort)), (tuple, Some(snapshotPort))) + + override def processTuple(tuple: Tuple, port: Int): Iterator[TupleLike] = ??? +} diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala new file mode 100644 index 00000000000..49ec903349f --- /dev/null +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.state.State +import org.apache.texera.amber.core.tuple.{AttributeType, Schema, Tuple, TupleLike} +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class CatchGateOpExecSpec extends AnyFlatSpec with Matchers { + + private val schema: Schema = Schema().add("field1", AttributeType.INTEGER) + private val tuple: Tuple = TupleLike(1).enforceSchema(schema) + private val snapshotPort = TryCatchOpDesc.SNAPSHOT_IN.id // 0 + private val signalPort = 1 + + private def mkGate(ownCone: List[String]): CatchGateOpExec = { + val config = new CatchGateConfig() + config.ownConeOpIds = ownCone + new CatchGateOpExec(objectMapper.writeValueAsString(config)) + } + + private val ownError = State.errorState("myop/main", "worker-0", new RuntimeException("boom")) + private val foreignError = + State.errorState("upstream/main", "worker-9", new RuntimeException("outer boom")) + + private val catchPort = org.apache.texera.amber.core.workflow.PortIdentity(1) + + "CatchGate" should "drop the snapshot when the try side completed cleanly" in { + val gate = mkGate(List("myop/main")) + gate.processTupleMultiPort(tuple, snapshotPort) shouldBe empty + gate.onFinishMultiPort(snapshotPort) shouldBe empty // no error rows either + } + + it should "forward the snapshot to the catch port after an own-cone error State" in { + val gate = mkGate(List("myop/main")) + gate.processState(ownError, signalPort) shouldBe None // absorbed + gate.processTupleMultiPort(tuple, snapshotPort).toList shouldBe + List((tuple, Some(catchPort))) + } + + it should "emit one deduplicated Error Info row per failure event" in { + val gate = mkGate(List("myop/main")) + gate.processState(ownError, signalPort) shouldBe None + // same failure arriving via a second signal port (fan-out duplicate) + gate.processState(ownError, signalPort + 1) shouldBe None + val rows = gate.onFinishMultiPort(snapshotPort).toList + rows should have size 1 + rows.head._2 shouldBe Some(TryCatchOpDesc.ERROR_INFO_PORT) + } + + it should "not trigger on a foreign error (failure upstream of the frame)" in { + val gate = mkGate(List("myop/main")) + gate.processState(foreignError, signalPort) shouldBe None // absorbed, not ours + gate.processTupleMultiPort(tuple, snapshotPort) shouldBe empty + gate.onFinishMultiPort(snapshotPort) shouldBe empty // no row: not ours to report + } + + it should "pass snapshot-lane States through (loop envelopes, foreign errors)" in { + val gate = mkGate(List("myop/main")) + val envelope = State(Map("some" -> "state")) + gate.processState(envelope, snapshotPort) shouldBe Some(envelope) + gate.processState(foreignError, snapshotPort) shouldBe Some(foreignError) + } + + it should "absorb ordinary States on signal ports" in { + val gate = mkGate(List("myop/main")) + gate.processState(State(Map("some" -> "state")), signalPort) shouldBe None + } + + it should "drop signal-lane tuples defensively" in { + val gate = mkGate(List("myop/main")) + gate.processState(ownError, signalPort) + gate.processTupleMultiPort(tuple, signalPort) shouldBe empty + } +} diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala new file mode 100644 index 00000000000..25460199865 --- /dev/null +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.state.State +import org.apache.texera.amber.core.tuple.{AttributeType, Schema, Tuple, TupleLike} +// Tuple is used for the field-level assertions below +import org.apache.texera.amber.util.JSONUtils.objectMapper +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class FinallyMergerOpExecSpec extends AnyFlatSpec with Matchers { + + private val schema: Schema = Schema().add("field1", AttributeType.INTEGER) + private def tuples(range: Range): List[Tuple] = + range.map(i => TupleLike(i).enforceSchema(schema)).toList + + private val FROM_TRY = 0 + private val FROM_CATCH = 1 + private val TRY_RESULT = FinallyOpDesc.TRY_RESULT + private val CATCH_RESULT = FinallyOpDesc.CATCH_RESULT + + private def mkMerger(ownCone: List[String] = List("myop/main")): FinallyMergerOpExec = { + val config = new FinallyMergerConfig() + config.ownConeOpIds = ownCone + new FinallyMergerOpExec(objectMapper.writeValueAsString(config)) + } + + private val ownError = State.errorState("myop/main", "worker-0", new RuntimeException("boom")) + private val foreignError = + State.errorState("upstream/main", "worker-9", new RuntimeException("outer boom")) + + "FinallyMerger" should "flush the try side out the Try Result port on success" in { + val merger = mkMerger() + val tryResults = tuples(0 until 5) + tryResults.foreach(t => merger.processTuple(t, FROM_TRY) shouldBe empty) + // dependee phase: no output ports yet + merger.onFinishMultiPort(FROM_TRY) shouldBe empty + merger.onFinishMultiPort(FROM_CATCH).toList shouldBe + tryResults.map(t => (t, Some(TRY_RESULT))) + } + + it should "flush the catch side out the Catch Result port on failure" in { + val merger = mkMerger() + val attempt = tuples(0 until 3) + val fallback = tuples(100 until 104) + attempt.foreach(merger.processTuple(_, FROM_TRY)) + // own-cone failure arrives in-band: staged attempt discarded, error absorbed + merger.processState(ownError, FROM_TRY) shouldBe None + // the worker suppresses onFinishMultiPort(FROM_TRY) for the poisoned port — not called + fallback.foreach(merger.processTuple(_, FROM_CATCH)) + merger.onFinishMultiPort(FROM_CATCH).toList shouldBe + fallback.map(t => (t, Some(CATCH_RESULT))) + } + + it should "emit nothing on double failure and forward the catch-side error (escalation)" in { + val merger = mkMerger() + tuples(0 until 3).foreach(merger.processTuple(_, FROM_TRY)) + merger.processState(ownError, FROM_TRY) shouldBe None + tuples(100 until 102).foreach(merger.processTuple(_, FROM_CATCH)) + // catch-side error: forwarded downstream — that IS escalation + merger.processState(ownError, FROM_CATCH) shouldBe Some(ownError) + // the worker suppresses both onFinish calls (both ports poisoned): nothing flushed + } + + it should "forward foreign errors on any port (they belong to an enclosing frame)" in { + val merger = mkMerger() + merger.processState(foreignError, FROM_TRY) shouldBe Some(foreignError) + merger.processState(foreignError, FROM_CATCH) shouldBe Some(foreignError) + } + + it should "pass ordinary States through" in { + val merger = mkMerger() + val envelope = State(Map("k" -> 1L)) + merger.processState(envelope, FROM_TRY) shouldBe Some(envelope) + } + + it should "emit the winning branch's actual field values, unchanged" in { + // Not just row COUNTS: the tuples that come out of Finally must be the + // very rows the winning branch produced, with their values intact. + val merger = mkMerger() + val tryRows = List( + TupleLike(11).enforceSchema(schema), + TupleLike(22).enforceSchema(schema) + ) + tryRows.foreach(merger.processTuple(_, FROM_TRY)) + merger.onFinishMultiPort(FROM_TRY) + val emitted = merger.onFinishMultiPort(FROM_CATCH).toList + emitted.map(_._2) shouldBe List(Some(TRY_RESULT), Some(TRY_RESULT)) + emitted.map(_._1) shouldBe tryRows + emitted.map(_._1.asInstanceOf[Tuple].getField[Integer]("field1")) shouldBe List(11, 22) + } + + it should "emit the catch branch's actual field values when the attempt failed" in { + val merger = mkMerger() + // the try attempt produced rows, then failed: those must NOT appear + List(TupleLike(1).enforceSchema(schema)).foreach(merger.processTuple(_, FROM_TRY)) + merger.processState(ownError, FROM_TRY) shouldBe None + val fallbackRows = List( + TupleLike(77).enforceSchema(schema), + TupleLike(88).enforceSchema(schema) + ) + fallbackRows.foreach(merger.processTuple(_, FROM_CATCH)) + // onFinishMultiPort(FROM_TRY) is suppressed by the worker for the poisoned port + val emitted = merger.onFinishMultiPort(FROM_CATCH).toList + emitted.map(_._2) shouldBe List(Some(CATCH_RESULT), Some(CATCH_RESULT)) + emitted.map(_._1) shouldBe fallbackRows + emitted.map(_._1.asInstanceOf[Tuple].getField[Integer]("field1")) shouldBe List(77, 88) + } +} diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala new file mode 100644 index 00000000000..9cc05b9bf04 --- /dev/null +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala @@ -0,0 +1,143 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.amber.operator.trycatch + +import org.apache.texera.amber.core.tuple.{AttributeType, Schema} +import org.apache.texera.amber.core.virtualidentity.{ExecutionIdentity, WorkflowIdentity} +import org.apache.texera.amber.core.workflow.PortIdentity +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class TryCatchOpDescSpec extends AnyFlatSpec with Matchers { + + private val schema: Schema = Schema().add("field1", AttributeType.INTEGER) + + "TryCatchOpDesc" should "declare one Data input and Try/Catch/Error Info outputs" in { + val desc = new TryCatchOpDesc() + val info = desc.operatorInfo + info.inputPorts.map(_.displayName) shouldBe List("Data") + info.outputPorts.map(_.displayName) shouldBe List("Try", "Catch", "Error Info") + info.outputPorts.map(_.id) shouldBe List(PortIdentity(), PortIdentity(1), PortIdentity(2)) + } + + it should "expand into splitter and gate connected by an internal snapshot link" in { + val desc = new TryCatchOpDesc() + val plan = desc.getPhysicalPlan(WorkflowIdentity(1L), ExecutionIdentity(1L)) + plan.operators.map(_.id.layerName) shouldBe Set( + TryCatchOpDesc.SPLITTER_LAYER, + TryCatchOpDesc.GATE_LAYER + ) + plan.links should have size 1 + val link = plan.links.head + link.fromPortId shouldBe TryCatchOpDesc.SNAPSHOT_OUT + link.toPortId shouldBe TryCatchOpDesc.SNAPSHOT_IN + link.fromPortId.internal shouldBe true + link.toPortId.internal shouldBe true + } + + it should "propagate the input schema to Try/Catch and the fixed schema to Error Info" in { + val desc = new TryCatchOpDesc() + val outputSchemas = desc.getExternalOutputSchemas(Map(PortIdentity() -> schema)) + outputSchemas shouldBe Map( + PortIdentity() -> schema, + PortIdentity(1) -> schema, + PortIdentity(2) -> TryCatchOpDesc.ERROR_INFO_SCHEMA + ) + } + + "FinallyOpDesc" should "declare Try Result and Catch Result output ports" in { + val desc = new FinallyOpDesc() + val info = desc.operatorInfo + info.outputPorts.map(_.displayName) shouldBe List("Try Result", "Catch Result") + info.outputPorts.map(_.id) shouldBe + List(FinallyOpDesc.TRY_RESULT, FinallyOpDesc.CATCH_RESULT) + } + + it should "require matching schemas and emit the common schema on both result ports" in { + val desc = new FinallyOpDesc() + val out = desc.getExternalOutputSchemas( + Map(PortIdentity() -> schema, PortIdentity(1) -> schema) + ) + out shouldBe Map( + FinallyOpDesc.TRY_RESULT -> schema, + FinallyOpDesc.CATCH_RESULT -> schema + ) + + val otherSchema = Schema().add("other", AttributeType.STRING) + assertThrows[Exception] { + desc.getExternalOutputSchemas( + Map(PortIdentity() -> schema, PortIdentity(1) -> otherSchema) + ) + } + } + + it should "adopt whatever schema the connected branches carry" in { + // Finally does not impose a schema: it takes the one the user's two + // branches actually produce (both must agree) and emits exactly that, so + // the frame is transparent to whatever columns flow through it. + val desc = new FinallyOpDesc() + val wideSchema = Schema() + .add("id", AttributeType.LONG) + .add("name", AttributeType.STRING) + .add("score", AttributeType.DOUBLE) + desc.getExternalOutputSchemas( + Map(PortIdentity() -> wideSchema, PortIdentity(1) -> wideSchema) + ) shouldBe Map( + FinallyOpDesc.TRY_RESULT -> wideSchema, + FinallyOpDesc.CATCH_RESULT -> wideSchema + ) + } + + it should "reject branches whose columns differ only in type or order" in { + val desc = new FinallyOpDesc() + val a = Schema().add("x", AttributeType.INTEGER).add("y", AttributeType.STRING) + // same names, different type on x + val differentType = + Schema().add("x", AttributeType.LONG).add("y", AttributeType.STRING) + // same attributes, different order + val differentOrder = + Schema().add("y", AttributeType.STRING).add("x", AttributeType.INTEGER) + + val typeError = intercept[Exception] { + desc.getExternalOutputSchemas(Map(PortIdentity() -> a, PortIdentity(1) -> differentType)) + } + typeError.getMessage should include("same schema") + + assertThrows[Exception] { + desc.getExternalOutputSchemas(Map(PortIdentity() -> a, PortIdentity(1) -> differentOrder)) + } + } + + "FinallyOpDesc" should "declare From Catch dependent on From Try" in { + val desc = new FinallyOpDesc() + val fromCatch = desc.operatorInfo.inputPorts.last + fromCatch.dependencies shouldBe List(PortIdentity()) + } + + "TrySplitterOpExec" should "tee every tuple to the try and snapshot ports" in { + val exec = new TrySplitterOpExec("") + val tuple = org.apache.texera.amber.core.tuple.TupleLike(1).enforceSchema(schema) + val out = exec.processTupleMultiPort(tuple, 0).toList + out shouldBe List( + (tuple, Some(PortIdentity())), + (tuple, Some(TryCatchOpDesc.SNAPSHOT_OUT)) + ) + } +} diff --git a/docs/reference/operators/control-block/_index.md b/docs/reference/operators/control-block/_index.md index 6a4dd74d6b1..2f194f24957 100644 --- a/docs/reference/operators/control-block/_index.md +++ b/docs/reference/operators/control-block/_index.md @@ -31,7 +31,9 @@ tags: [control-block] | Operator | Description | |----------|-------------| +| [Finally](finally/) | Emit the winning branch of a Try Catch frame: try results on success, catch results on failure | | [If](if/) | If | | [Sleep](sleep/) | Sleep n seconds between each tuple | +| [Try Catch](try-catch/) | Run a subgraph as one attempt; on any failure, replay the same input through a fallback subgraph | -**Total**: 2 operators +**Total**: 4 operators diff --git a/docs/reference/operators/control-block/finally.md b/docs/reference/operators/control-block/finally.md new file mode 100644 index 00000000000..dfd2b9dee5b --- /dev/null +++ b/docs/reference/operators/control-block/finally.md @@ -0,0 +1,79 @@ + + +--- +title: "Finally" +description: "Emit the winning branch of a Try Catch frame: try results on Try Result when the attempt succeeds, catch results on Catch Result when it fails" +category: "Control Block" +operator_type: "Finally" +tags: [control-block] +--- + +[Home](../../) > [Control Block](../) + +Finally is the reconvergence point of a [Try Catch](../try-catch/) frame. Wire +the tail of the Try subgraph into **From Try** and the tail of the Catch +subgraph into **From Catch**; Finally emits whichever branch actually ran, out +of the output port named after it — **Try Result** on success, **Catch +Result** on failure — so downstream can both consume the winner and observe +which side won. + +It is the dataflow equivalent of a `try`/`catch` *expression*: the construct +evaluates to the try result on success and to the catch result on failure, and +the code after it does not care which. + +### Semantics + +- **All or nothing.** Downstream of Finally sees exactly one branch's complete + output, never a mixture and never duplicates. Results are staged until the + outcome is known, then released in one go. +- **The outcome is part of the output.** The winner's rows leave through + **Try Result** when the attempt succeeded and through **Catch Result** when + it failed; the other port completes empty. Connect one port to react to the + outcome (e.g. alert only on recovery), or connect **both ports to the same + downstream input** — a Union — to get "the winner, whichever it was". +- **Deterministic outcome.** `From Catch` is consumed only after `From Try` + fully resolves, so the release decision never depends on timing. +- **Both branches must produce the same schema** (same rule as Union) — both + result ports carry that common schema. +- **On double failure** (the catch branch fails too) Finally emits nothing and + the failure escalates to any enclosing frame. +- **Optional.** A Try Catch works without a Finally: use it when the branches + end in their own sinks/result tables and there is nothing to reconverge. + +### Placing side effects + +Because Finally releases only the winning branch's complete output, operators +placed *after* it run exactly once, on data from exactly one attempt. That makes +it the right place for writes you do not want a failed attempt to have made. +Writes placed *inside* a branch are not rolled back if that branch fails. + +### Input Ports + +| Port | Description | +|------|-------------| +| From Try | Tail of the Try subgraph | +| From Catch | Tail of the Catch subgraph (consumed after From Try resolves) | + +### Output Ports + +| Port | Description | Mode | +|------|-------------|------| +| Try Result | The try branch's results, when the attempt succeeded (empty otherwise) | [Set Snapshot](../../output-modes/#set-snapshot) | +| Catch Result | The catch branch's results, when the attempt failed (empty otherwise) | [Set Snapshot](../../output-modes/#set-snapshot) | diff --git a/docs/reference/operators/control-block/try-catch.md b/docs/reference/operators/control-block/try-catch.md new file mode 100644 index 00000000000..dc2cc2d6d99 --- /dev/null +++ b/docs/reference/operators/control-block/try-catch.md @@ -0,0 +1,105 @@ + + +--- +title: "Try Catch" +description: "Run a subgraph as one attempt; on any failure, replay the same input through a fallback subgraph" +category: "Control Block" +operator_type: "TryCatch" +tags: [control-block] +--- + +[Home](../../) > [Control Block](../) + +Try Catch guards a whole subgraph. Everything downstream of the **Try** port is +one *attempt*: if any operator in it fails at runtime, the attempt is abandoned +and the **same input** is replayed through the subgraph downstream of the +**Catch** port. Pair it with a [Finally](../finally/) operator to reconverge +both branches into a single result stream. + +This is block-level, not per-row: one failing row abandons the whole attempt, +exactly as one thrown exception abandons a `try { ... }` block in a programming +language. It is a *fallback*, not a retry — the catch branch is a different +pipeline over the same data. + +### Example + +Fetch enrichment data from a remote API in the Try branch; if the API is down, +the Catch branch falls back to a local cached table. Both branches feed a +Finally, so everything after the frame is written once, from whichever branch +succeeded. + +### Semantics + +- **One attempt.** A failure anywhere in the Try subgraph — including in a + branch that was succeeding — abandons the whole attempt. +- **Replay, not re-read.** The frame snapshots its input, so the Catch branch + sees exactly the rows the Try branch was given, without re-running upstream + operators. +- **The Catch branch runs only on failure.** On success it receives no rows and + finishes immediately. +- **Unconnected Catch port = rethrow.** A Try Catch with nothing wired to Catch + propagates the failure outward (to an enclosing frame, or to the execution) — + it does not swallow it. To swallow deliberately, wire Catch to an operator + that discards its input. +- **Nesting.** Frames may nest. A failure is handled by the innermost frame + containing the failing operator. If that frame's *catch* branch also fails, + the failure escalates to the next enclosing frame — the dataflow equivalent + of rethrowing from a catch block. +- **Side effects are not rolled back.** Rows the failed attempt already wrote + (to a result table, an external sink) remain written, just as statements that + ran before a `throw` are not undone. Put side effects *after* a Finally if you + need them to happen exactly once. + +### What is caught + +Caught: runtime errors raised by operator logic — a Python or Java UDF raising, +a bad column reference, a malformed value, a failing `open()` (e.g. wrong +database credentials). + +**Not** caught: infrastructure failures — a worker process dying, a lost cluster +node, out-of-memory. Those end the execution, the same as before; a `kill -9` +does not run your `finally` block either. Recovering from infrastructure +failures is the job of fault-tolerance/replay, not of control flow. + +### Wiring rules + +The compiler rejects a workflow that breaks these: + +- The Try and Catch subgraphs must be **disjoint** — no operator may be in both + (they are separate blocks, like `try { }` and `catch { }`). +- If a Finally is present, its `From Try` input must come from the Try subgraph + and its `From Catch` input from the Catch subgraph of the **same** Try Catch. +- If a Finally is present, the Catch port must be connected. + +Data from outside the frame may be joined into either subgraph freely; the +frame only guards operators reachable from its own ports. + +### Input Ports + +| Port | Description | +|------|-------------| +| Data | The rows to guard | + +### Output Ports + +| Port | Description | Mode | +|------|-------------|------| +| Try | The attempt: connect the subgraph to guard | [Set Snapshot](../../output-modes/#set-snapshot) | +| Catch | The fallback: receives a replay of the input if the attempt fails | [Set Snapshot](../../output-modes/#set-snapshot) | diff --git a/frontend/src/assets/operator_images/Finally.png b/frontend/src/assets/operator_images/Finally.png new file mode 100644 index 0000000000000000000000000000000000000000..a1f9b084808f09a6f6dc86096f12f6e3232c40cd GIT binary patch literal 477 zcmeAS@N?(olHy`uVBq!ia0vp^2_VeD1|%QND7Ro>U|izq;uumf=k4vpqC*NIu89Tz ztCwBoy%Nz_EI;S{cDGM{hi3TY`c6{uyp;RvP%{4m|Y`DPFnd0uf5`dbsQ59 z`*Uuva(eS&GSiwa0oy-Y7&a>(;kVCH6mFmXVa~^hD@kfRo34Bi-o7Az-It=n{jpc< z*`ky7>u=w|dB5e+dHqW6Uu82s9$qi@VDiFg5_Lxz{M(|_euOelk^a=l*q4aU>k+N? zulpvS-+V~+!o%;|Z!^oR^=jDnv-WlfXTcgT#$S)FD{HY@1Z`!g{q(xvD$}1&|C>s_ z%LZh1Tbdp)OpIga-Mzg@i#?#jQQu)wl&?K6zxH~2i~a?`Xk+kn^>bP0l+XkKfi%Vc literal 0 HcmV?d00001 diff --git a/frontend/src/assets/operator_images/TryCatch.png b/frontend/src/assets/operator_images/TryCatch.png new file mode 100644 index 0000000000000000000000000000000000000000..8530ca413e6af8fd4bf8afea20acaffcb6621d63 GIT binary patch literal 523 zcmeAS@N?(olHy`uVBq!ia0vp^2_VeD1|%QND7Ro>V7%n%;uumf=k1(}ea#92uBn^; z|396xssHRXEhl~B?{lUf(`dV=yiK7(2B?LhZ;P`2-b3YFrL1=x8zfjdh$T9c&Np8c zWIg=eU1<~Ni^=_yCwn@?*X(K0VfuA)-?T_45zhU*)20izuKQzFA*UJmX4_@9^Xkf7 zcRyeD>_~dJe6j%RVSiP{uD1DZP9l8vtt}dI|Go(wQuwo0wZ-$tZ7+_+AGS{sNPC#C zuE=xvzL(RCw*9>=27LA0oQZP#Vh&|7N*N|0a8Cn^M`+ zY;jJeBMJ^YaN+(0Was@v_ruAR%LI~5Oj=e|z<*eRsp^_tH_3_u>w`n)*Y&FrTq#hWHn`XRMY7bUols2SpIWgAI?x XpO*!$yixh?0*LSF>gTe~DWM4fB*f+8 literal 0 HcmV?d00001 From 665b1d1baf854809fe38340628e92aa87e93abfd Mon Sep 17 00:00:00 2001 From: carloea2 Date: Fri, 7 Aug 2026 11:54:57 -0600 Subject: [PATCH 02/10] feat(engine): let Finally result ports carry per-branch schemas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rows never cross the Merger's result ports (try rows leave through Try Result, catch rows through Catch Result), so requiring the two branches to produce the same schema was unnecessarily restrictive — it also made every downstream operator report "schema is not available" whenever the branches differed. Each result port now adopts its own branch's schema; wiring both ports into one downstream input remains a Union, which enforces compatibility itself. --- .../operator/trycatch/FinallyOpDesc.scala | 24 ++++----- .../trycatch/TryCatchOpDescSpec.scala | 50 ++++++------------- .../operators/control-block/finally.md | 6 ++- 3 files changed, 30 insertions(+), 50 deletions(-) diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala index d44d1dde8d9..6f1c15220c4 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyOpDesc.scala @@ -34,10 +34,11 @@ import org.apache.texera.amber.util.JSONUtils.objectMapper * decision is deterministic, never a race. * * The winner leaves through the port named after it: `Try Result` on success, - * `Catch Result` on failure (both carry the branches' common schema). The - * outcome is therefore observable downstream — connect only one port to react - * to that outcome, or connect both to the same downstream input (a Union) to - * get "the winner, whichever it was". + * `Catch Result` on failure. Rows never cross ports, so each result port + * carries its own branch's schema — the branches need not agree. The outcome + * is therefore observable downstream — connect only one port to react to + * that outcome, or (when the branches do share a schema) connect both to the + * same downstream input (a Union) to get "the winner, whichever it was". */ class FinallyOpDesc extends LogicalOp { @@ -59,15 +60,14 @@ class FinallyOpDesc extends LogicalOp { .withOutputPorts(operatorInfo.outputPorts) .withPropagateSchema( SchemaPropagationFunc(inputSchemas => { - val trySchema = inputSchemas(operatorInfo.inputPorts.head.id) - val catchSchema = inputSchemas(operatorInfo.inputPorts.last.id) - require( - trySchema == catchSchema, - s"Finally requires the try and catch branches to produce the same schema (got $trySchema vs $catchSchema)" - ) + // Each result port adopts its own branch's schema: try rows only + // ever leave through Try Result and catch rows through Catch + // Result, so the branches need not agree. Wiring both ports into + // one downstream input is a Union, which enforces schema + // compatibility itself, like any other Union. Map( - FinallyOpDesc.TRY_RESULT -> trySchema, - FinallyOpDesc.CATCH_RESULT -> trySchema + FinallyOpDesc.TRY_RESULT -> inputSchemas(operatorInfo.inputPorts.head.id), + FinallyOpDesc.CATCH_RESULT -> inputSchemas(operatorInfo.inputPorts.last.id) ) }) ) diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala index 9cc05b9bf04..b9405dcbbe2 100644 --- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/TryCatchOpDescSpec.scala @@ -70,28 +70,26 @@ class TryCatchOpDescSpec extends AnyFlatSpec with Matchers { List(FinallyOpDesc.TRY_RESULT, FinallyOpDesc.CATCH_RESULT) } - it should "require matching schemas and emit the common schema on both result ports" in { + it should "give each result port its own branch's schema" in { + // Rows never cross ports (try rows leave through Try Result, catch rows + // through Catch Result), so the branches need not agree on a schema — + // e.g. a try branch fetching web content can fall back to a catch branch + // producing plain lines. A downstream union of the two ports enforces + // compatibility itself, like any other Union. val desc = new FinallyOpDesc() - val out = desc.getExternalOutputSchemas( - Map(PortIdentity() -> schema, PortIdentity(1) -> schema) - ) - out shouldBe Map( + val catchSchema = Schema().add("other", AttributeType.STRING) + desc.getExternalOutputSchemas( + Map(PortIdentity() -> schema, PortIdentity(1) -> catchSchema) + ) shouldBe Map( FinallyOpDesc.TRY_RESULT -> schema, - FinallyOpDesc.CATCH_RESULT -> schema + FinallyOpDesc.CATCH_RESULT -> catchSchema ) - - val otherSchema = Schema().add("other", AttributeType.STRING) - assertThrows[Exception] { - desc.getExternalOutputSchemas( - Map(PortIdentity() -> schema, PortIdentity(1) -> otherSchema) - ) - } } it should "adopt whatever schema the connected branches carry" in { - // Finally does not impose a schema: it takes the one the user's two - // branches actually produce (both must agree) and emits exactly that, so - // the frame is transparent to whatever columns flow through it. + // Finally does not impose a schema: each port takes exactly what its + // branch produces, so the frame is transparent to whatever columns flow + // through it. val desc = new FinallyOpDesc() val wideSchema = Schema() .add("id", AttributeType.LONG) @@ -105,26 +103,6 @@ class TryCatchOpDescSpec extends AnyFlatSpec with Matchers { ) } - it should "reject branches whose columns differ only in type or order" in { - val desc = new FinallyOpDesc() - val a = Schema().add("x", AttributeType.INTEGER).add("y", AttributeType.STRING) - // same names, different type on x - val differentType = - Schema().add("x", AttributeType.LONG).add("y", AttributeType.STRING) - // same attributes, different order - val differentOrder = - Schema().add("y", AttributeType.STRING).add("x", AttributeType.INTEGER) - - val typeError = intercept[Exception] { - desc.getExternalOutputSchemas(Map(PortIdentity() -> a, PortIdentity(1) -> differentType)) - } - typeError.getMessage should include("same schema") - - assertThrows[Exception] { - desc.getExternalOutputSchemas(Map(PortIdentity() -> a, PortIdentity(1) -> differentOrder)) - } - } - "FinallyOpDesc" should "declare From Catch dependent on From Try" in { val desc = new FinallyOpDesc() val fromCatch = desc.operatorInfo.inputPorts.last diff --git a/docs/reference/operators/control-block/finally.md b/docs/reference/operators/control-block/finally.md index dfd2b9dee5b..f9135cad939 100644 --- a/docs/reference/operators/control-block/finally.md +++ b/docs/reference/operators/control-block/finally.md @@ -50,8 +50,10 @@ the code after it does not care which. downstream input** — a Union — to get "the winner, whichever it was". - **Deterministic outcome.** `From Catch` is consumed only after `From Try` fully resolves, so the release decision never depends on timing. -- **Both branches must produce the same schema** (same rule as Union) — both - result ports carry that common schema. +- **Each result port carries its own branch's schema.** Rows never cross + ports, so the branches need not agree — a try branch fetching web content + can fall back to a catch branch producing plain lines. Unioning the two + ports downstream requires compatible schemas, as with any Union. - **On double failure** (the catch branch fails too) Finally emits nothing and the failure escalates to any enclosing frame. - **Optional.** A Try Catch works without a Finally: use it when the branches From ce86b8576188f7db72f04aec2bc3aa4e7f8d55fd Mon Sep 17 00:00:00 2001 From: carloea2 Date: Fri, 7 Aug 2026 12:01:30 -0600 Subject: [PATCH 03/10] fix(pyamber): write the failure State to port state storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failing Python worker emitted its error State only through the network partitioners — but a try-cone tail's outgoing edges (a Finally's dependee From Try, a catch gate's dependee signal ports) are materialized and have no live partitioners, so the failure signal reached nobody: the gate never released the snapshot and the catch branch ran empty. Mirror the Scala worker's emitState, which always pairs the network emit with a write to the output port's state storage that the materialization readers replay. --- .../main/python/core/runnables/main_loop.py | 11 +++++-- .../python/core/runnables/test_main_loop.py | 30 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/amber/src/main/python/core/runnables/main_loop.py b/amber/src/main/python/core/runnables/main_loop.py index 426a670cd8e..7e5b4644289 100644 --- a/amber/src/main/python/core/runnables/main_loop.py +++ b/amber/src/main/python/core/runnables/main_loop.py @@ -699,10 +699,17 @@ def _emit_error_state(self) -> None: self.context.worker_id, err, ) - # Emit directly: _emit_and_save_state funnels back into _check_exception - # on failure, and the storage write is irrelevant for a failure signal. + # Emit directly rather than via _emit_and_save_state: that helper + # funnels its own failures back into _check_exception, which is the + # caller of this method. Both halves of the Scala worker's emitState + # are needed: the network buffers for live links, AND the port state + # storage for MATERIALIZED links — a try-cone tail's outgoing edges + # (a Finally's dependee `From Try`, a gate's dependee signal ports) + # are materialized and have no live partitioners at all, so storage + # is the only road the failure signal can travel to the gate. try: self._emit_batches(self.context.output_manager.emit_state(error_state)) + self.context.output_manager.save_state_to_storage_if_needed(error_state) except Exception as emit_err: # pragma: no cover - defensive logger.exception(emit_err) diff --git a/amber/src/test/python/core/runnables/test_main_loop.py b/amber/src/test/python/core/runnables/test_main_loop.py index 40733cf9dea..01c06ea9922 100644 --- a/amber/src/test/python/core/runnables/test_main_loop.py +++ b/amber/src/test/python/core/runnables/test_main_loop.py @@ -2189,6 +2189,36 @@ def test_unguarded_failure_pauses_like_before_frames(self, main_loop, monkeypatc assert not main_loop._self_failed assert not main_loop._is_port_poisoned(PortIdentity(id=0)) + @pytest.mark.timeout(2) + def test_error_state_reaches_materialized_state_storage( + self, main_loop, monkeypatch + ): + # A try-cone tail's outgoing edges (a Finally's dependee From Try, a + # gate's dependee signal ports) are MATERIALIZED: the worker has no + # live partitioners for them, so emit_state alone sends the failure + # signal to nobody. It must also be written to the port state storage + # that the materialization readers replay — mirroring the Scala + # worker's emitState, which always does both. + main_loop.context.guarded = True + monkeypatch.setattr(main_loop, "_send_console_message", lambda msg: None) + monkeypatch.setattr(main_loop, "_emit_batches", lambda batches: None) + saved = [] + monkeypatch.setattr( + main_loop.context.output_manager, + "save_state_to_storage_if_needed", + lambda state, *args, **kwargs: saved.append(state), + ) + + try: + raise RuntimeError("boom") + except RuntimeError: + exc_info = sys.exc_info() + main_loop.context.exception_manager.set_exception_info(exc_info) + main_loop._check_exception() + + assert len(saved) == 1 + assert saved[0].is_error() + @pytest.mark.timeout(2) def test_failed_worker_drains_instead_of_invoking_the_executor( self, main_loop, monkeypatch From c7fb142625320946013b48dd1800c448d759a9d2 Mon Sep 17 00:00:00 2001 From: carloea2 Date: Fri, 7 Aug 2026 12:22:46 -0600 Subject: [PATCH 04/10] fix(compiler): dedupe gate signal edges and enforce inside-out Finallys Two fixes for nested frames without their own Finally: - A nested frame's terminal catch leaf qualifies as both an enclosing-frame-owned tail and an escalation tap; wiring both signal edges materialized the same source port twice (signal ports are dependees), racing to create one storage table. The gate signal list is now deduplicated. e2e added: two sibling Finally-less inner frames recover independently inside an outer frame. - A frame without a Finally is terminal: if its subgraph flows into the Finally of a frame not nested inside it (Try1 -> Try2 -> Finally-of-1 with Try2 unclosed), the inner frame never closes -- its unbounded cone swallows the outer Merger and its gate goes blind to failures. Now rejected at compile time: Finallys close inside-out. --- .../engine/e2e/TryCatchIntegrationSpec.scala | 58 +++++++++++++ .../common/compiler/TryCatchFramePass.scala | 30 ++++++- .../compiler/TryCatchFramePassSpec.scala | 85 +++++++++++++++++++ 3 files changed, 171 insertions(+), 2 deletions(-) diff --git a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala index 7f958c0dc99..38bca0040ba 100644 --- a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala +++ b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala @@ -323,6 +323,64 @@ class TryCatchIntegrationSpec assert(portRowCount(outerFin, port1) == 1) } + it should "recover independently in sibling Finally-less inner frames (self-contained branches)" in { + // Two inner TryCatch frames WITHOUT Finallys, side by side inside an + // outer frame. A Finally-less frame is terminal: its branches end in + // their own result tables. Each inner frame owns its own failure (the + // innermost-frame rule), recovers independently, and a successful + // recovery is invisible to the outer frame — whose own try path and + // Finally proceed as a clean run. + val src = textInput("1\n2\n3") + val outer = new TryCatchOpDesc() + val outerTail = limit(3) // outer's own try path, feeds the outer Finally + val inner1 = new TryCatchOpDesc() + val inner1Try = failingFilter() // fails => inner1's catch replays + val inner1Catch = limit(2) // terminal: its result table is the recovery + val inner2 = new TryCatchOpDesc() + val inner2Try = limit(1) // clean => inner2's catch stays empty + val inner2Catch = limit(3) + val outerCatch = limit(1) + val fin = new FinallyOpDesc() + + val operators = List( + src, + outer, + outerTail, + inner1, + inner1Try, + inner1Catch, + inner2, + inner2Try, + inner2Catch, + outerCatch, + fin + ) + val links = List( + link(src, port0, outer, port0), + link(outer, port0, outerTail, port0), // outer try path + link(outer, port0, inner1, port0), // fan-out into inner frame 1 + link(outer, port0, inner2, port0), // fan-out into inner frame 2 + link(inner1, port0, inner1Try, port0), + link(inner1, port1, inner1Catch, port0), + link(inner2, port0, inner2Try, port0), + link(inner2, port1, inner2Catch, port0), + link(outerTail, port0, fin, port0), // outer From Try + link(outer, port1, outerCatch, port0), // outer Catch + link(outerCatch, port0, fin, port1) // outer From Catch + ) + val materialized = runAndGetMaterializedRowCounts(operators, links) + // inner1 failed and recovered: its catch replayed all 3 rows, limit(2) + assert(materialized(inner1Catch.operatorIdentifier) == 2) + // the failing branch's own table is empty + assert(materialized(inner1Try.operatorIdentifier) == 0) + // inner2 was clean: try table filled, catch ran empty + assert(materialized(inner2Try.operatorIdentifier) == 1) + assert(materialized(inner2Catch.operatorIdentifier) == 0) + // both inner outcomes are invisible to the outer frame: clean try side + assert(materialized(fin.operatorIdentifier) == 3) + assert(portRowCount(fin, port1) == 0) + } + it should "route the catch branch by error type: catch(SpecificError) via Error Info + If" in { // Simulates `catch (SpecificError e)`: a classifier UDF consumes the // frame's Error Info rows and emits a boolean State; an If routes the diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala index 46d08616906..822da501416 100644 --- a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala @@ -161,6 +161,28 @@ object TryCatchFramePass { // ---- 3. validations frames.foreach(validateFrame(plan, _)) + // Finallys close inside-out: a frame WITHOUT a Finally is terminal — its + // branches must end in their own sinks. If its cone flows into another + // frame's Finally (one that is not nested inside it), the inner frame + // never closes: its unbounded cone would swallow the outer Merger and + // everything past it, mis-owning failures and starving its own gate of + // signal edges. Reject with the rule's own words. + frames.filter(_.merger.isEmpty).foreach { open => + frames + .filter(other => + other.merger.exists(m => open.cone.contains(m.id)) && + !open.cone.contains(other.splitter.id) + ) + .foreach { other => + throw new IllegalArgumentException( + s"TryCatch '${open.logicalOpId.id}' has no Finally, but its subgraph flows into " + + s"the Finally of '${other.logicalOpId.id}'. A frame without a Finally must end " + + s"in its own sinks — add a Finally to '${open.logicalOpId.id}' to continue past " + + "it (Finallys close inside-out)." + ) + } + } + // ---- 4. per-op innermost-frame assignment (smallest containing cone) def innermostFrameOf(opId: PhysicalOpIdentity): Option[Frame] = frames.filter(_.tryCone.contains(opId)).sortBy(_.tryCone.size).headOption @@ -230,8 +252,12 @@ object TryCatchFramePass { // ---- 5. rebuild the plan with reconfigured gates/mergers + signal links val gateSignals: Map[PhysicalOpIdentity, List[(PhysicalOpIdentity, PortIdentity)]] = frames.map { frame => - val signals = signalSources(frame.logicalOpId) ++ - escalationSources.getOrElse(frame.logicalOpId, List.empty) + // distinct: a nested frame's TERMINAL catch leaf is both an + // enclosing-frame-owned tail (signal source) and an escalation tap. + // Wiring it twice would materialize the same source port twice + // (signal ports are dependees), racing to create one storage table. + val signals = (signalSources(frame.logicalOpId) ++ + escalationSources.getOrElse(frame.logicalOpId, List.empty)).distinct frame.gate.id -> signals }.toMap diff --git a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala index 49b9c2978f8..85178e5fb79 100644 --- a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala +++ b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala @@ -367,6 +367,91 @@ class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { config.ownConeOpIds should not contain "side/main" } + it should "signal a nested Finally-less frame's terminal catch leaf exactly once" in { + // A terminal catch leaf of a nested (Finally-less) frame qualifies as an + // enclosing-frame-owned tail AND as an escalation tap. It must get ONE + // signal edge, not two: signal ports are dependees, dependee edges + // materialize their source port, and a duplicate would race to create + // the same storage table (iceberg commit conflict). + val tc2Desc = new TryCatchOpDesc() + tc2Desc.setOperatorId("tc2") + val tc2Plan = tc2Desc.getPhysicalPlan(wid, eid) + val splitter2Id = opId("tc2", TryCatchOpDesc.SPLITTER_LAYER) + val gate2Id = opId("tc2", TryCatchOpDesc.GATE_LAYER) + + var plan = buildFramePlan() + (Set(simpleOp("tryOp2"), simpleOp("catchOp2")) ++ tc2Plan.operators).foreach { op => + plan = plan.addOperator(op.propagateSchema()) + } + List( + // the outer Try also feeds a nested frame whose branches are terminal + link(opId("tc", TryCatchOpDesc.SPLITTER_LAYER), out0, splitter2Id, in0), + tc2Plan.links.head, // splitter2 -> gate2 snapshot + link(splitter2Id, out0, opId("tryOp2"), in0), + link(gate2Id, catchPort, opId("catchOp2"), in0) + ).foreach(l => plan = plan.addLink(l)) + + val result = TryCatchFramePass.run(plan, None) + val outerGate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + val fromCatchLeaf = result.links.filter(l => + l.fromOpId == opId("catchOp2") && l.toOpId == outerGate.id + ) + fromCatchLeaf should have size 1 + // inner try tail signals the inner gate, not the outer one + val innerGate = result.getOperator(gate2Id) + result.links.count(l => l.fromOpId == opId("tryOp2") && l.toOpId == innerGate.id) shouldBe 1 + result.links.count(l => l.fromOpId == opId("tryOp2") && l.toOpId == outerGate.id) shouldBe 0 + } + + it should "reject a Finally-less frame flowing into an enclosing frame's Finally (close inside-out)" in { + // Try1 -> Try2 -> Finally(of 1): the inner frame never closes — its + // unbounded cone would swallow the outer Merger, starving its own gate + // of signal edges (failures in its try branch would be invisible) and + // mis-owning everything past the Finally. Must be rejected, telling the + // user to close the inner frame first. + val tc1Desc = new TryCatchOpDesc() + tc1Desc.setOperatorId("tc1") + val tc2Desc = new TryCatchOpDesc() + tc2Desc.setOperatorId("tc2") + val finDesc = new FinallyOpDesc() + finDesc.setOperatorId("fin1") + val tc1Plan = tc1Desc.getPhysicalPlan(wid, eid) + val tc2Plan = tc2Desc.getPhysicalPlan(wid, eid) + val merger = finDesc.getPhysicalOp(wid, eid) + val splitter1Id = opId("tc1", TryCatchOpDesc.SPLITTER_LAYER) + val gate1Id = opId("tc1", TryCatchOpDesc.GATE_LAYER) + val splitter2Id = opId("tc2", TryCatchOpDesc.SPLITTER_LAYER) + val gate2Id = opId("tc2", TryCatchOpDesc.GATE_LAYER) + + var plan = PhysicalPlan(operators = Set.empty, links = Set.empty) + (Set( + sourceOp("src"), + simpleOp("tryOp"), + simpleOp("catchOp2"), + simpleOp("catchOp"), + merger + ) ++ tc1Plan.operators ++ tc2Plan.operators).foreach { op => + plan = plan.addOperator(op.propagateSchema()) + } + List( + link(opId("src"), out0, splitter1Id, in0), + tc1Plan.links.head, + tc2Plan.links.head, + link(splitter1Id, out0, splitter2Id, in0), // Try1 -> Try2 + link(splitter2Id, out0, opId("tryOp"), in0), // Try2's try branch... + link(opId("tryOp"), out0, merger.id, PortIdentity()), // ...into Finally(of 1)! + link(gate2Id, catchPort, opId("catchOp2"), in0), // Try2's catch (terminal) + link(gate1Id, catchPort, opId("catchOp"), in0), // Try1's catch + link(opId("catchOp"), out0, merger.id, PortIdentity(1)) + ).foreach(l => plan = plan.addLink(l)) + + val rejected = intercept[IllegalArgumentException] { + TryCatchFramePass.run(plan, None) + } + rejected.getMessage should include("inside-out") + rejected.getMessage should include("tc2") + } + it should "reject a Finally whose From Try comes from outside the frame" in { val tcDesc = new TryCatchOpDesc() tcDesc.setOperatorId("tc") From 2c70d632405eb9c3d3cff16af146b87f752c0c9c Mon Sep 17 00:00:00 2001 From: carloea2 Date: Fri, 7 Aug 2026 12:31:13 -0600 Subject: [PATCH 05/10] test(engine): pin an inner frame inside the catch branch Cover the PL shape `try1 {} catch1 { try2 {} catch2 {} } finally1`: the outer recovery is itself guarded, the inner construct closes with its own Finally, and both result ports union into the outer From Catch. With both attempts failing, the inner recovery becomes the outer construct's value. --- .../engine/e2e/TryCatchIntegrationSpec.scala | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala index 38bca0040ba..2cc75569e0c 100644 --- a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala +++ b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala @@ -323,6 +323,44 @@ class TryCatchIntegrationSpec assert(portRowCount(outerFin, port1) == 1) } + it should "support an inner frame inside the CATCH branch: try1 {} catch1 { try2 {} catch2 {} } finally1" in { + // The PL shape `try1 { attempt } catch1 { try2 { A } catch2 { B } } finally1`: + // the outer recovery is itself guarded. The inner construct's closing + // brace is its own Finally, whose result ports union into the outer + // From Catch. Here BOTH attempts fail, so the rows that reach the outer + // Finally are the inner frame's recovery — a catch inside a catch. + val src = textInput("1\n2\n3") + val outer = new TryCatchOpDesc() + val outerTry = failingFilter() // outer attempt fails => catch1 runs + val inner = new TryCatchOpDesc() // catch1's body IS an inner frame + val innerTry = failingFilter() // inner attempt fails too => catch2 runs + val innerCatch = limit(2) + val innerFin = new FinallyOpDesc() + val outerFin = new FinallyOpDesc() + + val operators = + List(src, outer, outerTry, inner, innerTry, innerCatch, innerFin, outerFin) + val links = List( + link(src, port0, outer, port0), + link(outer, port0, outerTry, port0), // try1 + link(outerTry, port0, outerFin, port0), // From Try (poisoned, discarded) + link(outer, port1, inner, port0), // catch1 = inner frame's input + link(inner, port0, innerTry, port0), // try2 + link(inner, port1, innerCatch, port0), // catch2 + link(innerTry, port0, innerFin, port0), + link(innerCatch, port0, innerFin, port1), + // inner closing brace: winner (whichever side) -> outer From Catch + link(innerFin, port0, outerFin, port1), + link(innerFin, port1, outerFin, port1) + ) + val materialized = runAndGetMaterializedRowCounts(operators, links) + // outer attempt failed; the recovery's own attempt failed; the inner + // catch replayed all 3 rows through limit(2) => the outer construct's + // value is those 2 rows, out its Catch Result port + assert(materialized(outerFin.operatorIdentifier) == 0) + assert(portRowCount(outerFin, port1) == 2) + } + it should "recover independently in sibling Finally-less inner frames (self-contained branches)" in { // Two inner TryCatch frames WITHOUT Finallys, side by side inside an // outer frame. A Finally-less frame is terminal: its branches end in From 7fdf6e74f0c3d191d4d319b5734bd7ca99ae3a13 Mon Sep 17 00:00:00 2001 From: carloea2 Date: Fri, 7 Aug 2026 12:37:07 -0600 Subject: [PATCH 06/10] style: apply scalafmt to the try/catch sources --- .../worker/DataProcessorSpec.scala | 3 +- .../common/compiler/TryCatchFramePass.scala | 43 +++++++++---------- .../compiler/TryCatchFramePassSpec.scala | 11 ++--- 3 files changed, 26 insertions(+), 31 deletions(-) diff --git a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala index 5074e3fc882..0bdec5e99ba 100644 --- a/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala +++ b/amber/src/test/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessorSpec.scala @@ -556,7 +556,8 @@ class DataProcessorSpec extends AnyFlatSpec with MockFactory with Matchers with m.foreach(emitted += _); () } .anyNumberOfTimes() - val errorState = State.errorState("upstreamOp/main", "some-worker", new RuntimeException("boom")) + val errorState = + State.errorState("upstreamOp/main", "some-worker", new RuntimeException("boom")) // the executor SEES the error State (frame operators react to it); the // default pass-through contract forwards it downstream ( diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala index 822da501416..46eb7840f96 100644 --- a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala @@ -190,27 +190,25 @@ object TryCatchFramePass { // tail ports of a frame's try cone: no outgoing links, or feeding the // paired merger's From Try — owned by the innermost frame only val signalSources: Map[OperatorIdentity, List[(PhysicalOpIdentity, PortIdentity)]] = - frames - .map { frame => - val tails = frame.tryCone.toList - .filter(opId => innermostFrameOf(opId).exists(_.logicalOpId == frame.logicalOpId)) - .flatMap { opId => - val op = plan.getOperator(opId) - op.outputPorts.keys - .filterNot(_.internal) - .filter { portId => - val outLinks = plan.links.filter(l => l.fromOpId == opId && l.fromPortId == portId) - outLinks.isEmpty || - frame.merger.exists(m => - outLinks.exists(l => l.toOpId == m.id && l.toPortId == FROM_TRY) - ) - } - .map(portId => (opId, portId)) - } - .sortBy { case (opId, portId) => (opId.logicalOpId.id, opId.layerName, portId.id) } - frame.logicalOpId -> tails - } - .toMap + frames.map { frame => + val tails = frame.tryCone.toList + .filter(opId => innermostFrameOf(opId).exists(_.logicalOpId == frame.logicalOpId)) + .flatMap { opId => + val op = plan.getOperator(opId) + op.outputPorts.keys + .filterNot(_.internal) + .filter { portId => + val outLinks = plan.links.filter(l => l.fromOpId == opId && l.fromPortId == portId) + outLinks.isEmpty || + frame.merger.exists(m => + outLinks.exists(l => l.toOpId == m.id && l.toPortId == FROM_TRY) + ) + } + .map(portId => (opId, portId)) + } + .sortBy { case (opId, portId) => (opId.logicalOpId.id, opId.layerName, portId.id) } + frame.logicalOpId -> tails + }.toMap // escalation: catch-cone TERMINAL leaves signal the innermost enclosing // frame's gate, so catch-side failures escalate even when their branch @@ -349,7 +347,8 @@ object TryCatchFramePass { * acyclic by construction: frames only add leaf->gate and escalation edges, * and try/catch cones are validated disjoint). Any operator left in a cycle * is appended last so a malformed graph fails in the compiler's own checks - * rather than here. */ + * rather than here. + */ private def topologicalIndex( opIds: Set[PhysicalOpIdentity], links: Set[PhysicalLink] diff --git a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala index 85178e5fb79..6c4b97262ac 100644 --- a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala +++ b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala @@ -28,11 +28,7 @@ import org.apache.texera.amber.core.virtualidentity.{ WorkflowIdentity } import org.apache.texera.amber.core.workflow._ -import org.apache.texera.amber.operator.trycatch.{ - CatchGateConfig, - FinallyOpDesc, - TryCatchOpDesc -} +import org.apache.texera.amber.operator.trycatch.{CatchGateConfig, FinallyOpDesc, TryCatchOpDesc} import org.apache.texera.amber.util.JSONUtils.objectMapper import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -393,9 +389,8 @@ class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { val result = TryCatchFramePass.run(plan, None) val outerGate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) - val fromCatchLeaf = result.links.filter(l => - l.fromOpId == opId("catchOp2") && l.toOpId == outerGate.id - ) + val fromCatchLeaf = + result.links.filter(l => l.fromOpId == opId("catchOp2") && l.toOpId == outerGate.id) fromCatchLeaf should have size 1 // inner try tail signals the inner gate, not the outer one val innerGate = result.getOperator(gate2Id) From 2b9c468ad1f3483cdcd995b26bcc4f2483d9740f Mon Sep 17 00:00:00 2001 From: carloea2 Date: Fri, 7 Aug 2026 13:18:50 -0600 Subject: [PATCH 07/10] fix(operator): narrow the If unknown-State guard to error States only The guard added for try/catch frames skipped routing for ANY state missing the configured conditionName, which silently dropped the existing contract that a missing key throws NoSuchElementException -- the safeguard against a typo'd conditionName quietly misrouting every tuple to the default port (pinned by IfOpExecSpec, and caught by CI). Frames only need error States to pass through, so exempt exactly those: a failure traveling downstream as a dataflow event must not become a second failure inside an If, while any other state missing conditionName still fails loudly. Adds a test pinning that an error State is forwarded without disturbing the branch already chosen. --- .../amber/operator/ifStatement/IfOpExec.scala | 13 +++++++++---- .../operator/ifStatement/IfOpExecSpec.scala | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala index de5bf3f75e7..4a1469b7a7d 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/ifStatement/IfOpExec.scala @@ -32,11 +32,16 @@ class IfOpExec(descString: String) extends OperatorExecutor { //This function can handle one or more states. //The state can have mutiple key-value pairs. Keys are not identified by conditionName will be ignored. //It can accept any value that can be converted to a boolean. For example, Int 1 will be converted to true. - //States that do not carry conditionName at all (e.g. error States, loop envelopes) - //are passed through without touching the routing decision. override def processState(state: State, port: Int): Option[State] = { - state.values.get(desc.conditionName).foreach { value => - outputPort = if (value.asInstanceOf[Boolean]) PortIdentity(1) else PortIdentity() + // An error State is a failure traveling as a dataflow event, not a + // routing decision: forward it untouched so a failure upstream of an If + // does not become a second failure inside the If. Any OTHER state is + // expected to carry conditionName, and a missing key still surfaces as + // NoSuchElementException rather than a quiet misroute. + if (!State.isError(state)) { + outputPort = + if (state.values(desc.conditionName).asInstanceOf[Boolean]) PortIdentity(1) + else PortIdentity() } Some(state) } diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/ifStatement/IfOpExecSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/ifStatement/IfOpExecSpec.scala index 0fe3294fc87..04563e75f15 100644 --- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/ifStatement/IfOpExecSpec.scala +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/ifStatement/IfOpExecSpec.scala @@ -118,6 +118,24 @@ class IfOpExecSpec extends AnyFlatSpec { } } + it should "forward an error State untouched, keeping the routing decision" in { + // A failure travels downstream as a State carrying the reserved error + // key. It is not a routing decision and does not carry conditionName, so + // the If must pass it through without throwing (a failure upstream of an + // If must not become a second failure inside the If) and without + // disturbing the branch already chosen. + val exec = new IfOpExec(desc("flag")) + exec.processState(State(Map[String, Any]("flag" -> false)), 0) + + val errorState = State.errorState("someop/main", "worker-0", new RuntimeException("boom")) + val forwarded = exec.processState(errorState, 0) + assert(forwarded.contains(errorState)) + + // still routed by the last real decision, not reset to the default + val out = exec.processTupleMultiPort(tuple(1), 0).toList + assert(out == List((tuple(1), Some(falsePortId)))) + } + it should "treat a null condition value as false (default Boolean unbox)" in { val exec = new IfOpExec(desc("flag")) // `null.asInstanceOf[Boolean]` quietly unboxes to `false` in Scala, so From 9e1fd01d9e347cc4c5590b95d5425f79a25a6fa9 Mon Sep 17 00:00:00 2001 From: carloea2 Date: Tue, 11 Aug 2026 00:30:44 -0600 Subject: [PATCH 08/10] fix(compiler): cut a nested frame's cone at the enclosing Finally A TryCatch without its own Finally is not necessarily terminal: it may be a whole frame nested in another's block, which is ordinary PL -- try1 { .. } catch1 { try2 { .. } catch2 { .. } finally2 { } } finally1 { }. Its cone must end at the enclosing frame's Finally, the dataflow reading of "an inner block's scope ends no later than the enclosing block's". Without that cut the unclosed inner cone swallowed the enclosing Merger and everything past it: it mis-owned those failures, its try and catch cones overlapped out there, and -- worst -- its own try tail stopped qualifying as a tail, so its gate got zero signal edges and its catch branch could never fire. Enclosure is itself defined by the cones, so they are now solved by fixpoint: start cut at own Mergers, then repeatedly cut each frame at the Mergers of frames it does not contain (a nested frame's Merger is interior and must stay reachable). Cutting only shrinks cones and a smaller cone only adds cuts, so the iteration settles. A tail is now "a port whose output leaves the try cone" -- no consumer, or every consumer at the frame boundary -- generalizing the old rule that only recognized a frame's own Merger. The inside-out validation is narrowed accordingly: what a Finally-less frame must not do is feed an ENCLOSING Finally from its TRY cone, since an attempt streams rows downstream before it fails and those partial rows would be released mixed with the recovery. Its catch cone may feed the enclosing Finally freely, and an unclosed OUTER frame feeding a nested frame's Finally is fine. The error message now names the leak and the remedy. --- .../common/compiler/TryCatchFramePass.scala | 123 +++++++++++++----- .../compiler/TryCatchFramePassSpec.scala | 118 +++++++++++++++++ 2 files changed, 208 insertions(+), 33 deletions(-) diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala index 46eb7840f96..2f6891ea56e 100644 --- a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala @@ -145,50 +145,110 @@ object TryCatchFramePass { } }.toMap - // ---- 2. cones (ownership; inclusive of nested frames, cut at own merger) - val frames: List[Frame] = splitters.map { s => - val gate = gates(s.id.logicalOpId) - val merger = - mergerToFrame.collectFirst { - case (mergerId, frameId) if frameId == s.id.logicalOpId => plan.getOperator(mergerId) - } - val stop = merger.map(_.id).toSet - val tryCone = reach(plan, s.id, TRY_PORT, crossInternal = true, stopAt = stop) - val catchCone = reach(plan, gate.id, CATCH_PORT, crossInternal = true, stopAt = stop) - Frame(s, gate, merger, tryCone, catchCone) + // ---- 2. cones (ownership; inclusive of nested frames) + // + // A frame's cone ends at its own Finally. A frame WITHOUT one ends at the + // Finally of whichever frame encloses it — the dataflow reading of "an + // inner block's scope ends no later than the enclosing block's". Without + // that cut an unclosed inner frame swallows the enclosing Merger and + // everything past it: it mis-owns those operators' failures, its try and + // catch cones overlap out there, and its own tails stop looking like + // tails, so its gate never hears about a failure and its catch branch + // never fires. + // + // Enclosure is itself defined by the cones, so solve by fixpoint: start + // with every frame cut only at its own Merger, then repeatedly cut each + // frame at the Mergers of the frames it does NOT contain (a Merger of a + // frame nested inside this one is interior and must stay reachable). + // Cutting only shrinks cones, and a smaller cone only adds cuts, so the + // iteration decreases monotonically and settles. + val mergerIdOf: Map[OperatorIdentity, PhysicalOpIdentity] = + mergerToFrame.map { case (mergerId, frameId) => frameId -> mergerId } + + def conesCutAt(stopFor: OperatorIdentity => Set[PhysicalOpIdentity]): List[Frame] = + splitters.map { s => + val frameId = s.id.logicalOpId + val gate = gates(frameId) + val stop = stopFor(frameId) + Frame( + s, + gate, + mergerIdOf.get(frameId).map(plan.getOperator), + reach(plan, s.id, TRY_PORT, crossInternal = true, stopAt = stop), + reach(plan, gate.id, CATCH_PORT, crossInternal = true, stopAt = stop) + ) + } + + var frames: List[Frame] = conesCutAt(id => mergerIdOf.get(id).toSet) + var settled = false + var round = 0 + while (!settled && round <= splitters.size) { + round += 1 + val byId = frames.map(f => f.logicalOpId -> f).toMap + val next = conesCutAt { id => + val self = byId(id) + mergerIdOf.get(id).toSet ++ mergerIdOf.collect { + case (otherId, mergerId) + if otherId != id && !self.cone.contains(byId(otherId).splitter.id) => + mergerId + }.toSet + } + def shape(fs: List[Frame]) = fs.map(f => (f.logicalOpId, f.tryCone, f.catchCone)) + settled = shape(next) == shape(frames) + frames = next } // ---- 3. validations frames.foreach(validateFrame(plan, _)) - // Finallys close inside-out: a frame WITHOUT a Finally is terminal — its - // branches must end in their own sinks. If its cone flows into another - // frame's Finally (one that is not nested inside it), the inner frame - // never closes: its unbounded cone would swallow the outer Merger and - // everything past it, mis-owning failures and starving its own gate of - // signal edges. Reject with the rule's own words. + // A frame without its own Finally may sit inside another frame's block — + // that is ordinary nesting (`try { .. } catch { try { .. } catch { .. } }`) + // and its cone is cut at the enclosing Finally above. What it must NOT do + // is feed that enclosing Finally from its TRY cone: an attempt streams + // rows downstream as it goes and only then fails, so those partial rows + // would already be staged at the enclosing reconvergence point and get + // released mixed with (or in place of) the recovery — breaking the + // all-or-nothing contract the Finally exists to provide. Staging and + // discarding a failed attempt is precisely a Finally's job, so the inner + // construct needs one of its own. Its CATCH cone may feed the enclosing + // Finally freely: those rows ARE the recovery, and if the catch fails too + // the error travels on as escalation, which is the correct outcome. frames.filter(_.merger.isEmpty).foreach { open => frames - .filter(other => - other.merger.exists(m => open.cone.contains(m.id)) && - !open.cone.contains(other.splitter.id) - ) + // only an ENCLOSING frame's Finally is a problem. A Finally belonging + // to a frame nested INSIDE this one is interior to its block, and an + // unclosed outer frame feeding it is ordinary (`try1 { try2 { .. } + // catch2 { .. } finally2 { } .. }` with try1 itself terminal). + .filter(other => other.cone.contains(open.splitter.id)) .foreach { other => - throw new IllegalArgumentException( - s"TryCatch '${open.logicalOpId.id}' has no Finally, but its subgraph flows into " + - s"the Finally of '${other.logicalOpId.id}'. A frame without a Finally must end " + - s"in its own sinks — add a Finally to '${open.logicalOpId.id}' to continue past " + - "it (Finallys close inside-out)." - ) + other.merger.foreach { m => + val leaking = plan.links + .filter(l => l.toOpId == m.id && open.tryCone.contains(l.fromOpId)) + .map(_.fromOpId) + if (leaking.nonEmpty) { + throw new IllegalArgumentException( + s"TryCatch '${open.logicalOpId.id}' has no Finally of its own, but its Try " + + s"subgraph feeds the Finally of '${other.logicalOpId.id}' (via " + + s"${leaking.map(_.logicalOpId.id).mkString(", ")}). A failed attempt would leak " + + s"its partial rows into that reconvergence point: give '${open.logicalOpId.id}' " + + "its own Finally and wire that into " + + s"'${other.logicalOpId.id}' instead (Finallys close inside-out)." + ) + } } + } } // ---- 4. per-op innermost-frame assignment (smallest containing cone) def innermostFrameOf(opId: PhysicalOpIdentity): Option[Frame] = frames.filter(_.tryCone.contains(opId)).sortBy(_.tryCone.size).headOption - // tail ports of a frame's try cone: no outgoing links, or feeding the - // paired merger's From Try — owned by the innermost frame only + // tail ports of a frame's try cone — owned by the innermost frame only. + // A tail is a port whose output LEAVES the try cone: it ends the attempt + // (no consumer at all), or every consumer sits at the frame boundary — + // the frame's own Merger, or the enclosing frame's, both of which the + // cone is cut at. A port feeding another cone operator is interior, so + // the failure would still be travelling and the attempt is not over. val signalSources: Map[OperatorIdentity, List[(PhysicalOpIdentity, PortIdentity)]] = frames.map { frame => val tails = frame.tryCone.toList @@ -199,10 +259,7 @@ object TryCatchFramePass { .filterNot(_.internal) .filter { portId => val outLinks = plan.links.filter(l => l.fromOpId == opId && l.fromPortId == portId) - outLinks.isEmpty || - frame.merger.exists(m => - outLinks.exists(l => l.toOpId == m.id && l.toPortId == FROM_TRY) - ) + !outLinks.exists(l => frame.tryCone.contains(l.toOpId)) } .map(portId => (opId, portId)) } diff --git a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala index 6c4b97262ac..eed5a481a5a 100644 --- a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala +++ b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala @@ -447,6 +447,124 @@ class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { rejected.getMessage should include("tc2") } + /** + * `try1 { tryOp } catch1 { try2 { try2op } catch2 { catch2op } } + * finally1 { }` — a whole frame nested inside the enclosing frame's CATCH + * block. `inner` is the nested frame's own Finally when `withInnerFinally`, + * otherwise its branches are wired straight into Finally1's From Catch. + */ + private def nestedInCatchPlan(withInnerFinally: Boolean): PhysicalPlan = { + val tc2Desc = new TryCatchOpDesc() + tc2Desc.setOperatorId("tc2") + val tc2Plan = tc2Desc.getPhysicalPlan(wid, eid) + val splitter2Id = opId("tc2", TryCatchOpDesc.SPLITTER_LAYER) + val gate2Id = opId("tc2", TryCatchOpDesc.GATE_LAYER) + val gate1Id = opId("tc", TryCatchOpDesc.GATE_LAYER) + + // start from the standard frame, then REPLACE its catch branch with the + // nested frame + var plan = buildFramePlan() + plan = plan.removeLink(link(opId("catchOp"), out0, opId("fin"), PortIdentity(1))) + plan = plan.removeLink(link(gate1Id, catchPort, opId("catchOp"), in0)) + (Set(simpleOp("try2op"), simpleOp("catch2op")) ++ tc2Plan.operators).foreach { op => + plan = plan.addOperator(op.propagateSchema()) + } + List( + link(gate1Id, catchPort, splitter2Id, in0), + tc2Plan.links.head, // splitter2 -> gate2 snapshot + link(splitter2Id, out0, opId("try2op"), in0), + link(gate2Id, catchPort, opId("catch2op"), in0) + ).foreach(l => plan = plan.addLink(l)) + + if (withInnerFinally) { + val fin2Desc = new FinallyOpDesc() + fin2Desc.setOperatorId("fin2") + val merger2 = fin2Desc.getPhysicalOp(wid, eid) + plan = plan.addOperator(merger2.propagateSchema()) + List( + link(opId("try2op"), out0, merger2.id, PortIdentity()), + link(opId("catch2op"), out0, merger2.id, PortIdentity(1)), + link(merger2.id, out0, opId("fin"), PortIdentity(1)) // inner result -> From Catch + ).foreach(l => plan = plan.addLink(l)) + } else { + List( + link(opId("try2op"), out0, opId("fin"), PortIdentity(1)), + link(opId("catch2op"), out0, opId("fin"), PortIdentity(1)) + ).foreach(l => plan = plan.addLink(l)) + } + plan + } + + it should "accept a whole frame nested inside the enclosing frame's CATCH block" in { + // Legal PL: try1 { .. } catch1 { try2 { .. } catch2 { .. } finally2 { } } + // finally1 { }. The inner frame opens and closes inside catch1, so nothing + // is unbalanced — and the inner frame must come out ARMED: its cone cut at + // its own Finally (never swallowing the enclosing one), and its try tail + // signalling its OWN gate so an inner failure fires catch2. + val result = TryCatchFramePass.run(nestedInCatchPlan(withInnerFinally = true), None) + val innerGate = result.getOperator(opId("tc2", TryCatchOpDesc.GATE_LAYER)) + val outerGate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + + result.links.count(l => l.fromOpId == opId("try2op") && l.toOpId == innerGate.id) shouldBe 1 + result.links.count(l => l.fromOpId == opId("try2op") && l.toOpId == outerGate.id) shouldBe 0 + + // the inner cone stops at its own Finally: neither the enclosing Merger + // nor the region past it is the inner frame's business + val innerCone = gateConfig(innerGate).ownConeOpIds + innerCone should contain("try2op/main") + innerCone should contain("catch2op/main") + innerCone should not contain "fin/main" + innerCone should not contain "down/main" + innerCone should not contain "fin2/main" + + // the enclosing frame owns the whole nested block, inner Finally included + // (inclusive ownership), but its try side is still just its own branch + val outerCone = gateConfig(outerGate).ownConeOpIds + outerCone should contain("tryOp/main") + outerCone should contain("try2op/main") + outerCone should contain("fin2/main") + outerCone should not contain "down/main" + // exactly one signal port each: the outer try tail and the inner try tail + outerGate.inputPorts.keys.count(p => p.internal && p.id > 0) shouldBe 1 + innerGate.inputPorts.keys.count(p => p.internal && p.id > 0) shouldBe 1 + } + + it should "reject a nested frame whose TRY branch feeds the enclosing Finally directly" in { + // Same nesting, but with NO inner Finally: try2op streams rows into + // Finally1's From Catch as it goes, and only then fails. Those partial + // rows are already staged at the enclosing reconvergence point, so the + // recovery would be released mixed with a failed attempt's output — + // exactly what all-or-nothing forbids. Staging and discarding a failed + // attempt IS a Finally's job: the inner construct needs its own. + val rejected = intercept[IllegalArgumentException] { + TryCatchFramePass.run(nestedInCatchPlan(withInnerFinally = false), None) + } + rejected.getMessage should include("tc2") + rejected.getMessage should include("leak") + rejected.getMessage should include("its own Finally") + } + + it should "require the enclosing Finally's From Catch to come from its own block, not through a nested frame's gate" in { + // Documented limitation, pinned as real behavior. Wiring ONLY a nested + // frame's catch branch into the enclosing Finally (its try branch ending + // in its own sink) dodges the partial-row leak, but the enclosing Finally + // can no longer be paired: frame pairing refuses to traverse + // frame-internal (snapshot) links -- which is what makes pairing unique + // under nesting -- so a nested frame's catch branch, reachable only + // through that frame's own gate, is invisible to it. The shape is also + // semantically thin: the enclosing catch block would contribute nothing + // whenever the inner attempt succeeds. The fix for a user is the same as + // above -- close the inner frame with its own Finally and wire THAT into + // the enclosing one -- so the pairing error points the right way. + var plan = nestedInCatchPlan(withInnerFinally = false) + plan = plan.removeLink(link(opId("try2op"), out0, opId("fin"), PortIdentity(1))) + + val rejected = intercept[IllegalArgumentException] { + TryCatchFramePass.run(plan, None) + } + rejected.getMessage should include("not wired to any TryCatch") + } + it should "reject a Finally whose From Try comes from outside the frame" in { val tcDesc = new TryCatchOpDesc() tcDesc.setOperatorId("tc") From 21cc777cd548245b77d0fa43d562a9a6d72377ad Mon Sep 17 00:00:00 2001 From: carloea2 Date: Tue, 11 Aug 2026 01:21:53 -0600 Subject: [PATCH 09/10] feat(engine): complete nested-frame failure semantics Three completions for nested try/catch frames: - Real rethrow for a frame with an unconnected Catch: the gate holds an own-cone error through the signal phase (dependee ports run before the operator has output ports, so emitting there goes nowhere) and rethrows it at the snapshot lane's finish, where it travels the gate's dangling ports' synthesized signal edges to the enclosing gate and Merger. No Error Info row is recorded for a rethrown error -- the report belongs to whoever catches it -- and Error Info consumers on a catch-less frame are rejected at compile time (the table could never hold a row). - The Merger now sees the same decision evidence as the gate, via internal dependee signal ports (SignalPartitioning, ids from 2) for every cone ending not wired into its data ports. A failure on an unwired try ending discards the staged attempt (caught, absorbed); a failure on an unwired catch ending suppresses the release and forwards the error (escalation). Previously the two could disagree: the gate releasing the catch replay while the Merger flushed a clean wired fork's rows as a success, or the Merger releasing a recovery whose terminal fork had died. - Two-pass pairing: a Finally fed only through a nested frame's gate (catch1 { try2 { sideEffect } catch2 { B } } finally1, with B alone continuing -- "the recovery emits rows only when its own inner attempt failed") could not be paired by the strict traversal. A second pass, run only for Finallys the strict pass cannot see, crosses frame-internal links while stopping at every other Merger, which keeps chain shapes unique while try/catch disjointness keeps genuine nesting unique. e2e additions: nested rethrow caught by the outer frame; terminal try-fork failure releasing the recovery; terminal catch-fork failure releasing nothing; the catch-only nested recovery pattern; a frame recovering on every iteration of a loop around it; and a failing Error Info branch leaving the Finally's recovery intact. --- .../engine/e2e/TryCatchIntegrationSpec.scala | 214 +++++++++++++++++- .../common/compiler/TryCatchFramePass.scala | 166 +++++++++++++- .../compiler/TryCatchFramePassSpec.scala | 157 +++++++++++-- .../operator/trycatch/CatchGateConfig.scala | 8 + .../operator/trycatch/CatchGateOpExec.scala | 28 ++- .../trycatch/FinallyMergerConfig.scala | 12 + .../trycatch/FinallyMergerOpExec.scala | 49 +++- .../trycatch/CatchGateOpExecSpec.scala | 46 +++- .../trycatch/FinallyMergerOpExecSpec.scala | 47 +++- 9 files changed, 687 insertions(+), 40 deletions(-) diff --git a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala index 2cc75569e0c..2745f0417b7 100644 --- a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala +++ b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala @@ -28,7 +28,11 @@ import org.apache.texera.amber.core.storage.DocumentFactory import org.apache.texera.amber.core.storage.model.VirtualDocument import org.apache.texera.amber.core.tuple.Tuple import org.apache.texera.amber.core.virtualidentity.{ExecutionIdentity, OperatorIdentity} -import org.apache.texera.amber.core.workflow.PortIdentity +import org.apache.texera.amber.core.workflow.{ + ExecutionMode, + PortIdentity, + WorkflowSettings +} import org.apache.texera.amber.engine.common.AmberRuntime import org.apache.texera.amber.engine.e2e.TestUtils.{ buildWorkflow, @@ -45,6 +49,7 @@ import org.apache.texera.amber.operator.filter.{ SpecializedFilterOpDesc } import org.apache.texera.amber.operator.limit.LimitOpDesc +import org.apache.texera.amber.operator.loop.{LoopEndOpDesc, LoopStartOpDesc} import org.apache.texera.amber.operator.source.scan.text.TextInputSourceOpDesc import org.apache.texera.amber.operator.trycatch.{FinallyOpDesc, TryCatchOpDesc} import org.apache.texera.amber.operator.udf.python.PythonUDFOpDescV2 @@ -323,6 +328,92 @@ class TryCatchIntegrationSpec assert(portRowCount(outerFin, port1) == 1) } + it should "rethrow from a nested frame with an unconnected Catch to the enclosing catch" in { + // PL: try1 { x = A(); try2 { B() } /* no catch2 */ } catch1 { C() } — an + // uncaught inner failure aborts the OUTER attempt. The inner gate, having + // no catch subgraph, forwards the error instead of absorbing it; the + // forward travels its dangling ports' signal edges to the outer gate + // (replay) and the outer Merger (release agreement). + val src = textInput("1\n2\n3") + val outer = new TryCatchOpDesc() + val outerTail = limit(3) // outer's own try path: completes CLEAN + val inner = new TryCatchOpDesc() // Catch left unconnected: pure guard + val innerTry = failingFilter() // terminal; fails => rethrow + val outerCatch = limit(2) + val fin = new FinallyOpDesc() + + val materialized = runAndGetMaterializedRowCounts( + List(src, outer, outerTail, inner, innerTry, outerCatch, fin), + List( + link(src, port0, outer, port0), + link(outer, port0, outerTail, port0), + link(outer, port0, inner, port0), + link(inner, port0, innerTry, port0), // inner Try; Catch unconnected + link(outerTail, port0, fin, port0), // From Try (clean data!) + link(outer, port1, outerCatch, port0), // outer Catch + link(outerCatch, port0, fin, port1) // From Catch + ) + ) + // the outer catch replayed all 3 rows through limit(2); and although the + // wired try path completed cleanly, the Merger must NOT flush it — the + // attempt as a whole failed + assert(materialized(fin.operatorIdentifier) == 0) + assert(portRowCount(fin, port1) == 2) + } + + it should "release the recovery when a terminal try fork fails but the wired fork is clean" in { + // The gate hears every ending of the try cone; the Merger must reach the + // same verdict. A failing terminal fork used to be invisible to the + // Merger (no error on From Try), which then flushed the clean fork's + // rows while the gate was simultaneously releasing the catch replay. + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val okFork = limit(3) // wired into the Finally, completes clean + val badFork = failingFilter() // terminal, dies + val catchBranch = limit(1) + val fin = new FinallyOpDesc() + + val materialized = runAndGetMaterializedRowCounts( + List(src, tryCatch, okFork, badFork, catchBranch, fin), + List( + link(src, port0, tryCatch, port0), + link(tryCatch, port0, okFork, port0), + link(tryCatch, port0, badFork, port0), + link(okFork, port0, fin, port0), // From Try + link(tryCatch, port1, catchBranch, port0), + link(catchBranch, port0, fin, port1) // From Catch + ) + ) + assert(materialized(fin.operatorIdentifier) == 0) // NOT okFork's 3 rows + assert(portRowCount(fin, port1) == 1) // the replay through limit(1) + } + + it should "release nothing when a terminal fork of the CATCH branch fails" in { + // The recovery itself forked and one fork died: the recovery is not + // whole, so the Merger must not release the surviving fork's rows as if + // it were. Both result ports stay empty and the run still terminates. + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val tryBranch = failingFilter() // the attempt fails => catch replays + val catchOk = limit(2) // surviving fork, wired into the Finally + val catchBad = failingFilter() // terminal fork of the recovery, dies + val fin = new FinallyOpDesc() + + val materialized = runAndGetMaterializedRowCounts( + List(src, tryCatch, tryBranch, catchOk, catchBad, fin), + List( + link(src, port0, tryCatch, port0), + link(tryCatch, port0, tryBranch, port0), + link(tryBranch, port0, fin, port0), // From Try (poisoned) + link(tryCatch, port1, catchOk, port0), + link(tryCatch, port1, catchBad, port0), + link(catchOk, port0, fin, port1) // From Catch + ) + ) + assert(materialized(fin.operatorIdentifier) == 0) + assert(portRowCount(fin, port1) == 0) // suppressed: half a recovery is no recovery + } + it should "support an inner frame inside the CATCH branch: try1 {} catch1 { try2 {} catch2 {} } finally1" in { // The PL shape `try1 { attempt } catch1 { try2 { A } catch2 { B } } finally1`: // the outer recovery is itself guarded. The inner construct's closing @@ -473,6 +564,127 @@ class ProcessTupleOperator(UDFOperatorV2): assert(portRowCount(fin, port1) == 2) } + it should "emit recovery rows only when the recovery's own inner attempt fails: catch1 { try2 { sideEffect } catch2 { B } } finally1" in { + // A nested frame inside the catch block whose CATCH branch alone feeds + // the enclosing Finally (its try branch is a terminal side effect): + // real-life "in the recovery, attempt a side-effecting operation; if IT + // fails, route fallback rows onward instead". The Finally is fed only + // through the nested frame's gate, which the strict pairing pass cannot + // see — the two-pass pairing resolves it. Here BOTH attempts fail, so B + // replays the recovery and its rows are the outer construct's value. + val src = textInput("1\n2\n3") + val outer = new TryCatchOpDesc() + val outerTry = failingFilter() // outer attempt fails => catch1 runs + val inner = new TryCatchOpDesc() // catch1's body: a guarded side effect + val innerSide = failingFilter() // terminal side effect; fails => B runs + val fallback = limit(2) + val fin = new FinallyOpDesc() + + val materialized = runAndGetMaterializedRowCounts( + List(src, outer, outerTry, inner, innerSide, fallback, fin), + List( + link(src, port0, outer, port0), + link(outer, port0, outerTry, port0), // try1 + link(outerTry, port0, fin, port0), // From Try (poisoned, discarded) + link(outer, port1, inner, port0), // catch1 = the nested frame + link(inner, port0, innerSide, port0), // try2, terminal + link(inner, port1, fallback, port0), // catch2 + link(fallback, port0, fin, port1) // ONLY catch2 continues onward + ) + ) + assert(materialized(fin.operatorIdentifier) == 0) + assert(portRowCount(fin, port1) == 2) // the fallback's replay + assert(materialized(innerSide.operatorIdentifier) == 0) + } + + it should "keep the Finally intact when the Error Info branch itself fails" in { + // try fails -> catch recovers -> the REPORTER lane (Error Info consumers) + // fails too. The reporter sits outside both cones, so its failure cannot + // reach the Finally (forward-only poison + provenance): the recovery is + // delivered untouched and the run terminates. The reporter lane is + // wrapped in its own guard here — an UNGUARDED reporter failure pauses + // the workflow instead, by the pause-preservation rule. + val src = textInput("1\n2\n3") + val tryCatch = new TryCatchOpDesc() + val failing = failingFilter() + val catchBranch = limit(2) + val fin = new FinallyOpDesc() + val reporterGuard = new TryCatchOpDesc() // Catch unconnected: pure guard + val reporterBoom = failingFilter() // dies on the first Error Info row + + val materialized = runAndGetMaterializedRowCounts( + List(src, tryCatch, failing, catchBranch, fin, reporterGuard, reporterBoom), + List( + link(src, port0, tryCatch, port0), + link(tryCatch, port0, failing, port0), // Try + link(failing, port0, fin, port0), // From Try (poisoned, discarded) + link(tryCatch, port1, catchBranch, port0), // Catch + link(catchBranch, port0, fin, port1), // From Catch + link(tryCatch, PortIdentity(2), reporterGuard, port0), // Error Info + link(reporterGuard, port0, reporterBoom, port0) + ) + ) + // the recovery is exactly the catch rows, despite the reporter dying + assert(materialized(fin.operatorIdentifier) == 0) + assert(portRowCount(fin, port1) == 2) + assert(materialized(reporterBoom.operatorIdentifier) == 0) + } + + it should "catch and recover on EVERY iteration of a loop around the frame" in { + // A frame inside a loop body: each iteration replays one row through the + // frame, the attempt fails, the catch recovers it, and the loop takes the + // back-edge. Workers are recreated per iteration, so this exercises the + // whole frame apparatus (signal edges, snapshot release, staging) being + // rebuilt every time; the LoopEnd's materialized table accumulates across + // iterations (reuseStorage), so 3 rows == the frame recovered 3 times. + val src = textInput("1\n2\n3") + val start = new LoopStartOpDesc() + start.initialization = "i = 0" + start.output = "table.iloc[i]" + val tryCatch = new TryCatchOpDesc() + val tryBranch = failingFilter() + val catchBranch = limit(1) + val fin = new FinallyOpDesc() + val end = new LoopEndOpDesc() + end.update = "i += 1" + end.condition = "i < len(table)" + + val operators = List(src, start, tryCatch, tryBranch, catchBranch, fin, end) + val links = List( + link(src, port0, start, port0), + link(start, port0, tryCatch, port0), + link(tryCatch, port0, tryBranch, port0), + link(tryBranch, port0, fin, port0), + link(tryCatch, port1, catchBranch, port0), + link(catchBranch, port0, fin, port1), + // the construct's value continues into the loop tail, whichever side won + link(fin, port0, end, port0), + link(fin, port1, end, port0) + ) + // loops require MATERIALIZED execution (the back-edge is a cross-region + // state channel), so this test runs under its own context settings + val materialized = runWorkflowAndReadResults( + system, + buildWorkflow( + operators, + links, + workflowContext( + specId, + WorkflowSettings(dataTransferBatchSize = 400, executionMode = ExecutionMode.MATERIALIZED) + ) + ), + operators.map(_.operatorIdentifier), + _.getCount, + Duration.fromSeconds(90) + ) + val endRows = materialized.getOrElse(end.operatorIdentifier, -1L) + assert( + endRows == 3, + s"the frame must recover once per iteration and the loop must run all 3: got $endRows " + + s"(all: $materialized)" + ) + } + it should "guard a Python UDF failure and fall back to the catch branch" in { // The common real-world case: user code in a Python UDF raises. This // exercises the pyamber side of the design (error State emission + diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala index 2f6891ea56e..7e59861aaa4 100644 --- a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala @@ -120,16 +120,41 @@ object TryCatchFramePass { // makes the pairing unique under nesting. val gates = splitters.map(s => s.id.logicalOpId -> gateOf(plan, s)).toMap - val mergerToFrame: Map[PhysicalOpIdentity, OperatorIdentity] = mergers.flatMap { merger => + def pairingCandidates( + merger: PhysicalOp, + crossInternal: Boolean, + stopAt: Set[PhysicalOpIdentity] + ): List[PhysicalOp] = { val fromTrySources = sourcesInto(plan, merger.id, FROM_TRY) val fromCatchSources = sourcesInto(plan, merger.id, FROM_CATCH) - val candidates = splitters.filter { s => - val tryReach = reach(plan, s.id, TRY_PORT, crossInternal = false, stopAt = Set.empty) + splitters.filter { s => + val tryReach = reach(plan, s.id, TRY_PORT, crossInternal, stopAt) val catchReach = - reach(plan, gates(s.id.logicalOpId).id, CATCH_PORT, crossInternal = false, Set.empty) + reach(plan, gates(s.id.logicalOpId).id, CATCH_PORT, crossInternal, stopAt) fromTrySources.exists(src => tryReach.contains(src) || src == s.id) && fromCatchSources.exists(src => catchReach.contains(src)) } + } + + val mergerToFrame: Map[PhysicalOpIdentity, OperatorIdentity] = mergers.flatMap { merger => + // Pass 1 — strict: do not cross frame-internal (snapshot) links. This + // pairs every Finally fed directly from its frame's own subgraphs. + // Pass 2 — for a Finally that pass 1 could not see (its inputs come + // through a NESTED frame's gate, e.g. catch1 { try2 {..} catch2 { B } + // } with only B continuing): cross internal links, but stop at every + // OTHER Merger — a Merger is a closing brace, and never crossing one + // is what keeps pairing unique in chains (tc1 -> fin1 -> tc2 -> fin2); + // for genuine nesting the try/catch disjointness already guarantees + // only one frame can supply BOTH ports. + val candidates = pairingCandidates(merger, crossInternal = false, Set.empty) match { + case Nil => + pairingCandidates( + merger, + crossInternal = true, + stopAt = mergers.map(_.id).toSet - merger.id + ) + case found => found + } candidates match { case single :: Nil => Some(merger.id -> single.id.logicalOpId) case Nil => @@ -316,6 +341,86 @@ object TryCatchFramePass { frame.gate.id -> signals }.toMap + // The Merger must see the same decision evidence the gate does, for every + // cone ending that does not already flow into one of its data ports. + // Otherwise the two disagree: a failure on an unwired try ending leaves + // the Merger flushing the try side while the gate releases the replay, + // and a failing terminal fork of the catch branch lets the Merger release + // a half-dead recovery as if it were whole. + // try side: exactly the gate's signal list (it IS the try-side + // aggregate), minus endings wired into From Try; + // catch side: the frame's own catch-cone boundary endings (same tail + // rule; interiors of frames nested INSIDE this one are + // excluded — their apparatus escape ports, e.g. a + // rethrowing gate's dangling Catch, fall out of the + // boundary rule naturally), plus nested frames' terminal + // catch leaves (their failure is this frame's failed + // recovery), minus endings wired into From Catch. + val mergerSignals: Map[ + PhysicalOpIdentity, + (List[(PhysicalOpIdentity, PortIdentity)], List[(PhysicalOpIdentity, PortIdentity)]) + ] = + frames.flatMap { frame => + frame.merger.map { m => + def wiredTo(portId: PortIdentity): Set[(PhysicalOpIdentity, PortIdentity)] = + plan.links + .filter(l => l.toOpId == m.id && l.toPortId == portId) + .map(l => (l.fromOpId, l.fromPortId)) + .toSet + + val trySide = gateSignals(frame.gate.id).filterNot(wiredTo(FROM_TRY)) + + val nestedFrames = frames.filter(g => + g.logicalOpId != frame.logicalOpId && frame.cone.contains(g.splitter.id) + ) + val interiorToNested: PhysicalOpIdentity => Boolean = + opId => nestedFrames.exists(_.cone.contains(opId)) + + val ownEndings = frame.catchCone.toList + .filterNot(interiorToNested) + .flatMap { opId => + val op = plan.getOperator(opId) + op.outputPorts.keys + .filterNot(_.internal) + .filter { portId => + val outLinks = + plan.links.filter(l => l.fromOpId == opId && l.fromPortId == portId) + !outLinks.exists(l => frame.catchCone.contains(l.toOpId)) + } + .map(portId => (opId, portId)) + } + + val nestedCatchLeaves = nestedFrames + .filter(g => frame.catchCone.contains(g.splitter.id)) + .filter { g => // this frame is the innermost catch-block encloser + frames + .filter(h => + h.logicalOpId != g.logicalOpId && h.catchCone.contains(g.splitter.id) + ) + .sortBy(_.catchCone.size) + .headOption + .exists(_.logicalOpId == frame.logicalOpId) + } + .flatMap { g => + g.catchCone.toList.flatMap { opId => + val op = plan.getOperator(opId) + op.outputPorts.keys + .filterNot(_.internal) + .filter(portId => + plan.links.forall(l => !(l.fromOpId == opId && l.fromPortId == portId)) + ) + .map(portId => (opId, portId)) + } + } + + val catchSide = (ownEndings ++ nestedCatchLeaves).distinct + .filterNot(wiredTo(FROM_CATCH)) + .sortBy { case (opId, portId) => (opId.logicalOpId.id, opId.layerName, portId.id) } + + m.id -> (trySide, catchSide) + } + }.toMap + // guarded = drain semantics on own failure (error State in-band). Marks // every cone operator plus the frame apparatus itself; everything outside // any frame keeps the default report-and-pause behavior. @@ -336,6 +441,8 @@ object TryCatchFramePass { ) val config = new CatchGateConfig() config.ownConeOpIds = frame.coneIdStrings + config.catchConnected = + plan.links.exists(l => l.fromOpId == frame.gate.id && l.fromPortId == CATCH_PORT) op.withInputPorts(snapshotPort :: signalPorts) .withPartitionRequirement( List(None) ++ signalPorts.map(_ => Some(SignalPartition())) @@ -346,11 +453,29 @@ object TryCatchFramePass { case None => frames.find(_.merger.exists(_.id == op.id)) match { case Some(frame) => + val (trySide, catchSide) = mergerSignals(op.id) + // internal ids start at 2: executors receive only the int id, + // and 0/1 are the external From Try / From Catch + val signalPorts = (trySide ++ catchSide).zipWithIndex.map { + case (_, idx) => + InputPort(PortIdentity(idx + 2, internal = true), s"signal-${idx + 2}") + } val config = new FinallyMergerConfig() config.ownConeOpIds = frame.coneIdStrings - op.copy(opExecInitInfo = - OpExecWithClassName(MergerClassName, objectMapper.writeValueAsString(config)) - ) + config.trySignalPortIds = trySide.indices.map(_ + 2).toList + config.catchSignalPortIds = catchSide.indices.map(_ + 2 + trySide.size).toList + val fromTry = op.inputPorts(FROM_TRY)._1 + val fromCatch = op + .inputPorts(FROM_CATCH) + ._1 + .copy(dependencies = FROM_TRY :: signalPorts.map(_.id).toList) + op.withInputPorts(fromTry :: fromCatch :: signalPorts) + .withPartitionRequirement( + List(None, None) ++ signalPorts.map(_ => Some(SignalPartition())) + ) + .copy(opExecInitInfo = + OpExecWithClassName(MergerClassName, objectMapper.writeValueAsString(config)) + ) case None => op } } @@ -365,7 +490,15 @@ object TryCatchFramePass { } }.toSet - rebuildPlan(rebuiltOps, plan.links ++ signalLinks) + val mergerSignalLinks: Set[PhysicalLink] = mergerSignals.flatMap { + case (mergerId, (trySide, catchSide)) => + (trySide ++ catchSide).zipWithIndex.map { + case ((srcOpId, srcPortId), idx) => + PhysicalLink(srcOpId, srcPortId, mergerId, PortIdentity(idx + 2, internal = true)) + } + }.toSet + + rebuildPlan(rebuiltOps, plan.links ++ signalLinks ++ mergerSignalLinks) } /** @@ -499,6 +632,21 @@ object TryCatchFramePass { overlap.map(_.logicalOpId.id).mkString(", ") ) } + // Error Info lists CAUGHT failures. With no catch subgraph the frame + // handles nothing — failures rethrow to the enclosing frame — so a + // connected Error Info would stay forever empty (and forwarding the + // rethrown State would poison its consumers). Reject the near-miss. + val catchConnected = + plan.links.exists(l => l.fromOpId == frame.gate.id && l.fromPortId == CATCH_PORT) + val errorInfoConnected = + plan.links.exists(l => l.fromOpId == frame.gate.id && l.fromPortId == ERROR_INFO_PORT) + if (errorInfoConnected && !catchConnected) { + throw new IllegalArgumentException( + s"TryCatch '$name': Error Info lists caught failures, but the Catch port is " + + "unconnected so nothing is caught (failures rethrow to the enclosing frame) — " + + "connect Catch, or remove the Error Info consumers" + ) + } // Error Info feeding back into the try subgraph is a structural cycle // (reporter -> cone op -> tails -> signal edges -> gate) and a temporal // paradox (the report exists only once the attempt resolved). Catch-cone @@ -529,8 +677,6 @@ object TryCatchFramePass { s"offending: ${badCatch.map(_.logicalOpId.id).mkString(", ")}" ) } - val catchConnected = - plan.links.exists(l => l.fromOpId == frame.gate.id && l.fromPortId == CATCH_PORT) if (!catchConnected) { throw new IllegalArgumentException( s"TryCatch '$name': a paired Finally requires the Catch port to be connected" diff --git a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala index eed5a481a5a..37ae6b907e9 100644 --- a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala +++ b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala @@ -28,7 +28,12 @@ import org.apache.texera.amber.core.virtualidentity.{ WorkflowIdentity } import org.apache.texera.amber.core.workflow._ -import org.apache.texera.amber.operator.trycatch.{CatchGateConfig, FinallyOpDesc, TryCatchOpDesc} +import org.apache.texera.amber.operator.trycatch.{ + CatchGateConfig, + FinallyMergerConfig, + FinallyOpDesc, + TryCatchOpDesc +} import org.apache.texera.amber.util.JSONUtils.objectMapper import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -94,6 +99,30 @@ class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { objectMapper.readValue(descString, classOf[CatchGateConfig]) } + private def mergerConfig(merger: PhysicalOp): FinallyMergerConfig = { + val descString = merger.opExecInitInfo match { + case OpExecWithClassName(_, desc) => desc + } + objectMapper.readValue(descString, classOf[FinallyMergerConfig]) + } + + /** src -> TryCatch; Try -> tryOp (terminal); Catch and Error Info unconnected */ + private def rethrowFramePlan(): PhysicalPlan = { + val tcDesc = new TryCatchOpDesc() + tcDesc.setOperatorId("tc") + val tcPlan = tcDesc.getPhysicalPlan(wid, eid) + var plan = PhysicalPlan(operators = Set.empty, links = Set.empty) + (Set(sourceOp("src"), simpleOp("tryOp")) ++ tcPlan.operators).foreach { op => + plan = plan.addOperator(op.propagateSchema()) + } + List( + link(opId("src"), out0, opId("tc", TryCatchOpDesc.SPLITTER_LAYER), in0), + tcPlan.links.head, + link(opId("tc", TryCatchOpDesc.SPLITTER_LAYER), out0, opId("tryOp"), in0) + ).foreach(l => plan = plan.addLink(l)) + plan + } + private def link( from: PhysicalOpIdentity, fromPort: PortIdentity, @@ -240,6 +269,95 @@ class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { ) } + it should "give the Merger no signal ports when every cone ending is wired into it" in { + // The simple shape: tryOp -> From Try, catchOp -> From Catch. Failures on + // wired endings arrive on the data ports themselves, so signal ports + // would be pure redundancy — the Merger keeps its two-port shape. + val result = TryCatchFramePass.run(buildFramePlan(), None) + val merger = result.getOperator(opId("fin")) + merger.inputPorts.keys.count(_.internal) shouldBe 0 + val config = mergerConfig(merger) + config.trySignalPortIds shouldBe empty + config.catchSignalPortIds shouldBe empty + } + + it should "wire an unwired try ending into the Merger as a try-signal port" in { + // A terminal fork of the try cone: its failure reaches the gate (which + // releases the replay), so the Merger must hear it too, or the two + // disagree and the Merger flushes the partial attempt as a success. + var plan = buildFramePlan() + val sideTail = simpleOp("sideTail") + plan = plan.addOperator(sideTail.propagateSchema()) + plan = plan.addLink( + link(opId("tc", TryCatchOpDesc.SPLITTER_LAYER), out0, opId("sideTail"), in0) + ) + + val result = TryCatchFramePass.run(plan, None) + val merger = result.getOperator(opId("fin")) + val config = mergerConfig(merger) + config.trySignalPortIds shouldBe List(2) + config.catchSignalPortIds shouldBe empty + // the signal link originates at the fork's port and lands on internal 2 + result.links.count(l => + l.fromOpId == opId("sideTail") && l.toOpId == merger.id && + l.toPortId == PortIdentity(2, internal = true) + ) shouldBe 1 + // From Catch waits for the signal (dependee ordering), and the link + // carries only States/completion + val fromCatch = merger.inputPorts(PortIdentity(1))._1 + fromCatch.dependencies should contain(PortIdentity(2, internal = true)) + merger.partitionRequirement.lift(2).flatten shouldBe Some(SignalPartition()) + } + + it should "wire an unwired catch ending into the Merger as a catch-signal port" in { + // A terminal fork of the catch cone: if it dies, the recovery as a whole + // failed — the Merger must suppress the release instead of emitting the + // surviving fork's rows as if the recovery were whole. + var plan = buildFramePlan() + val catchFork = simpleOp("catchFork") + plan = plan.addOperator(catchFork.propagateSchema()) + plan = plan.addLink( + link(opId("tc", TryCatchOpDesc.GATE_LAYER), catchPort, opId("catchFork"), in0) + ) + + val result = TryCatchFramePass.run(plan, None) + val merger = result.getOperator(opId("fin")) + val config = mergerConfig(merger) + config.trySignalPortIds shouldBe empty + config.catchSignalPortIds shouldBe List(2) + result.links.count(l => + l.fromOpId == opId("catchFork") && l.toOpId == merger.id && + l.toPortId == PortIdentity(2, internal = true) + ) shouldBe 1 + } + + it should "mark the gate of a frame with an unconnected Catch port for rethrow" in { + val result = TryCatchFramePass.run(rethrowFramePlan(), None) + gateConfig(result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER))).catchConnected shouldBe + false + // and the standard frame keeps catching + val connected = TryCatchFramePass.run(buildFramePlan(), None) + gateConfig(connected.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER))).catchConnected shouldBe + true + } + + it should "reject Error Info consumers on a frame whose Catch port is unconnected" in { + // Error Info lists CAUGHT failures; with no catch subgraph nothing is + // caught (failures rethrow) — the table would stay forever empty, and + // the rethrown State would poison the consumers. + var plan = rethrowFramePlan() + val audit = fixedSchemaOp("audit") + plan = plan.addOperator(audit.propagateSchema()) + plan = plan.addLink( + link(opId("tc", TryCatchOpDesc.GATE_LAYER), TryCatchOpDesc.ERROR_INFO_PORT, opId("audit"), in0) + ) + val rejected = intercept[IllegalArgumentException] { + TryCatchFramePass.run(plan, None) + } + rejected.getMessage should include("Error Info") + rejected.getMessage should include("Catch port is unconnected") + } + it should "not signal-partition the user's data link into Finally" in { // Partitioning is PER-LINK: the tail wired to Finally keeps two links out // of the same output port -- the user's data link (normal partitioning, so @@ -544,25 +662,30 @@ class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { rejected.getMessage should include("its own Finally") } - it should "require the enclosing Finally's From Catch to come from its own block, not through a nested frame's gate" in { - // Documented limitation, pinned as real behavior. Wiring ONLY a nested - // frame's catch branch into the enclosing Finally (its try branch ending - // in its own sink) dodges the partial-row leak, but the enclosing Finally - // can no longer be paired: frame pairing refuses to traverse - // frame-internal (snapshot) links -- which is what makes pairing unique - // under nesting -- so a nested frame's catch branch, reachable only - // through that frame's own gate, is invisible to it. The shape is also - // semantically thin: the enclosing catch block would contribute nothing - // whenever the inner attempt succeeds. The fix for a user is the same as - // above -- close the inner frame with its own Finally and wire THAT into - // the enclosing one -- so the pairing error points the right way. + it should "pair a Finally fed only through a nested frame's catch branch (two-pass pairing)" in { + // Real-life PL: catch1 { try2 { sideEffect() } catch2 { B } } finally1 — + // the recovery emits rows ONLY when its own inner attempt failed. The + // strict pairing pass cannot see B (reachable from gate1.Catch only + // through tc2's internal snapshot link), so a second pass crosses + // internal links while stopping at every other Merger, which keeps + // chain shapes unique. The nested frame comes out fully armed. var plan = nestedInCatchPlan(withInnerFinally = false) plan = plan.removeLink(link(opId("try2op"), out0, opId("fin"), PortIdentity(1))) - val rejected = intercept[IllegalArgumentException] { - TryCatchFramePass.run(plan, None) - } - rejected.getMessage should include("not wired to any TryCatch") + val result = TryCatchFramePass.run(plan, None) + val innerGate = result.getOperator(opId("tc2", TryCatchOpDesc.GATE_LAYER)) + val outerGate = result.getOperator(opId("tc", TryCatchOpDesc.GATE_LAYER)) + // the inner try tail (terminal sink) signals ITS gate, so try2's failure + // releases catch2 — and nothing signals across frames by accident + result.links.count(l => l.fromOpId == opId("try2op") && l.toOpId == innerGate.id) shouldBe 1 + result.links.count(l => l.fromOpId == opId("try2op") && l.toOpId == outerGate.id) shouldBe 0 + // the outer frame owns the nested block inclusively; the inner does not + // swallow the outer Merger + gateConfig(outerGate).ownConeOpIds should contain("catch2op/main") + gateConfig(innerGate).ownConeOpIds should not contain "fin/main" + // and the Finally still pairs with the OUTER frame: its From Catch input + // (catch2op) is validated against tc's catch cone + mergerConfig(result.getOperator(opId("fin"))).ownConeOpIds should contain("try2op/main") } it should "reject a Finally whose From Try comes from outside the frame" in { diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala index 9c342813214..8b449ab3f70 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateConfig.scala @@ -31,4 +31,12 @@ import com.fasterxml.jackson.annotation.JsonProperty class CatchGateConfig { @JsonProperty var ownConeOpIds: List[String] = List() + + // Whether the Catch port has consumers. Without a catch subgraph the frame + // handles nothing: an own-cone failure is FORWARDED (rethrow) instead of + // absorbed, so an enclosing frame — whose gate the pass already wires to + // this gate's dangling ports — can catch it. At top level forwarding goes + // nowhere and the run terminates with the console error, as before. + @JsonProperty + var catchConnected: Boolean = true } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala index 0503ee0e7ab..ccf92b6e0c1 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExec.scala @@ -60,21 +60,43 @@ class CatchGateOpExec(descString: String) extends OperatorExecutor { // one error State per execution, so (operatorId, workerId) identifies the // event however many signal ports (fan-out) it arrived on private val reportedErrors = mutable.LinkedHashMap[(String, String), TupleLike]() + // the error to RETHROW (catch unconnected): held until the snapshot lane, + // because signal ports are dependees and during the dependee phase this + // operator has no output ports yet — emitting there would go nowhere + private var rethrow: Option[State] = None override def processState(state: State, port: Int): Option[State] = { if (port == snapshotPortId) { // snapshot lane: pass through (loop envelopes, foreign errors) Some(state) - } else { - // signal lane: record own-cone failures, absorb everything - if (State.isError(state) && State.errorOperatorId(state).exists(ownCone.contains)) { + } else if (State.isError(state) && State.errorOperatorId(state).exists(ownCone.contains)) { + if (config.catchConnected) { + // signal lane, own-cone failure: trigger the catch and absorb — a + // caught error must not leak past the frame failed = true recordError(state) + } else if (rethrow.isEmpty) { + // no catch subgraph: nothing is handled here, so RETHROW — hold the + // error for the snapshot lane (phase 2, output ports assigned); it + // then travels this gate's dangling ports' signal edges to the + // enclosing gate and Merger (own-cone there, by inclusive + // ownership). No Error Info row either: the report belongs to + // whoever catches it. One error suffices — like PL, a single + // exception escapes the block. + rethrow = Some(state) } None + } else { + // ordinary or foreign States on a signal lane are absorbed; a foreign + // error always also traveled the splitter, so the snapshot lane + // forwards it + None } } + override def produceStateOnFinish(port: Int): Option[State] = + if (port == snapshotPortId) rethrow else None + private def recordError(state: State): Unit = { val envelope = state.values.get(State.ErrorKey) match { case Some(m: Map[_, _]) => m.asInstanceOf[Map[String, Any]] diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala index 0c137f500ff..449a475fa22 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerConfig.scala @@ -31,4 +31,16 @@ import com.fasterxml.jackson.annotation.JsonProperty class FinallyMergerConfig { @JsonProperty var ownConeOpIds: List[String] = List() + + // Signal-port ids (internal, starting at 2 so they cannot collide with the + // external From Try = 0 / From Catch = 1, since executors see only the int). + // One per cone ending NOT wired into this Merger: the gate aggregates every + // ending of the try cone for ITS decision, and the Merger must see the same + // evidence, or a failure on an unwired ending leaves the two disagreeing — + // the gate releasing the catch replay while the Merger still flushes the + // try side (or flushing a "recovery" whose unwired fork died). + @JsonProperty + var trySignalPortIds: List[Int] = List() + @JsonProperty + var catchSignalPortIds: List[Int] = List() } diff --git a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala index 1054c413981..7a916b18d2a 100644 --- a/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala +++ b/common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExec.scala @@ -54,6 +54,8 @@ class FinallyMergerOpExec(descString: String) extends OperatorExecutor { private val config: FinallyMergerConfig = objectMapper.readValue(descString, classOf[FinallyMergerConfig]) private val ownCone: Set[String] = config.ownConeOpIds.toSet + private val trySignals: Set[Int] = config.trySignalPortIds.toSet + private val catchSignals: Set[Int] = config.catchSignalPortIds.toSet private val FROM_TRY = 0 private val FROM_CATCH = 1 @@ -61,17 +63,44 @@ class FinallyMergerOpExec(descString: String) extends OperatorExecutor { private val stagedTry = new ArrayBuffer[Tuple]() private val stagedCatch = new ArrayBuffer[Tuple]() private var trySideClean = false + // Failures on cone endings NOT wired into this Merger, reported through the + // signal ports. Wired failures need no flag: the error State poisons the + // data port and the worker suppresses its finish hooks. + private var tryFailed = false + private var catchFailed = false override def processTuple(tuple: Tuple, port: Int): Iterator[TupleLike] = { - if (port == FROM_TRY) stagedTry.append(tuple) else stagedCatch.append(tuple) + if (port == FROM_TRY) stagedTry.append(tuple) + else if (port == FROM_CATCH) stagedCatch.append(tuple) + // signal-lane tuples cannot occur (SignalPartitioning drops them at the + // sender); ignore defensively Iterator.empty } override def processState(state: State, port: Int): Option[State] = { - if ( - port == FROM_TRY && State.isError(state) && - State.errorOperatorId(state).exists(ownCone.contains) - ) { + val ownError = + State.isError(state) && State.errorOperatorId(state).exists(ownCone.contains) + if (trySignals.contains(port) || catchSignals.contains(port)) { + // signal lane: same decision evidence the gate sees, for the endings + // that do not flow into this Merger + if (ownError && trySignals.contains(port)) { + // caught by this frame (the gate is releasing the replay): discard + // the failed attempt and absorb — it must not leak past the frame + tryFailed = true + stagedTry.clear() + None + } else if (ownError) { + // a catch-side ending died: the recovery as a whole failed. Suppress + // the release and forward the error — escalation to the enclosing + // frame, exactly like a wired catch-side failure + catchFailed = true + Some(state) + } else { + // ordinary/foreign States on a signal lane are absorbed (foreign + // errors travel the data lanes) + None + } + } else if (port == FROM_TRY && ownError) { // caught: discard the failed attempt's staged output and absorb the // error — it must not leak past the frame stagedTry.clear() @@ -89,10 +118,16 @@ class FinallyMergerOpExec(descString: String) extends OperatorExecutor { // (this callback is suppressed by the worker if the port was poisoned) trySideClean = true Iterator.empty - } else if (trySideClean) { + } else if (port != FROM_CATCH) { + Iterator.empty // a signal port finishing carries no output + } else if (trySideClean && !tryFailed) { stagedTry.iterator.map(t => (t, Some(FinallyOpDesc.TRY_RESULT))) - } else { + } else if (!catchFailed) { stagedCatch.iterator.map(t => (t, Some(FinallyOpDesc.CATCH_RESULT))) + } else { + // double failure: the attempt failed and so did part of the recovery — + // nothing is released; the forwarded catch-side error escalates + Iterator.empty } } } diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala index 49ec903349f..06b00082b9f 100644 --- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/CatchGateOpExecSpec.scala @@ -32,9 +32,13 @@ class CatchGateOpExecSpec extends AnyFlatSpec with Matchers { private val snapshotPort = TryCatchOpDesc.SNAPSHOT_IN.id // 0 private val signalPort = 1 - private def mkGate(ownCone: List[String]): CatchGateOpExec = { + private def mkGate( + ownCone: List[String], + catchConnected: Boolean = true + ): CatchGateOpExec = { val config = new CatchGateConfig() config.ownConeOpIds = ownCone + config.catchConnected = catchConnected new CatchGateOpExec(objectMapper.writeValueAsString(config)) } @@ -91,4 +95,44 @@ class CatchGateOpExecSpec extends AnyFlatSpec with Matchers { gate.processState(ownError, signalPort) gate.processTupleMultiPort(tuple, signalPort) shouldBe empty } + + it should "RETHROW an own-cone error when the Catch port is unconnected" in { + // No catch subgraph => the frame handles nothing. The error is held + // through the signal phase (dependee ports run before this operator has + // output ports, so emitting there would go nowhere) and rethrown at the + // snapshot lane's finish, where it travels the gate's dangling ports' + // signal edges to the enclosing gate and Merger. No Error Info row is + // recorded — the report belongs to whoever actually catches it. + val gate = mkGate(List("myop/main"), catchConnected = false) + gate.processState(ownError, signalPort) shouldBe None // held, not emitted yet + gate.produceStateOnFinish(snapshotPort) shouldBe Some(ownError) + gate.onFinishMultiPort(snapshotPort) shouldBe empty // no report + } + + it should "rethrow a single error even when several own-cone failures arrive" in { + // Like PL, one exception escapes the block: the first held error wins. + val gate = mkGate(List("myop/main", "otherop/main"), catchConnected = false) + val secondError = + State.errorState("otherop/main", "worker-1", new RuntimeException("later")) + gate.processState(ownError, signalPort) shouldBe None + gate.processState(secondError, signalPort + 1) shouldBe None + gate.produceStateOnFinish(snapshotPort) shouldBe Some(ownError) + } + + it should "produce no finish State when the catch is connected or nothing failed" in { + val catching = mkGate(List("myop/main")) + catching.processState(ownError, signalPort) + catching.produceStateOnFinish(snapshotPort) shouldBe None // absorbed, not rethrown + val clean = mkGate(List("myop/main"), catchConnected = false) + clean.produceStateOnFinish(snapshotPort) shouldBe None + } + + it should "still absorb foreign errors and ordinary States when the Catch port is unconnected" in { + // Rethrow applies to OWN failures only: a foreign error always also + // traveled through the splitter, so the snapshot lane forwards it — + // forwarding it here too would duplicate it. + val gate = mkGate(List("myop/main"), catchConnected = false) + gate.processState(foreignError, signalPort) shouldBe None + gate.processState(State(Map("some" -> "state")), signalPort) shouldBe None + } } diff --git a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala index 25460199865..3f0ba2430b4 100644 --- a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala +++ b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/trycatch/FinallyMergerOpExecSpec.scala @@ -37,9 +37,15 @@ class FinallyMergerOpExecSpec extends AnyFlatSpec with Matchers { private val TRY_RESULT = FinallyOpDesc.TRY_RESULT private val CATCH_RESULT = FinallyOpDesc.CATCH_RESULT - private def mkMerger(ownCone: List[String] = List("myop/main")): FinallyMergerOpExec = { + private def mkMerger( + ownCone: List[String] = List("myop/main"), + trySignalPortIds: List[Int] = List(), + catchSignalPortIds: List[Int] = List() + ): FinallyMergerOpExec = { val config = new FinallyMergerConfig() config.ownConeOpIds = ownCone + config.trySignalPortIds = trySignalPortIds + config.catchSignalPortIds = catchSignalPortIds new FinallyMergerOpExec(objectMapper.writeValueAsString(config)) } @@ -92,6 +98,45 @@ class FinallyMergerOpExecSpec extends AnyFlatSpec with Matchers { merger.processState(envelope, FROM_TRY) shouldBe Some(envelope) } + it should "flush the catch side when an UNWIRED try ending failed (signal lane)" in { + // A failing terminal fork of the try cone never touches From Try: its + // error arrives on a signal port. The wired fork completed cleanly + // (onFinishMultiPort(From Try) runs), but the attempt as a whole failed — + // the Merger must agree with the gate and release the recovery, not the + // partial attempt. + val merger = mkMerger(trySignalPortIds = List(2)) + tuples(0 until 3).foreach(merger.processTuple(_, FROM_TRY)) // wired fork, clean + merger.processState(ownError, 2) shouldBe None // absorbed: caught by this frame + merger.onFinishMultiPort(FROM_TRY) shouldBe empty + val fallback = tuples(100 until 102) + fallback.foreach(merger.processTuple(_, FROM_CATCH)) + merger.onFinishMultiPort(FROM_CATCH).toList shouldBe + fallback.map(t => (t, Some(CATCH_RESULT))) + } + + it should "release nothing when an UNWIRED catch ending failed (signal lane)" in { + // The recovery forked and a terminal fork died: the recovery as a whole + // failed. The error is FORWARDED (escalation, like a wired catch-side + // failure) and the release is suppressed — never a half-dead recovery. + val merger = mkMerger(trySignalPortIds = List(2), catchSignalPortIds = List(3)) + merger.processState(ownError, 2) shouldBe None // attempt failed + merger.onFinishMultiPort(FROM_TRY) shouldBe empty + tuples(100 until 102).foreach(merger.processTuple(_, FROM_CATCH)) // surviving fork + merger.processState(ownError, 3) shouldBe Some(ownError) // forwarded: escalation + merger.onFinishMultiPort(FROM_CATCH) shouldBe empty + } + + it should "absorb foreign and ordinary States on signal lanes" in { + val merger = mkMerger(trySignalPortIds = List(2), catchSignalPortIds = List(3)) + merger.processState(foreignError, 2) shouldBe None + merger.processState(State(Map("k" -> 1L)), 3) shouldBe None + // and neither set a failure flag: a clean run still flushes the try side + val rows = tuples(0 until 2) + rows.foreach(merger.processTuple(_, FROM_TRY)) + merger.onFinishMultiPort(FROM_TRY) shouldBe empty + merger.onFinishMultiPort(FROM_CATCH).toList shouldBe rows.map(t => (t, Some(TRY_RESULT))) + } + it should "emit the winning branch's actual field values, unchanged" in { // Not just row COUNTS: the tuples that come out of Finally must be the // very rows the winning branch produced, with their values intact. From 7065fc2ea722414d25981d1f17b5e32ffb7cf4c6 Mon Sep 17 00:00:00 2001 From: carloea2 Date: Tue, 11 Aug 2026 01:38:02 -0600 Subject: [PATCH 10/10] style: format nested-frame changes --- .../engine/e2e/TryCatchIntegrationSpec.scala | 6 +--- .../common/compiler/TryCatchFramePass.scala | 32 +++++++++---------- .../compiler/TryCatchFramePassSpec.scala | 7 +++- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala index 2745f0417b7..e478787d8a1 100644 --- a/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala +++ b/amber/src/test/integration/org/apache/texera/amber/engine/e2e/TryCatchIntegrationSpec.scala @@ -28,11 +28,7 @@ import org.apache.texera.amber.core.storage.DocumentFactory import org.apache.texera.amber.core.storage.model.VirtualDocument import org.apache.texera.amber.core.tuple.Tuple import org.apache.texera.amber.core.virtualidentity.{ExecutionIdentity, OperatorIdentity} -import org.apache.texera.amber.core.workflow.{ - ExecutionMode, - PortIdentity, - WorkflowSettings -} +import org.apache.texera.amber.core.workflow.{ExecutionMode, PortIdentity, WorkflowSettings} import org.apache.texera.amber.engine.common.AmberRuntime import org.apache.texera.amber.engine.e2e.TestUtils.{ buildWorkflow, diff --git a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala index 7e59861aaa4..34214cedbdb 100644 --- a/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala +++ b/common/workflow-compiler/src/main/scala/org/apache/texera/common/compiler/TryCatchFramePass.scala @@ -246,22 +246,22 @@ object TryCatchFramePass { // catch2 { .. } finally2 { } .. }` with try1 itself terminal). .filter(other => other.cone.contains(open.splitter.id)) .foreach { other => - other.merger.foreach { m => - val leaking = plan.links - .filter(l => l.toOpId == m.id && open.tryCone.contains(l.fromOpId)) - .map(_.fromOpId) - if (leaking.nonEmpty) { - throw new IllegalArgumentException( - s"TryCatch '${open.logicalOpId.id}' has no Finally of its own, but its Try " + - s"subgraph feeds the Finally of '${other.logicalOpId.id}' (via " + - s"${leaking.map(_.logicalOpId.id).mkString(", ")}). A failed attempt would leak " + - s"its partial rows into that reconvergence point: give '${open.logicalOpId.id}' " + - "its own Finally and wire that into " + - s"'${other.logicalOpId.id}' instead (Finallys close inside-out)." - ) + other.merger.foreach { m => + val leaking = plan.links + .filter(l => l.toOpId == m.id && open.tryCone.contains(l.fromOpId)) + .map(_.fromOpId) + if (leaking.nonEmpty) { + throw new IllegalArgumentException( + s"TryCatch '${open.logicalOpId.id}' has no Finally of its own, but its Try " + + s"subgraph feeds the Finally of '${other.logicalOpId.id}' (via " + + s"${leaking.map(_.logicalOpId.id).mkString(", ")}). A failed attempt would leak " + + s"its partial rows into that reconvergence point: give '${open.logicalOpId.id}' " + + "its own Finally and wire that into " + + s"'${other.logicalOpId.id}' instead (Finallys close inside-out)." + ) + } } } - } } // ---- 4. per-op innermost-frame assignment (smallest containing cone) @@ -394,9 +394,7 @@ object TryCatchFramePass { .filter(g => frame.catchCone.contains(g.splitter.id)) .filter { g => // this frame is the innermost catch-block encloser frames - .filter(h => - h.logicalOpId != g.logicalOpId && h.catchCone.contains(g.splitter.id) - ) + .filter(h => h.logicalOpId != g.logicalOpId && h.catchCone.contains(g.splitter.id)) .sortBy(_.catchCone.size) .headOption .exists(_.logicalOpId == frame.logicalOpId) diff --git a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala index 37ae6b907e9..908a27a9ae5 100644 --- a/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala +++ b/common/workflow-compiler/src/test/scala/org/apache/texera/common/compiler/TryCatchFramePassSpec.scala @@ -349,7 +349,12 @@ class TryCatchFramePassSpec extends AnyFlatSpec with Matchers { val audit = fixedSchemaOp("audit") plan = plan.addOperator(audit.propagateSchema()) plan = plan.addLink( - link(opId("tc", TryCatchOpDesc.GATE_LAYER), TryCatchOpDesc.ERROR_INFO_PORT, opId("audit"), in0) + link( + opId("tc", TryCatchOpDesc.GATE_LAYER), + TryCatchOpDesc.ERROR_INFO_PORT, + opId("audit"), + in0 + ) ) val rejected = intercept[IllegalArgumentException] { TryCatchFramePass.run(plan, None)