feat: allow Multi-Link on Input Ports#4342
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables multi-link support on input ports in the workflow editor, aligning input-port connection behavior with existing output-port multi-link behavior (closes #4329).
Changes:
- Allows multiple links to connect to the same input port in the workflow editor connection validator.
- Updates workflow validation so input ports no longer require exactly one incoming link.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| frontend/src/app/workspace/service/validation/validation-workflow.service.ts | Adjusts operator connection validation logic for input ports to permit multi-link. |
| frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts | Relaxes UI connection validation to allow additional links into already-connected input ports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/app/workspace/service/validation/validation-workflow.service.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/workspace/service/validation/validation-workflow.service.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/workspace/service/validation/validation-workflow.service.ts
Outdated
Show resolved
Hide resolved
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts
Outdated
Show resolved
Hide resolved
…low.service.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
|
@Xiao-zhen-Liu Please review it. |
frontend/src/app/workspace/service/validation/validation-workflow.service.ts
Outdated
Show resolved
Hide resolved
|
Per offline discussion, we will change the flag from "allowMultiInputs" to "disallowMultiInputs", and disallow multi-inputs on visualization operators. |
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
I thought we agreed on letting disallowMultiInput be false by default but true for visualization operators. Your changes do not seem to reflect that.
frontend/src/app/workspace/service/operator-metadata/mock-operator-metadata.data.ts
Outdated
Show resolved
Hide resolved
...tor/src/main/scala/org/apache/texera/amber/operator/metadata/OperatorMetadataGenerator.scala
Outdated
Show resolved
Hide resolved
...tor/src/main/scala/org/apache/texera/amber/operator/metadata/OperatorMetadataGenerator.scala
Outdated
Show resolved
Hide resolved
Yicong-Huang
left a comment
There was a problem hiding this comment.
I left some comments. @aglinxinyuan please add some more description on the PR itself, so we know the changes in high level.
I find many unexpected changes, in my opinion they should not be in this PR, according to the current description. @chenlica please chime in on review process.
| uri-without-scheme = "localhost:5432/texera_iceberg_catalog" | ||
| uri-without-scheme = ${?STORAGE_ICEBERG_CATALOG_POSTGRES_URI_WITHOUT_SCHEME} | ||
|
|
||
| username = "texera" |
There was a problem hiding this comment.
@aglinxinyuan @Xiao-zhen-Liu are these changes intentional? it is not mentioned in PR description.
There was a problem hiding this comment.
This change is unintentional, but it has no effect since it's just a placeholder.
There was a problem hiding this comment.
we should avoid this kind of unintentional change. whether it has effect or not is debatable. for example, for other developers they may have conflict when they pull the change that they need to deal with. so that's also "effect".
There was a problem hiding this comment.
I totally agree. It's definitely a mistake. Besides this one, all the other changes are intentional.
| ) | ||
| } | ||
| } else { | ||
| List(InputPort(PortIdentity(), allowMultiLinks = true)) |
There was a problem hiding this comment.
I think we need to keep the PortIdentity() part. PortIdentity() returns PortIdentity(0).
There was a problem hiding this comment.
It will return the same result.
There was a problem hiding this comment.
I don't think so. do you have a test result to show they are equal?
PortIdentity(0) is not equal to 0 or null.
There was a problem hiding this comment.
Yes. I will provide test result later. If it's not the same, the test case should fail.
| "a 3D Scatter Plot; Bubbles are graphed using x and y labels, and their sizes determined by a z-value.", | ||
| OperatorGroupConstants.VISUALIZATION_BASIC_GROUP, | ||
| inputPorts = List(InputPort()), | ||
| outputPorts = List(OutputPort(mode = OutputMode.SINGLE_SNAPSHOT)) |
There was a problem hiding this comment.
why do we have those changes on the output port? I thought the PR is only about input port?
There was a problem hiding this comment.
It's just a refactor. The input port and the output port is still the same. The duplicate port definition is moved to the parent class.
There was a problem hiding this comment.
Let's separate refactor and feature change. even if you combine them, it will be good to annotate on the PR description. other wise this is unexpected change.
| @JsonSchemaTitle("Steps") | ||
| @JsonPropertyDescription("Optional: Each step includes a start and end value e.g., 0, 100.") | ||
| var steps: JList[BulletChartStepDefinition] = new ArrayList[BulletChartStepDefinition]() | ||
| var steps: JList[BulletChartStepDefinition] = new util.ArrayList[BulletChartStepDefinition]() |
There was a problem hiding this comment.
Done by formatter.
There was a problem hiding this comment.
this is unnecessary change. if it is caused by formatter then we should fix the formatter. does our formatter give inconsistent results across different runs?
There was a problem hiding this comment.
I also checked, it's a better practice.
All the changes are intentional expect one line. That line is also a placeholder so it has no effect on the codebase. All the changes in the PR are being discussed and reviewed. Every reviewer has different standards, so it's very subjective. |
Please minimize the change. even if a change has no "big effect" it should be avoided, if it is unrelated to the PR's scope. Not to mention there is indeed unintentional change included in the PR that should not have been merged. |
What changes were proposed in this PR?
Enabling multi-link support for input ports.
Any related issues, documentation, discussions?
Closes #4329
How was this PR tested?
Tested manually.
Was this PR authored or co-authored using generative AI tooling?
No.