spuru9 commented on code in PR #28500: URL: https://github.com/apache/flink/pull/28500#discussion_r3451581732
########## flink-examples/flink-examples-table/src/test/java/org/apache/flink/table/examples/java/basics/WatermarkExampleITCase.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.examples.java.basics; + +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.TableResult; +import org.apache.flink.table.api.bridge.java.StreamTableEnvironment; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Test for Java Watermark Query in Batch and Streaming mode. */ +class WatermarkExampleITCase { + private static final String createTableA = + "CREATE TABLE a (" + + "testTime BIGINT NOT NULL, " + + "eventTime AS TO_TIMESTAMP_LTZ(testTime, 3), " + + "WATERMARK FOR eventTime AS eventTime - INTERVAL '5' SECOND" + + ") WITH (" + + "'connector' = 'datagen', " + + "'number-of-rows' = '10'" + + ")"; + private static final String createTableB = + "CREATE TABLE b (" + + "testTime BIGINT NOT NULL, " + + "eventTime TIMESTAMP_LTZ(3), " + + "WATERMARK FOR eventTime AS eventTime - INTERVAL '5' SECOND" + + ") WITH (" + + "'connector' = 'filesystem', " + + "'format' = 'csv', " + + "'path' = '/tmp/'" + + ")"; + private static final String insertIntoB = "INSERT INTO b SELECT * FROM a"; + private static final String selectFromB = "SELECT * FROM b"; + private static final String testTimeStr = "testTime"; + private static final String eventTimeStr = "eventTime"; + + @Test + void testWatermarkInBatchMode() { + Configuration configuration = new Configuration(); + configuration.setString( + ExecutionOptions.RUNTIME_MODE.key(), RuntimeExecutionMode.BATCH.toString()); Review Comment: nit: Use the typed config setter instead of setString(opt.key(), enum.toString()) ```suggestion configuration.set(ExecutionOptions.RUNTIME_MODE, RuntimeExecutionMode.BATCH); ``` Similarly for other similar statements. ########## flink-examples/flink-examples-table/src/test/java/org/apache/flink/table/examples/java/basics/WatermarkExampleITCase.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.examples.java.basics; Review Comment: This looks misplaced. flink-examples-table/.../basics tests cover specific example programs — there's no WatermarkExample, and this test is really exercising StreamTableEnvironment.create. It most probably belongs next to the code it covers (e.g. an ITCase under flink-table-api-java-bridge). ########## flink-table/flink-table-api-java-bridge/src/main/java/org/apache/flink/table/api/bridge/java/StreamTableEnvironment.java: ########## @@ -88,7 +88,13 @@ public interface StreamTableEnvironment extends TableEnvironment { * TableEnvironment}. */ static StreamTableEnvironment create(StreamExecutionEnvironment executionEnvironment) { - return create(executionEnvironment, EnvironmentSettings.newInstance().build()); + return create( Review Comment: The Scala bridge has the identical bug — create(executionEnvironment, EnvironmentSettings.newInstance().build) ignores the env's runtime mode. A user hitting FLINK-39014 from Scala won't be fixed by this PR. Worth fixing in this PR (or at least noting in the JIRA as a follow-up) so the two bridges stay consistent. ########## flink-table/flink-table-api-java-bridge/src/main/java/org/apache/flink/table/api/bridge/java/StreamTableEnvironment.java: ########## @@ -88,7 +88,13 @@ public interface StreamTableEnvironment extends TableEnvironment { * TableEnvironment}. */ static StreamTableEnvironment create(StreamExecutionEnvironment executionEnvironment) { - return create(executionEnvironment, EnvironmentSettings.newInstance().build()); + return create( + executionEnvironment, + EnvironmentSettings.newInstance() + .withConfiguration( + (org.apache.flink.configuration.Configuration) Review Comment: executionEnvironment.getConfiguration() returns ReadableConfig, not Configuration, and the source of that method explicitly warns against this cast: ``` // Note to implementers: // In theory, you can cast the return value of this method to Configuration and perform // mutations. In practice, this could cause side effects. A better approach is to implement // the ReadableConfig interface and create a layered configuration. ``` Could be something like: ``` static StreamTableEnvironment create(StreamExecutionEnvironment executionEnvironment) { final EnvironmentSettings.Builder settingsBuilder = EnvironmentSettings.newInstance(); if (executionEnvironment.getConfiguration().get(ExecutionOptions.RUNTIME_MODE) == RuntimeExecutionMode.BATCH) { settingsBuilder.inBatchMode(); } return create(executionEnvironment, settingsBuilder.build()); } ``` Can you check this. -- 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]
