-
Notifications
You must be signed in to change notification settings - Fork 116
feat: allow Multi-Link on Input Ports #4342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0457987
5b36a66
cd43d9f
e602ae8
4d2035e
4bfa9fb
39ced05
d073642
1c1f97b
9d4a640
6116d18
8918de0
b498313
7fcf839
09d4b2e
549ad07
3d6ad85
d63958a
cc6d497
d2ebecb
ae1a0aa
96e0e3b
fd29f50
c3cd887
58e1cec
1fbece2
267cf46
43f8a5a
9d68e14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,12 +39,12 @@ class DummyOpDesc extends LogicalOp with PortDescriptor { | |
| InputPort( | ||
| PortIdentity(idx), | ||
| displayName = portDesc.displayName, | ||
| allowMultiLinks = portDesc.allowMultiInputs, | ||
| disallowMultiLinks = portDesc.disallowMultiInputs, | ||
| dependencies = portDesc.dependencies.map(idx => PortIdentity(idx)) | ||
| ) | ||
| } | ||
| } else { | ||
| List(InputPort(PortIdentity(), allowMultiLinks = true)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to keep the PortIdentity() part.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will return the same result.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| List(InputPort()) | ||
| } | ||
| val outputPortInfo = if (outputPorts != null) { | ||
| outputPorts.zipWithIndex.map { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,12 +85,10 @@ class BubbleChartOpDesc extends PythonOperatorDescriptor { | |
| } | ||
|
|
||
| override def operatorInfo: OperatorInfo = | ||
| OperatorInfo( | ||
| OperatorInfo.forVisualization( | ||
| "Bubble Chart", | ||
| "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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| OperatorGroupConstants.VISUALIZATION_BASIC_GROUP | ||
| ) | ||
|
|
||
| def manipulateTable(): PythonTemplateBuilder = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ import org.apache.texera.amber.operator.PythonOperatorDescriptor | |
| import org.apache.texera.amber.operator.metadata.annotations.AutofillAttributeName | ||
| import org.apache.texera.amber.operator.metadata.{OperatorGroupConstants, OperatorInfo} | ||
|
|
||
| import java.util | ||
| import java.util.{ArrayList, List => JList} | ||
| import scala.jdk.CollectionConverters._ | ||
|
|
||
|
|
@@ -57,7 +58,7 @@ class BulletChartOpDesc extends PythonOperatorDescriptor { | |
| @JsonProperty(value = "steps", required = false) | ||
| @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]() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done by formatter.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also checked, it's a better practice. |
||
|
|
||
| override def getOutputSchemas( | ||
| inputSchemas: Map[PortIdentity, Schema] | ||
|
|
@@ -67,13 +68,11 @@ class BulletChartOpDesc extends PythonOperatorDescriptor { | |
| } | ||
|
|
||
| override def operatorInfo: OperatorInfo = | ||
| OperatorInfo( | ||
| OperatorInfo.forVisualization( | ||
| "Bullet Chart", | ||
| """Visualize data using a Bullet Chart that shows a primary quantitative bar and delta indicator. | ||
| |Optional elements such as qualitative ranges (steps) and a performance threshold are displayed only when provided.""".stripMargin, | ||
| OperatorGroupConstants.VISUALIZATION_FINANCIAL_GROUP, | ||
| inputPorts = List(InputPort()), | ||
| outputPorts = List(OutputPort(mode = OutputMode.SINGLE_SNAPSHOT)) | ||
| OperatorGroupConstants.VISUALIZATION_FINANCIAL_GROUP | ||
| ) | ||
|
|
||
| override def generatePythonCode(): String = { | ||
|
|
||

There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.