Skip to content

feat: allow Multi-Link on Input Ports#4342

Merged
aglinxinyuan merged 29 commits intomainfrom
xinyuan-union
Apr 11, 2026
Merged

feat: allow Multi-Link on Input Ports#4342
aglinxinyuan merged 29 commits intomainfrom
xinyuan-union

Conversation

@aglinxinyuan
Copy link
Copy Markdown
Contributor

@aglinxinyuan aglinxinyuan commented Mar 31, 2026

What changes were proposed in this PR?

Enabling multi-link support for input ports.

Before the change After the change
image image

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.

@aglinxinyuan aglinxinyuan self-assigned this Mar 31, 2026
Copilot AI review requested due to automatic review settings March 31, 2026 23:42
@github-actions github-actions bot added the frontend Changes related to the frontend GUI label Mar 31, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…low.service.ts

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
@chenlica
Copy link
Copy Markdown
Contributor

chenlica commented Apr 1, 2026

@Xiao-zhen-Liu Please review it.

Copy link
Copy Markdown
Contributor

@Xiao-zhen-Liu Xiao-zhen-Liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment.

@aglinxinyuan aglinxinyuan marked this pull request as draft April 3, 2026 05:33
@aglinxinyuan
Copy link
Copy Markdown
Contributor Author

Per offline discussion, we will change the flag from "allowMultiInputs" to "disallowMultiInputs", and disallow multi-inputs on visualization operators.

@aglinxinyuan aglinxinyuan marked this pull request as ready for review April 3, 2026 05:35
@aglinxinyuan aglinxinyuan marked this pull request as draft April 3, 2026 05:35
@github-actions github-actions bot added the common label Apr 4, 2026
@aglinxinyuan aglinxinyuan marked this pull request as ready for review April 6, 2026 02:10
Copy link
Copy Markdown
Contributor

@Xiao-zhen-Liu Xiao-zhen-Liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we agreed on letting disallowMultiInput be false by default but true for visualization operators. Your changes do not seem to reflect that.

Copy link
Copy Markdown
Contributor

@Xiao-zhen-Liu Xiao-zhen-Liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aglinxinyuan aglinxinyuan enabled auto-merge (squash) April 9, 2026 07:02
@aglinxinyuan aglinxinyuan merged commit dffd031 into main Apr 11, 2026
11 checks passed
@aglinxinyuan aglinxinyuan deleted the xinyuan-union branch April 11, 2026 00:03
Copy link
Copy Markdown
Contributor

@Yicong-Huang Yicong-Huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aglinxinyuan @Xiao-zhen-Liu are these changes intentional? it is not mentioned in PR description.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is unintentional, but it has no effect since it's just a placeholder.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree. It's definitely a mistake. Besides this one, all the other changes are intentional.

)
}
} else {
List(InputPort(PortIdentity(), allowMultiLinks = true))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep the PortIdentity() part. PortIdentity() returns PortIdentity(0).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will return the same result.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have those changes on the output port? I thought the PR is only about input port?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done by formatter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also checked, it's a better practice.

@aglinxinyuan
Copy link
Copy Markdown
Contributor Author

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.

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.

@Yicong-Huang
Copy link
Copy Markdown
Contributor

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common engine frontend Changes related to the frontend GUI python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow Multi-Link on Input Ports

5 participants