[GitHub] [flink] Airblader commented on a change in pull request #17217: [FLINK-24186][table-planner] Allow multiple rowtime attributes for collect() and print()

[GitBox]

Airblader commented on a change in pull request #17217:
URL: https://github.com/apache/flink/pull/17217#discussion_r705303998



##########
File path: 
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecSink.java
##########
@@ -120,13 +121,16 @@ public StreamExecSink(
                 rowtimeFieldIndices.add(i);
             }
         }
+
+        final boolean isCollectSink = tableSinkSpec.getTableSink() instanceof 
CollectDynamicSink;

Review comment:
       > Wouldn't it be "cleaner" if an isCollectSink() method (or 
simplyisCollect()) is introduced to the iface?
   
   `DynamicTableSink` is a public API, so we'd be throwing a method into it for 
internal purposes. Such a method also breaks the abstraction, because you're 
asking the interface for whether it is a specific implementation.
   
   > Or alternatively, if the method is implemented in a utility class in that 
package, so that changing the class to public just for the purpose of 
instanceof call is avoided?
   
   Then you'd just have to make that utility public instead, no?




-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org