[GitHub] [flink] slinkydeveloper commented on a change in pull request #18885: [FLINK-26131][table] CompiledPlan implements Executable

[GitBox <gi...@apache.org>]

slinkydeveloper commented on a change in pull request #18885:
URL: https://github.com/apache/flink/pull/18885#discussion_r816630290



##########
File path: flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/utils/PlannerMock.java
##########
@@ -50,12 +50,12 @@ public String explain(List<Operation> operations, ExplainDetail... extraDetails)
     }
 
     @Override
-    public CompiledPlan loadPlan(PlanReference planReference) throws IOException {
+    public CompiledPlanInternalFactory loadPlan(PlanReference planReference) throws IOException {

Review comment:
       TBH I think this is the way it should look, as this factory just underlines how I need a dependency injection before being able to use an object fulfilling the `CompiledPlan` interface. The fact that the name is not nice is a different problem, we can rename it to `CompiledPlanFactory` if you prefer.
   
   IMHO using `UnsupportedOperationException` is not correct, as the object returned by `loadPlan` here would not respect the interface `CompiledPlan`, which is `Executable` and `Explainable`. It also makes reasoning about this code and the functionalities of these interfaces harder, as now I need to go look into the specific `ExecNodeCompiledPlan` implementation to find out that I need an additional wrapping to be able to implement the full `CompiledPlan` interface. I rather prefer to make `ExecNodeCompiledPlan` mutable and clarify that I need additional dependency injection before being able to use it.




-- 
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-unsubscribe@flink.apache.org

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