Copilot commented on code in PR #28500:
URL: https://github.com/apache/flink/pull/28500#discussion_r3735211889
##########
flink-table/flink-table-api-scala-bridge/src/main/scala/org/apache/flink/table/api/bridge/scala/StreamTableEnvironment.scala:
##########
@@ -833,7 +835,13 @@ object StreamTableEnvironment {
* The Scala [[StreamExecutionEnvironment]] of the [[TableEnvironment]].
*/
def create(executionEnvironment: StreamExecutionEnvironment):
StreamTableEnvironment = {
- create(executionEnvironment, EnvironmentSettings.newInstance().build)
+ val runtimeMode =
+ if
(executionEnvironment.getConfiguration.get(ExecutionOptions.RUNTIME_MODE) ==
+ RuntimeExecutionMode.BATCH) RuntimeExecutionMode.BATCH
+ else RuntimeExecutionMode.STREAMING
+ create(
+ executionEnvironment,
+ EnvironmentSettings.newInstance().inMode(runtimeMode).build)
Review Comment:
The runtime mode derivation maps anything other than BATCH to STREAMING. For
RuntimeExecutionMode.AUTOMATIC this silently creates a STREAMING planner while
the underlying StreamExecutionEnvironment stays AUTOMATIC, which still violates
the goal of inheriting the execution mode. Consider failing fast for AUTOMATIC
with a TableException (Table API supports only explicit BATCH/STREAMING) and
otherwise passing through the actual enum value.
##########
flink-table/flink-table-api-java-bridge/src/main/java/org/apache/flink/table/api/bridge/java/StreamTableEnvironment.java:
##########
@@ -88,7 +90,13 @@ public interface StreamTableEnvironment extends
TableEnvironment {
* TableEnvironment}.
*/
static StreamTableEnvironment create(StreamExecutionEnvironment
executionEnvironment) {
- return create(executionEnvironment,
EnvironmentSettings.newInstance().build());
+ final RuntimeExecutionMode runtimeMode =
+
executionEnvironment.getConfiguration().get(ExecutionOptions.RUNTIME_MODE)
+ == RuntimeExecutionMode.BATCH
+ ? RuntimeExecutionMode.BATCH
+ : RuntimeExecutionMode.STREAMING;
+ return create(
+ executionEnvironment,
EnvironmentSettings.newInstance().inMode(runtimeMode).build());
Review Comment:
The runtime mode derivation collapses any non-BATCH value to STREAMING. If
the execution environment is configured with RuntimeExecutionMode.AUTOMATIC,
this silently creates a STREAMING planner while the StreamExecutionEnvironment
remains AUTOMATIC, leaving the execution/planner mode mismatch unresolved.
Either propagate the actual enum and reject unsupported modes, or explicitly
fail fast for AUTOMATIC with a clear error (Table API supports only
BATCH/STREAMING).
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/table/StreamTableEnvironmentRuntimeModeTest.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.planner.runtime.stream.table;
+
+import org.apache.flink.api.common.RuntimeExecutionMode;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.ExecutionOptions;
+import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment;
+import org.apache.flink.table.api.bridge.java.StreamTableEnvironment;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Tests that {@link
StreamTableEnvironment#create(StreamExecutionEnvironment)} inherits the runtime
+ * execution mode from the given {@link StreamExecutionEnvironment} instead of
always defaulting to
+ * streaming (FLINK-39014).
+ */
Review Comment:
The PR description mentions adding an integration test in
WatermarkExampleITCase, but this PR actually adds
StreamTableEnvironmentRuntimeModeTest instead. Please update the PR description
(or add the mentioned test) so reviewers can accurately understand what was
changed and how it was verified.
This issue also appears on line 51 of the same file.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/EnvironmentSettings.java:
##########
@@ -187,6 +188,12 @@ public Builder inStreamingMode() {
return this;
}
+ /** Sets the {@link RuntimeExecutionMode} that the components should
work in. */
+ public Builder inMode(RuntimeExecutionMode mode) {
+ configuration.set(RUNTIME_MODE, mode);
+ return this;
+ }
Review Comment:
EnvironmentSettings.Builder.inMode currently accepts any
RuntimeExecutionMode, but Table API only supports explicit BATCH or STREAMING
(DefaultPlannerFactory throws for other modes). Adding a guard (and updating
the JavaDoc) would prevent users from configuring AUTOMATIC via this new public
API and only discovering the problem later.
--
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]
