wuchong commented on code in PR #20247:
URL: https://github.com/apache/flink/pull/20247#discussion_r928462089
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/DialectFactory.java:
##########
@@ -20,37 +20,52 @@
import org.apache.flink.annotation.Internal;
import org.apache.flink.table.api.config.TableConfigOptions;
+import org.apache.flink.table.api.internal.TableResultInternal;
import org.apache.flink.table.catalog.CatalogManager;
+import org.apache.flink.table.delegation.Executor;
+import org.apache.flink.table.delegation.OperationExternalExecutor;
import org.apache.flink.table.delegation.Parser;
import org.apache.flink.table.factories.Factory;
+import org.apache.flink.table.operations.Operation;
+
+import java.util.Optional;
/**
- * Factory that creates {@link Parser}.
+ * Factory that creates {@link Parser} and {@link OperationExternalExecutor}.
*
* <p>The {@link #factoryIdentifier()} is identified by matching it against
{@link
* TableConfigOptions#TABLE_SQL_DIALECT}.
*/
@Internal
-public interface ParserFactory extends Factory {
+public interface DialectFactory extends Factory {
/** Creates a new parser. */
Parser create(Context context);
+ default OperationExternalExecutor createOperatorExternalExecutor(Context
context) {
Review Comment:
`External` might be misleading here, what about `ExtendedOperationExecutor`?
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentInternal.java:
##########
@@ -81,6 +90,15 @@ public interface TableEnvironmentInternal extends
TableEnvironment {
* @param operation The operation to be executed.
* @return the content of the execution result.
*/
+ TableResultInternal executeOperation(Operation operation);
Review Comment:
This method is quite confusing that what's the difference between this one
and `executeInternal`. Besides, why there are no
`executeOperation(List<ModifyOperation> operations)` ?
Couldn't we just implement the delegation in `executeInternal`?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/delegation/DialectFactory.java:
##########
@@ -62,5 +77,20 @@ public CatalogManager getCatalogManager() {
public PlannerContext getPlannerContext() {
return plannerContext;
}
+
+ @Override
+ public Executor getExecutor() {
+ return executor;
+ }
+ }
+
+ /** Default implementation for {@link OperationExternalExecutor}. */
+ class DefaultOperationExternalExecutor implements
OperationExternalExecutor {
Review Comment:
The default implementation can be `EmptyOperationExecutor` with comment: a
default implementation of `ExtendedOperationExecutor` that doesn't extend any
operation behavior but forward all operations to the Flink planner.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentInternal.java:
##########
@@ -53,6 +54,14 @@ public interface TableEnvironmentInternal extends
TableEnvironment {
*/
Parser getParser();
+ /**
+ * Return a {@link OperationExternalExecutor} that provides method for
executing operation in
+ * external implementation.
+ *
+ * @return initialized {@link OperationExternalExecutor}.
+ */
+ OperationExternalExecutor getOperationExternalExecutor();
Review Comment:
It seems this method is only called by the implementation itself. So no need
to expose 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]
