twalthr commented on a change in pull request #18885:
URL: https://github.com/apache/flink/pull/18885#discussion_r816761680
##########
File path:
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/TableEnvironment.java
##########
@@ -1103,54 +1103,21 @@ void createFunction(
*
* <p>Note: The compiled plan feature is not supported in batch mode.
*
- * @see #executePlan(CompiledPlan)
+ * @see CompiledPlan#execute()
* @see #loadPlan(PlanReference)
* @throws TableException if the SQL statement is invalid or if the plan
cannot be persisted.
*/
@Experimental
CompiledPlan compilePlanSql(String stmt) throws TableException;
/**
- * Executes the provided {@link CompiledPlan}.
+ * Shorthand for {@code tEnv.loadPlan(planReference).execute()}.
*
- * <p>Compiled plans can be persisted and reloaded across Flink versions.
They describe static
- * pipelines to ensure backwards compatibility and enable stateful
streaming job upgrades. See
- * {@link CompiledPlan} and the website documentation for more information.
- *
- * <p>If a job is resumed from a savepoint, it will eventually resume the
execution.
- *
- * <p>Note: The compiled plan feature is not supported in batch mode.
- *
- * @see #compilePlanSql(String)
* @see #loadPlan(PlanReference)
- */
- @Experimental
- TableResult executePlan(CompiledPlan plan);
-
- /**
- * Shorthand for {@code tEnv.executePlan(tEnv.loadPlan(planReference))}.
- *
- * @see #loadPlan(PlanReference)
- * @see #executePlan(CompiledPlan)
+ * @see CompiledPlan#execute()
Review comment:
sorry, I missed this
##########
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:
The `Explainable` was working before. We are just discussing
`Executable` here. I don't think it makes a difference whether we throw an
`UnsupportedOperationException` for `execute()` or throw an exception if the
dependency injection has not performed.
Passing a `Function<CompiledPlan, TableResult>` argument is also not nice.
I don't have a better idea. So let's just shorten the name to
`CompiledPlanFactory` and stick to this design. But I have the feeling the
class structure gets more and more complex for no reason. Also the need for
`CompiledPlanInternal` is weak. `TableEnvironmentInternal` has actually a
comment about `Once old planner is removed, this class also can be removed.`
--
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]
