You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "xiangyuf (via GitHub)" <gi...@apache.org> on 2023/09/24 14:06:59 UTC
[GitHub] [flink] xiangyuf opened a new pull request, #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries to sql gateway
xiangyuf opened a new pull request, #23455:
URL: https://github.com/apache/flink/pull/23455
## What is the purpose of the change
when submitting queries use table api, the job name should not always be 'collect'. Using sql as the job name will improve the usability.
## Brief change log
- Added a new interface: QuerySqlOperation, supporting getting detailed query sql string.
- PlannerQueryOperation changes to implement interface QuerySqlOperation, the query sql is getting from SqlNodeToOperationConversion#getQuotedSqlString and set it to PlannerQueryOperation through the construct method
- When submitting queries, jobName will be set as sql for QuerySqlOperation
## Verifying this change
Test manually by submitting queries to sql gateway, the jobName changes to sql
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] KarmaGYZ commented on a diff in pull request #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client
Posted by "KarmaGYZ (via GitHub)" <gi...@apache.org>.
KarmaGYZ commented on code in PR #23455:
URL: https://github.com/apache/flink/pull/23455#discussion_r1336669036
##########
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:
If it is a special requirement, we may introduce another interface like "SqlContentSupplier". Make this class implement "QueryOperation" and "QuerySqlSupplier".
##########
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:
Why only this class needs to implement the newly introduced protocol?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] xiangyuf commented on a diff in pull request #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client
Posted by "xiangyuf (via GitHub)" <gi...@apache.org>.
xiangyuf commented on code in PR #23455:
URL: https://github.com/apache/flink/pull/23455#discussion_r1336585759
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java:
##########
@@ -1050,10 +1052,15 @@ private TableResultInternal executeInternal(
}
private TableResultInternal executeQueryOperation(QueryOperation operation) {
+ String querySql = null;
+ if (operation instanceof QuerySqlOperation) {
+ querySql = ((QuerySqlOperation) operation).getQuerySql();
+ }
CollectModifyOperation sinkOperation = new CollectModifyOperation(operation);
List<Transformation<?>> transformations =
translate(Collections.singletonList(sinkOperation));
- final String defaultJobName = "collect";
+ final String defaultJobName =
Review Comment:
@FangYongs hi, in community version, only jobId will be used in the checkpoint path.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] FangYongs commented on a diff in pull request #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client
Posted by "FangYongs (via GitHub)" <gi...@apache.org>.
FangYongs commented on code in PR #23455:
URL: https://github.com/apache/flink/pull/23455#discussion_r1335701445
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java:
##########
@@ -1050,10 +1052,15 @@ private TableResultInternal executeInternal(
}
private TableResultInternal executeQueryOperation(QueryOperation operation) {
+ String querySql = null;
+ if (operation instanceof QuerySqlOperation) {
+ querySql = ((QuerySqlOperation) operation).getQuerySql();
+ }
CollectModifyOperation sinkOperation = new CollectModifyOperation(operation);
List<Transformation<?>> transformations =
translate(Collections.singletonList(sinkOperation));
- final String defaultJobName = "collect";
+ final String defaultJobName =
Review Comment:
Job name will be used in the checkpoint path, is it ok when there're some special characters in the sql statement?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-25015][Table SQL] Use SQL string as jobName for DQL jobs submitted by sql-gateway [flink]
Posted by "xiangyuf (via GitHub)" <gi...@apache.org>.
xiangyuf commented on PR #23455:
URL: https://github.com/apache/flink/pull/23455#issuecomment-1951356537
Hi @libenchao @KarmaGYZ , can you kindly help review this when you have time?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-25015][Table SQL] Use SQL string as jobName for DQL jobs submitted by sql-gateway [flink]
Posted by "KarmaGYZ (via GitHub)" <gi...@apache.org>.
KarmaGYZ commented on PR #23455:
URL: https://github.com/apache/flink/pull/23455#issuecomment-1951630056
LGTM
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client [flink]
Posted by "xiangyuf (via GitHub)" <gi...@apache.org>.
xiangyuf commented on PR #23455:
URL: https://github.com/apache/flink/pull/23455#issuecomment-1752322197
@flinkbot run azure
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] davidradl commented on a diff in pull request #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #23455:
URL: https://github.com/apache/flink/pull/23455#discussion_r1337682156
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java:
##########
@@ -1050,10 +1052,15 @@ private TableResultInternal executeInternal(
}
private TableResultInternal executeQueryOperation(QueryOperation operation) {
+ String querySql = null;
+ if (operation instanceof QuerySqlOperation) {
+ querySql = ((QuerySqlOperation) operation).getQuerySql();
+ }
CollectModifyOperation sinkOperation = new CollectModifyOperation(operation);
List<Transformation<?>> transformations =
translate(Collections.singletonList(sinkOperation));
- final String defaultJobName = "collect";
+ final String defaultJobName =
+ StringUtils.isNullOrWhitespaceOnly(querySql) ? "collect" : querySql;
Review Comment:
We could have an enumeration that would hold the type of the operation ; this field would be defined at the top of the hierarchy. Then to check the type of the operation, it is a comparison on this type rather than having to use reflection.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-25015][Table SQL] Use SQL string as jobName for DQL jobs submitted by sql-gateway [flink]
Posted by "KarmaGYZ (via GitHub)" <gi...@apache.org>.
KarmaGYZ merged PR #23455:
URL: https://github.com/apache/flink/pull/23455
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] FangYongs commented on a diff in pull request #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client
Posted by "FangYongs (via GitHub)" <gi...@apache.org>.
FangYongs commented on code in PR #23455:
URL: https://github.com/apache/flink/pull/23455#discussion_r1335703134
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java:
##########
@@ -1050,10 +1052,15 @@ private TableResultInternal executeInternal(
}
private TableResultInternal executeQueryOperation(QueryOperation operation) {
+ String querySql = null;
+ if (operation instanceof QuerySqlOperation) {
+ querySql = ((QuerySqlOperation) operation).getQuerySql();
+ }
CollectModifyOperation sinkOperation = new CollectModifyOperation(operation);
List<Transformation<?>> transformations =
translate(Collections.singletonList(sinkOperation));
- final String defaultJobName = "collect";
+ final String defaultJobName =
+ StringUtils.isNullOrWhitespaceOnly(querySql) ? "collect" : querySql;
Review Comment:
Add test units to check that the job name is modified
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] davidradl commented on a diff in pull request #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #23455:
URL: https://github.com/apache/flink/pull/23455#discussion_r1337500260
##########
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:
As is there is a sqlQuery property in a PlannerQueryOperation that might not be sql. I wonder if there should be a specialisation of PlannerQueryOperation as SQLPlannerQueryOperation - that is only used for SQL cases. This seems more natural.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #23455: [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries to sql gateway
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23455:
URL: https://github.com/apache/flink/pull/23455#issuecomment-1732578640
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7d4b9f4e116cc26f19b0c085ef1f94ea8bc2eb43",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "7d4b9f4e116cc26f19b0c085ef1f94ea8bc2eb43",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7d4b9f4e116cc26f19b0c085ef1f94ea8bc2eb43 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-25015][Table SQL/Client] change job name to sql when submitting queries using client [flink]
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org