You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "lihaosky (via GitHub)" <gi...@apache.org> on 2025/05/05 23:43:35 UTC
[PR] [FLINK-35019] support convert show create model sql to operation [flink]
lihaosky opened a new pull request, #26528:
URL: https://github.com/apache/flink/pull/26528
## What is the purpose of the change
support convert `show create model` sql to operation.
## Brief change log
* Add converter to convert `show create model` sql to operation.
## Verifying this change
Unit test
## 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? (yes)
- If yes, how is the feature documented? (JavaDocs)
--
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-35019] support convert show create model sql to operation [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2077909232
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverters.java:
##########
@@ -53,6 +53,7 @@ public class SqlNodeConverters {
register(new SqlTruncateTableConverter());
register(new SqlShowFunctionsConverter());
register(new SqlShowModelsConverter());
+ register(new SqlShowCreateModelConverter());
Review Comment:
I am curious - are we missing a showCreateTableConverter in this list?
--
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-35019] support convert show create model sql to operation [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #26528:
URL: https://github.com/apache/flink/pull/26528#issuecomment-2852726466
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5f2ba2d3bb1be6a85d09a4bb39bfb208837cc4d4",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "5f2ba2d3bb1be6a85d09a4bb39bfb208837cc4d4",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5f2ba2d3bb1be6a85d09a4bb39bfb208837cc4d4 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-35019] support convert show create model sql to operation [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2077928618
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/api/TableEnvironmentTest.scala:
##########
@@ -2957,6 +2957,136 @@ class TableEnvironmentTest {
checkData(expectedResult1.iterator(), modelResult2.collect())
}
+ @Test
Review Comment:
As AI endpoints can involve complex objects. I suggest adding some tests with more complex objects, arrays of arrays , arrays of objects, nested objects and arrays of nested objects.
--
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-35019] support convert show create model sql to operation [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2077909232
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverters.java:
##########
@@ -53,6 +53,7 @@ public class SqlNodeConverters {
register(new SqlTruncateTableConverter());
register(new SqlShowFunctionsConverter());
register(new SqlShowModelsConverter());
+ register(new SqlShowCreateModelConverter());
Review Comment:
I am curious - are we missing a showCreateTableConverter?
--
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-35019] support convert show create model sql to operation [flink]
Posted by "fsk119 (via GitHub)" <gi...@apache.org>.
fsk119 commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2083722961
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -177,6 +206,17 @@ static String extractFormattedColumns(ResolvedCatalogBaseTable<?> table, String
.collect(Collectors.joining(",\n"));
}
+ static Optional<String> extractFormattedColumns(ResolvedSchema schema) {
Review Comment:
do we need printIndent here? cc line 203.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -48,6 +51,23 @@ public class ShowCreateUtil {
private ShowCreateUtil() {}
+ public static String buildShowCreateModelRow(
+ ResolvedCatalogModel model, ObjectIdentifier modelIdentifier, boolean isTemporary) {
+ StringBuilder sb =
+ new StringBuilder()
+ .append(buildCreateModelFormattedPrefix(modelIdentifier, isTemporary));
+ extractFormattedColumns(model.getResolvedInputSchema())
+ .ifPresent(
+ c -> sb.append(String.format("INPUT (%s)%s", c, System.lineSeparator())));
+ extractFormattedColumns(model.getResolvedOutputSchema())
+ .ifPresent(
+ c -> sb.append(String.format("OUTPUT (%s)%s", c, System.lineSeparator())));
+ extractComment(model).ifPresent(c -> sb.append(formatComment(c)).append("\n"));
Review Comment:
use System.lineSeparator() to replace '\n'
--
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-35019] support convert show create model sql to operation [flink]
Posted by "fsk119 (via GitHub)" <gi...@apache.org>.
fsk119 commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2083710337
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -130,6 +150,15 @@ static String buildCreateFormattedPrefix(
System.lineSeparator());
}
+ static String buildCreateModelFormattedPrefix(
Review Comment:
It's better we can reuse the util `buildCreateFormattedPrefix`.
```
static String buildCreateFormattedPrefix(
String objectType, boolean isTemporary, ObjectIdentifier identifier) {
return String.format(
"CREATE %s%s %s (%s",
isTemporary ? "TEMPORARY " : "",
tableType,
identifier.asSerializableString(),
System.lineSeparator());
}
```
--
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-35019] support convert show create model sql to operation [flink]
Posted by "lihaosky (via GitHub)" <gi...@apache.org>.
lihaosky commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2077932178
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverters.java:
##########
@@ -53,6 +53,7 @@ public class SqlNodeConverters {
register(new SqlTruncateTableConverter());
register(new SqlShowFunctionsConverter());
register(new SqlShowModelsConverter());
+ register(new SqlShowCreateModelConverter());
Review Comment:
It's not using this pattern: https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeToOperationConversion.java#L322
The converters pattern is introduced after table operations added. There can be followups to migration table operations to converters maybe
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/api/TableEnvironmentTest.scala:
##########
@@ -2957,6 +2957,136 @@ class TableEnvironmentTest {
checkData(expectedResult1.iterator(), modelResult2.collect())
}
+ @Test
Review Comment:
Sure I'll add more types
--
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-35019] support convert show create model sql to operation [flink]
Posted by "lihaosky (via GitHub)" <gi...@apache.org>.
lihaosky commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2078311124
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/api/TableEnvironmentTest.scala:
##########
@@ -2957,6 +2957,177 @@ class TableEnvironmentTest {
checkData(expectedResult1.iterator(), modelResult2.collect())
}
+ @Test
+ def testShowCreateModel(): Unit = {
+ val sourceDDL =
+ """
+ |CREATE MODEL M1
+ | INPUT(f0 char(10), f1 varchar(10))
+ | OUTPUT(f2 string)
+ |COMMENT 'this is a model'
+ |with (
+ | 'task' = 'clustering',
+ | 'provider' = 'openai',
+ | 'openai.endpoint' = 'some-endpoint'
+ |)
+ """.stripMargin
+
+ tableEnv.executeSql(sourceDDL)
+
+ val expectedDDL =
+ """|CREATE MODEL `default_catalog`.`default_database`.`M1`
+ |INPUT (`f0` CHAR(10), `f1` VARCHAR(10))
+ |OUTPUT (`f2` VARCHAR(2147483647))
+ |COMMENT 'this is a model'
+ |WITH (
+ | 'openai.endpoint' = 'some-endpoint',
+ | 'provider' = 'openai',
+ | 'task' = 'clustering'
+ |)
+ |""".stripMargin
+ val row = tableEnv.executeSql("SHOW CREATE MODEL M1").collect().next()
+ assertEquals(expectedDDL, row.getField(0))
+ }
+
+ @Test
+ def testShowCreateModelComplexTypes(): Unit = {
Review Comment:
@davidradl , added these using AI 😉
--
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-35019] support convert show create model sql to operation [flink]
Posted by "lihaosky (via GitHub)" <gi...@apache.org>.
lihaosky commented on code in PR #26528:
URL: https://github.com/apache/flink/pull/26528#discussion_r2085179230
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/ShowCreateUtil.java:
##########
@@ -177,6 +206,17 @@ static String extractFormattedColumns(ResolvedCatalogBaseTable<?> table, String
.collect(Collectors.joining(",\n"));
}
+ static Optional<String> extractFormattedColumns(ResolvedSchema schema) {
Review Comment:
This is formatted differently from table. In table, each column is a row (joined by `\n'). In model, columns for `input` and `output` are in same row respectively (joined by `,`). I feel it's ok since the input/output columns for model won't be too many. wdyt?
--
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-35019] support convert show create model sql to operation [flink]
Posted by "fsk119 (via GitHub)" <gi...@apache.org>.
fsk119 merged PR #26528:
URL: https://github.com/apache/flink/pull/26528
--
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