twalthr commented on a change in pull request #18621:
URL: https://github.com/apache/flink/pull/18621#discussion_r800857849



##########
File path: 
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/ExecNodeContext.java
##########
@@ -113,6 +113,11 @@ public Integer getVersion() {
         return version;
     }
 
+    /** Returns a new {@code uid} for transformations. */
+    public String generateUid(String transformationName) {
+        return String.format("%s-%s-%s", getId(), getTypeAsString(), 
transformationName);

Review comment:
       the previous solution was fine, my comment was only referring to perform 
a regex check that `transformationName` is always lower case with dashes. we 
will also expose this method to users in one of the next PRs.

##########
File path: 
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/batch/BatchExecCalc.java
##########
@@ -53,4 +55,11 @@ public BatchExecCalc(
                 outputType,
                 description);
     }
+
+    @Override
+    protected TransformationMetadata createTransformationMeta(TableConfig 
tableConfig) {
+        return new TransformationMetadata(

Review comment:
       instead of adding this method to many batch node. perform an instanceof 
check and simlar to 
`org.apache.flink.table.planner.plan.nodes.exec.ExecNodeContext#newContext` you 
call the corresponding constructor

##########
File path: 
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecCalc.java
##########
@@ -42,10 +44,13 @@
 @ExecNodeMetadata(
         name = "stream-exec-calc",
         version = 1,
+        producedTransformations = StreamExecCalc.SUBSTITUTE_TRANSFORMATION,
         minPlanVersion = FlinkVersion.v1_15,
         minStateVersion = FlinkVersion.v1_15)
 public class StreamExecCalc extends CommonExecCalc implements 
StreamExecNode<RowData> {
 
+    public static final String SUBSTITUTE_TRANSFORMATION = "substitute";

Review comment:
       again, let's call this differently. `substitute` makes no sense, it can 
be `calc`




-- 
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]


Reply via email to