Copilot commented on code in PR #4379:
URL: https://github.com/apache/texera/pull/4379#discussion_r3076324838
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -421,7 +421,8 @@ export class ExecuteWorkflowService {
...op.operatorProperties,
operatorID: op.operatorID,
operatorType: op.operatorType,
- inputPorts: op.inputPorts,
+ inputPorts: op.inputPorts
+ .map(({ allowMultiInputs, ...port }: any) => port), //TODO: for
backward compatibility, remove later
Review Comment:
This compatibility filter only strips `allowMultiInputs` from `inputPorts`,
but legacy workflow JSONs can also include `allowMultiInputs` on `outputPorts`
(see example workflows under `bin/.../workflows`). Since `outputPorts` is
forwarded unchanged, the backend can still fail deserializing with
`UnrecognizedPropertyException`. Consider applying the same stripping (or a
shared sanitizer) to `outputPorts` as well before sending the logical plan.
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -421,7 +421,8 @@ export class ExecuteWorkflowService {
...op.operatorProperties,
operatorID: op.operatorID,
operatorType: op.operatorType,
- inputPorts: op.inputPorts,
+ inputPorts: op.inputPorts
+ .map(({ allowMultiInputs, ...port }: any) => port), //TODO: for
backward compatibility, remove later
Review Comment:
The `: any` annotation in the `map` callback defeats type-safety for port
objects. Since the intent is only to drop an optional legacy field, consider
using a narrow structural type (e.g., `PortDescription & { allowMultiInputs?:
boolean }`) or a small helper function instead of `any` so future port fields
remain checked by the compiler.
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -421,7 +421,8 @@ export class ExecuteWorkflowService {
...op.operatorProperties,
operatorID: op.operatorID,
operatorType: op.operatorType,
- inputPorts: op.inputPorts,
+ inputPorts: op.inputPorts
Review Comment:
This change introduces backward-compatibility logic for execution payload
generation, but there’s no unit test covering the legacy `allowMultiInputs`
field. Since `ExecuteWorkflowService.getLogicalPlanRequest` already has tests,
add a case that builds a workflow graph with ports containing
`allowMultiInputs` and asserts the resulting logical plan omits that field (for
both input and output ports, if supported).
--
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]
