libenchao commented on code in PR #23455:
URL: https://github.com/apache/flink/pull/23455#discussion_r1356057145
##########
flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/api/TableEnvironmentTest.java:
##########
@@ -127,6 +130,25 @@ void testTableFromDescriptor() {
assertThat(tEnv.getCatalogManager().listTables()).isEmpty();
}
+ @Test
+ void testGetQueryOperationDefaultJobName() {
+ final TableEnvironmentMock tEnv =
TableEnvironmentMock.getStreamingInstance();
+ String sql1 = "select * from t";
+ String sql2 = "";
+
+ QueryOperation mockOperation1 = new QuerySqlOperationMock(sql1);
+ QueryOperation mockOperation2 = new QuerySqlOperationMock(sql2);
+ QueryOperation mockOperation3 = new QueryOperationMock();
+
+ String jobName1 = tEnv.getDefaultJobName(mockOperation1);
Review Comment:
The test seems only verifying `TableEnvironmentMock#getDefaultJobName`
method. Is it possible to have tests that verify the whole process (something
we can call integration tests).
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeToOperationConversion.java:
##########
@@ -1379,6 +1379,6 @@ private String getQuotedSqlString(SqlNode sqlNode) {
private PlannerQueryOperation toQueryOperation(FlinkPlannerImpl planner,
SqlNode validated) {
// transform to a relational tree
RelRoot relational = planner.rel(validated);
- return new PlannerQueryOperation(relational.project());
+ return new PlannerQueryOperation(relational.project(),
getQuotedSqlString(validated));
Review Comment:
`SqlNodeToOperationConversion#toQueryOperation` seems only used in explain.
I'm not sure how you did the manually testing, IIUC, the job name cannot be
changed for select queries?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/PlannerQueryOperation.java:
##########
@@ -36,13 +37,18 @@
/** Wrapper for valid logical plans generated by Planner. */
@Internal
-public class PlannerQueryOperation implements QueryOperation {
-
+public class PlannerQueryOperation implements QuerySqlOperation {
Review Comment:
I share the same concern that is it necessary to add a new interface with
only one implementation?
Besides this way, I noticed that `Operation` already have a
`asSummaryString` method, do you think if it's ok to use it in this case?
(Anyway, the `PlannerQueryOperation#asSummaryString` should be improved, it
actually does not have children, and it should add the info derived from the
`RelNode` to the summary string)
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
