Copilot commented on code in PR #7159:
URL: https://github.com/apache/texera/pull/7159#discussion_r3690342630
##########
agent-service/src/agent/workflow-state.spec.ts:
##########
@@ -174,3 +174,223 @@ describe("WorkflowState - getSubDAG", () => {
expect(subDag.operators.map(o => o.operatorID)).toEqual(["op2"]);
});
});
+
+describe("WorkflowState - toLogicalPlan", () => {
+ test("produces operators, port-indexed links, and an empty reuse list", ()
=> {
+ const state = new WorkflowState();
+ state.addOperator(makeOperator("op1"));
+ state.addOperator(makeOperator("op2"));
+ state.addLink(makeLink("l1", "op1", "op2"));
+
+ const plan = state.toLogicalPlan();
+
+ expect(plan.operators.map(o => o.operatorID)).toEqual(["op1", "op2"]);
+ // operatorProperties are spread onto each logical operator; type + ports
carry through
+ expect(plan.operators[0].operatorType).toBe("TestOp");
+ expect(plan.links).toEqual([
+ { fromOpId: "op1", fromPortId: { id: 0, internal: false }, toOpId:
"op2", toPortId: { id: 0, internal: false } },
+ ]);
+ expect(plan.opsToReuseResult).toEqual([]);
+ });
+
+ test("resolves the link port indices from the operators' port lists", () => {
+ const state = new WorkflowState();
+ state.addOperator(
+ makeOperator("src", {
+ outputPorts: [
+ { portID: "output-0", displayName: "Output 0" },
+ { portID: "output-1", displayName: "Output 1" },
+ ],
+ })
+ );
+ state.addOperator(
+ makeOperator("dst", {
+ inputPorts: [
+ { portID: "input-0", displayName: "Input 0" },
+ { portID: "input-1", displayName: "Input 1" },
+ ],
+ })
+ );
+ state.addLink({
+ linkID: "l1",
+ source: { operatorID: "src", portID: "output-1" },
+ target: { operatorID: "dst", portID: "input-1" },
+ });
+
+ expect(state.toLogicalPlan().links[0]).toEqual({
+ fromOpId: "src",
+ fromPortId: { id: 1, internal: false },
+ toOpId: "dst",
+ toPortId: { id: 1, internal: false },
+ });
+ });
+
+ test("the targetOperatorId argument is currently ignored — the whole-graph
plan is produced either way", () => {
+ // The parameter is accepted but unused in the current implementation;
assert the
+ // observed behavior so a future change that starts honoring it is caught
here.
+ const state = new WorkflowState();
+ state.addOperator(makeOperator("op1"));
+ state.addOperator(makeOperator("op2"));
+ state.addLink(makeLink("l1", "op1", "op2"));
+
+ expect(state.toLogicalPlan("op1")).toEqual(state.toLogicalPlan());
Review Comment:
This test codifies that `toLogicalPlan(targetOperatorId?)` ignores
`targetOperatorId`. However, the linked issue #7158 (which this PR closes)
calls out covering the *sub-plan* behavior when `targetOperatorId` is provided.
If the intended contract is to eventually honor `targetOperatorId`, this
assertion will fail as soon as the implementation is corrected and may block
that fix.
Consider either (1) not closing #7158 until the implementation honors
`targetOperatorId`, or (2) updating `WorkflowState.toLogicalPlan` to actually
produce a sub-plan and adjusting the test to assert that behavior instead of
asserting equivalence with the whole-graph plan.
This issue also appears on line 336 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
