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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
