This is an automated email from the ASF dual-hosted git repository.
aglinxinyuan pushed a commit to branch xinyuan-loop-feb
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/xinyuan-loop-feb by this push:
new 36c4ff4501 fix(loop): keep UI and engine in sync on the
loop-MATERIALIZED rule
36c4ff4501 is described below
commit 36c4ff4501eb051bb882503d9a9e958f6f69d7b1
Author: Xinyuan Lin <[email protected]>
AuthorDate: Fri May 22 13:14:00 2026 -0700
fix(loop): keep UI and engine in sync on the loop-MATERIALIZED rule
PR #4206 review feedback: the previous behavior silently rewrote
WorkflowExecutionService's executionMode to MATERIALIZED when the
workflow contained a Loop Start, but the frontend Settings panel kept
displaying whatever the user picked (often PIPELINED), so the UI and
the engine ran on different settings with no signal to the user.
Frontend (settings.component):
* On every workflowChanged event, walk the operators and check for
LoopStart / LoopEnd.
* If present, force executionMode to MATERIALIZED via
WorkflowActionService.updateExecutionMode, disable the radio
group so the user can't toggle it back, and show a single info
toast the first time we flip the selection (subsequent passes
don't re-notify).
* If absent, re-enable the radio group.
* Add a styled hint under the radio group explaining the lock.
* Tests: six new cases covering coerce-on-LoopStart, coerce-on-
LoopEnd, re-enable on loop removal, one-shot toast, no toast
when the workflow already opens with MATERIALIZED, and no-op
behavior when no loop is present.
Backend (WorkflowExecutionService.scala):
* Restore the silent executionMode=MATERIALIZED coercion as a
safety net for non-frontend callers (direct API submissions,
older clients). With the frontend now mirroring this rule, the
coercion is normally a no-op; if it does fire, both sides end
up at MATERIALIZED, not divergent.
---
.../web/service/WorkflowExecutionService.scala | 31 ++++----
.../left-panel/settings/settings.component.html | 5 ++
.../left-panel/settings/settings.component.scss | 7 ++
.../left-panel/settings/settings.component.spec.ts | 88 +++++++++++++++++++++-
.../left-panel/settings/settings.component.ts | 55 ++++++++++++++
5 files changed, 167 insertions(+), 19 deletions(-)
diff --git
a/amber/src/main/scala/org/apache/texera/web/service/WorkflowExecutionService.scala
b/amber/src/main/scala/org/apache/texera/web/service/WorkflowExecutionService.scala
index 77ef524358..c5d0e9fc4f 100644
---
a/amber/src/main/scala/org/apache/texera/web/service/WorkflowExecutionService.scala
+++
b/amber/src/main/scala/org/apache/texera/web/service/WorkflowExecutionService.scala
@@ -68,23 +68,20 @@ class WorkflowExecutionService(
with LazyLogging {
// Loops require materialized edges to carry state between iterations.
- // Previously we silently rewrote the user's execution mode to
- // MATERIALIZED here, but that left the UI displaying the user's
- // original (e.g. PIPELINED) choice while the engine ran something
- // else -- the user had no way to tell the two had diverged. Fail
- // loudly instead: surface a fatal error with an actionable message so
- // the user can update the workflow setting and re-run.
- if (
- request.logicalPlan.operators.exists(_.isInstanceOf[LoopStartOpDesc])
- && request.workflowSettings.executionMode != ExecutionMode.MATERIALIZED
- ) {
- throw new IllegalArgumentException(
- "This workflow contains loop operators (Loop Start / Loop End), which
require " +
- "the execution mode to be MATERIALIZED. Please open Workflow Settings
→ " +
- "Execution Mode, change it to Materialized, and re-run."
- )
- }
- workflowContext.workflowSettings = request.workflowSettings
+ // The frontend's Settings panel already detects loop operators and
+ // forces the radio button to MATERIALIZED (see
+ // `frontend/.../left-panel/settings/settings.component.ts`), so under
+ // normal use this coercion is a no-op. We keep it on the server side
+ // as a safety net for callers that bypass the frontend (e.g. direct
+ // API submissions or older clients): rather than failing the run,
+ // silently coerce so the user gets a successful execution. Because
+ // the frontend mirrors this rule, UI and engine cannot disagree.
+ workflowContext.workflowSettings =
+ if (request.logicalPlan.operators.exists(_.isInstanceOf[LoopStartOpDesc]))
{
+ request.workflowSettings.copy(executionMode = ExecutionMode.MATERIALIZED)
+ } else {
+ request.workflowSettings
+ }
val wsInput = new WebsocketInput(errorHandler)
addSubscription(
diff --git
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.html
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.html
index ceeea480af..27c937d291 100644
---
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.html
+++
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.html
@@ -34,6 +34,11 @@
>Materialized</label
>
</nz-radio-group>
+ <div
+ *ngIf="hasLoopOperator"
+ class="loop-mode-note">
+ Materialized is required when the workflow contains Loop Start / Loop
End operators.
+ </div>
<br />
<div class="form-group">
<label for="dataTransferBatchSize">Data Transfer Batch Size:</label>
diff --git
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.scss
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.scss
index 16a468e92d..d1be339c08 100644
---
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.scss
+++
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.scss
@@ -53,3 +53,10 @@ input.form-control {
font-size: 0.8em;
margin: 0;
}
+
+.loop-mode-note {
+ color: rgba(0, 0, 0, 0.55);
+ font-size: 0.8em;
+ font-weight: normal;
+ margin: 4px 0 0 0;
+}
diff --git
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.spec.ts
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.spec.ts
index c504071e21..4b5d10811a 100644
---
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.spec.ts
+++
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.spec.ts
@@ -41,6 +41,7 @@ class StubWorkflowActionService {
dataTransferBatchSize: 100,
executionMode: ExecutionMode.PIPELINED,
};
+ private operators: { operatorType: string }[] = [];
private workflowChangedSubject = new Subject<unknown>();
getWorkflowSettings(): WorkflowSettings {
@@ -68,6 +69,21 @@ class StubWorkflowActionService {
workflowChanged(): Observable<unknown> {
return this.workflowChangedSubject.asObservable();
}
+
+ // SettingsComponent inspects the graph to decide whether the workflow
+ // contains loop operators. Expose a minimal stub that returns whatever
+ // a test has staged via `setOperators`.
+ getTexeraGraph(): { getAllOperators: () => { operatorType: string }[] } {
+ return { getAllOperators: () => this.operators };
+ }
+
+ setOperators(ops: { operatorType: string }[]): void {
+ this.operators = ops;
+ }
+
+ fireWorkflowChanged(): void {
+ this.workflowChangedSubject.next(undefined);
+ }
}
describe("SettingsComponent", () => {
@@ -76,11 +92,11 @@ describe("SettingsComponent", () => {
let workflowActionService: StubWorkflowActionService;
let userService: StubUserService;
let workflowPersistSpy: { persistWorkflow: ReturnType<typeof vi.fn> };
- let notificationSpy: { error: ReturnType<typeof vi.fn> };
+ let notificationSpy: { error: ReturnType<typeof vi.fn>; info:
ReturnType<typeof vi.fn> };
beforeEach(async () => {
workflowPersistSpy = { persistWorkflow: vi.fn().mockReturnValue(of({})) };
- notificationSpy = { error: vi.fn() };
+ notificationSpy = { error: vi.fn(), info: vi.fn() };
await TestBed.configureTestingModule({
providers: [
@@ -187,4 +203,72 @@ describe("SettingsComponent", () => {
expect(setBatchSizeSpy).not.toHaveBeenCalled();
});
+
+ // ---- Loop-aware execution mode coercion -------------------------------
+ //
+ // SettingsComponent must keep the UI and the engine in agreement: a
+ // workflow containing Loop Start / Loop End operators always runs in
+ // MATERIALIZED mode (server enforces this too). The component coerces
+ // the form value, disables the radio group, and shows a one-shot
+ // notification when it has to flip the user's selection.
+
+ it("should coerce executionMode to MATERIALIZED and disable the radio group
when a LoopStart appears", () => {
+ workflowActionService.setOperators([{ operatorType: "LoopStart" }]);
+ workflowActionService.fireWorkflowChanged();
+
+ const ctrl = component.settingsForm.get("executionMode")!;
+ expect(component.hasLoopOperator).toBe(true);
+
expect(workflowActionService.getWorkflowSettings().executionMode).toBe(ExecutionMode.MATERIALIZED);
+ expect(ctrl.disabled).toBe(true);
+ });
+
+ it("should also trigger on LoopEnd presence, not only LoopStart", () => {
+ workflowActionService.setOperators([{ operatorType: "LoopEnd" }]);
+ workflowActionService.fireWorkflowChanged();
+
+ expect(component.hasLoopOperator).toBe(true);
+
expect(workflowActionService.getWorkflowSettings().executionMode).toBe(ExecutionMode.MATERIALIZED);
+ });
+
+ it("should re-enable the radio group when the last loop operator is
removed", () => {
+ workflowActionService.setOperators([{ operatorType: "LoopStart" }]);
+ workflowActionService.fireWorkflowChanged();
+ expect(component.settingsForm.get("executionMode")!.disabled).toBe(true);
+
+ workflowActionService.setOperators([]);
+ workflowActionService.fireWorkflowChanged();
+
+ expect(component.hasLoopOperator).toBe(false);
+ expect(component.settingsForm.get("executionMode")!.disabled).toBe(false);
+ });
+
+ it("should surface an info notification exactly once when coercing PIPELINED
→ MATERIALIZED", () => {
+ workflowActionService.setOperators([{ operatorType: "LoopStart" }]);
+ workflowActionService.fireWorkflowChanged();
+ // Subsequent workflow-changed events that don't change the loop-status
+ // must not re-notify (otherwise we'd spam the user on every position
+ // change / link add).
+ workflowActionService.fireWorkflowChanged();
+ workflowActionService.fireWorkflowChanged();
+
+ expect(notificationSpy.info).toHaveBeenCalledTimes(1);
+ });
+
+ it("should not notify when a workflow opens already in MATERIALIZED mode
with a loop", () => {
+ workflowActionService.updateExecutionMode(ExecutionMode.MATERIALIZED);
+ workflowActionService.setOperators([{ operatorType: "LoopStart" }]);
+ workflowActionService.fireWorkflowChanged();
+
+ expect(component.hasLoopOperator).toBe(true);
+ expect(notificationSpy.info).not.toHaveBeenCalled();
+ });
+
+ it("should leave executionMode alone when no loop operator is present", ()
=> {
+ workflowActionService.setOperators([{ operatorType: "CSVFileScan" }]);
+ workflowActionService.fireWorkflowChanged();
+
+ expect(component.hasLoopOperator).toBe(false);
+
expect(workflowActionService.getWorkflowSettings().executionMode).toBe(ExecutionMode.PIPELINED);
+ expect(component.settingsForm.get("executionMode")!.disabled).toBe(false);
+ });
});
diff --git
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.ts
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.ts
index e70761916e..d196ae0c2e 100644
---
a/frontend/src/app/workspace/component/left-panel/settings/settings.component.ts
+++
b/frontend/src/app/workspace/component/left-panel/settings/settings.component.ts
@@ -28,6 +28,13 @@ import { ExecutionMode } from
"../../../../common/type/workflow";
import { NzRadioGroupComponent, NzRadioComponent } from "ng-zorro-antd/radio";
import { NgClass, NgIf } from "@angular/common";
+// Operator types (LogicalOp Jackson names) that require the workflow to
+// run in MATERIALIZED mode. The matching source-of-truth on the server
+// is `LoopStartOpDesc` / `LoopEndOpDesc` registered in `LogicalOp.scala`,
+// and WorkflowExecutionService coerces to MATERIALIZED on its end as a
+// safety net for non-frontend clients.
+const LOOP_OPERATOR_TYPES = ["LoopStart", "LoopEnd"] as const;
+
@UntilDestroy()
@Component({
selector: "texera-settings",
@@ -38,6 +45,11 @@ import { NgClass, NgIf } from "@angular/common";
export class SettingsComponent implements OnInit {
settingsForm: FormGroup;
+ /** True iff the current workflow contains a Loop Start / Loop End
+ * operator. When true, executionMode is forced to MATERIALIZED and the
+ * radio group is disabled so the UI and the engine can't disagree. */
+ hasLoopOperator = false;
+
constructor(
private fb: FormBuilder,
private workflowActionService: WorkflowActionService,
@@ -71,10 +83,15 @@ export class SettingsComponent implements OnInit {
this.updateExecutionMode(mode);
});
+ // Apply once on load so a workflow opened with a pre-existing loop
+ // is coerced before the user ever sees the wrong mode.
+ this.syncExecutionModeForLoopOperators();
+
this.workflowActionService
.workflowChanged()
.pipe(untilDestroyed(this))
.subscribe(() => {
+ this.syncExecutionModeForLoopOperators();
this.settingsForm.patchValue(
{
dataTransferBatchSize:
this.workflowActionService.getWorkflowContent().settings.dataTransferBatchSize,
@@ -85,6 +102,44 @@ export class SettingsComponent implements OnInit {
});
}
+ /** Inspect the current workflow for loop operators. If any are present,
+ * force executionMode to MATERIALIZED and disable the radio group so the
+ * UI shows the same value the engine will actually run on (which the
+ * server-side `WorkflowExecutionService` coerces independently as a
+ * safety net). If none are present, re-enable the radio group. */
+ private syncExecutionModeForLoopOperators(): void {
+ const operators =
this.workflowActionService.getTexeraGraph().getAllOperators();
+ const hadLoop = this.hasLoopOperator;
+ this.hasLoopOperator = operators.some(op => (LOOP_OPERATOR_TYPES as
readonly string[]).includes(op.operatorType));
+
+ const currentMode =
this.workflowActionService.getWorkflowContent().settings.executionMode;
+ if (this.hasLoopOperator && currentMode !== ExecutionMode.MATERIALIZED) {
+ // Update the workflow setting; the next workflowChanged pass-through
+ // patches the form to match.
+
this.workflowActionService.updateExecutionMode(ExecutionMode.MATERIALIZED);
+ }
+
+ const executionModeCtrl = this.settingsForm.get("executionMode");
+ if (executionModeCtrl !== null) {
+ if (this.hasLoopOperator && !executionModeCtrl.disabled) {
+ executionModeCtrl.disable({ emitEvent: false });
+ } else if (!this.hasLoopOperator && executionModeCtrl.disabled) {
+ executionModeCtrl.enable({ emitEvent: false });
+ }
+ }
+
+ // First time we just discovered a loop in a workflow that was loaded
+ // with PIPELINED -- surface a toast so the user understands why the
+ // UI suddenly switched modes on them. Subsequent re-checks (form
+ // patches, position changes, etc.) won't re-notify because hadLoop
+ // is now true.
+ if (this.hasLoopOperator && !hadLoop && currentMode !==
ExecutionMode.MATERIALIZED) {
+ this.notificationService.info(
+ "Execution mode set to Materialized because this workflow contains
loop operators."
+ );
+ }
+ }
+
public confirmUpdateDataTransferBatchSize(dataTransferBatchSize: number):
void {
if (dataTransferBatchSize > 0) {
this.workflowActionService.setWorkflowDataTransferBatchSize(dataTransferBatchSize);
