Re: [PR] [FLINK-34993] parser changes to support model CRUD sql [flink]
twalthr merged PR #24769: URL: https://github.com/apache/flink/pull/24769 -- 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
Re: [PR] [FLINK-34993] parser changes to support model CRUD sql [flink]
lihaosky commented on PR #24769: URL: https://github.com/apache/flink/pull/24769#issuecomment-2127940071 @twalthr , does this repo have CI build? I can't see it -- 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
Re: [PR] [FLINK-34993] parser changes to support model CRUD sql [flink]
lihaosky commented on code in PR #24769: URL: https://github.com/apache/flink/pull/24769#discussion_r1612241200 ## tools/maven/suppressions-parser.xml: ## @@ -0,0 +1,26 @@ + + + +http://www.puppycrawl.com/dtds/suppressions_1_1.dtd;> + + Review Comment: I saw some other components have dedicated file. Let me use `suppressions.xml` then -- 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
Re: [PR] [FLINK-34993] parser changes to support model CRUD sql [flink]
lihaosky commented on code in PR #24769: URL: https://github.com/apache/flink/pull/24769#discussion_r1612240751 ## flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/FlinkPlannerImpl.scala: ## @@ -147,6 +148,7 @@ class FlinkPlannerImpl( || sqlNode.isInstanceOf[SqlShowProcedures] || sqlNode.isInstanceOf[SqlShowJobs] || sqlNode.isInstanceOf[SqlDescribeJob] +|| sqlNode.isInstanceOf[SqlRichDescribeModel] Review Comment: It throws like this: ``` org.apache.flink.table.api.TableException: Unsupported query: DROP MODEL IF EXISTS M1 ``` -- 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
Re: [PR] [FLINK-34993] parser changes to support model CRUD sql [flink]
twalthr commented on code in PR #24769:
URL: https://github.com/apache/flink/pull/24769#discussion_r1609502734
##
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl:
##
@@ -3037,3 +3067,185 @@ SqlTruncateTable SqlTruncateTable() :
return new SqlTruncateTable(getPos(), sqlIdentifier);
}
}
+
+/**
+* SHOW MODELS [FROM [catalog.] database] sql call.
+*/
+SqlShowModels SqlShowModels() :
+{
+SqlIdentifier databaseName = null;
+SqlCharStringLiteral likeLiteral = null;
+String prep = null;
+boolean notLike = false;
+SqlParserPos pos;
+}
+{
+
+{ pos = getPos(); }
+[
+( { prep = "FROM"; } | { prep = "IN"; } )
+{ pos = getPos(); }
+databaseName = CompoundIdentifier()
+]
+[
+[
+
+{
+notLike = true;
+}
+]
+
+{
+String likeCondition = SqlParserUtil.parseString(token.image);
+likeLiteral = SqlLiteral.createCharString(likeCondition, getPos());
+}
+]
+{
+return new SqlShowModels(pos, prep, databaseName, notLike,
likeLiteral);
+}
+}
+
+/**
+* ALTER MODEL [IF EXISTS] modelName SET (property_key = property_val, ...)
+* ALTER MODEL [IF EXISTS] modelName RENAME TO newModelName
+*/
+SqlAlterModel SqlAlterModel() :
+{
+SqlParserPos startPos;
+boolean ifExists = false;
+SqlIdentifier modelIdentifier;
+SqlIdentifier newModelIdentifier = null;
+SqlNodeList modelOptionList = SqlNodeList.EMPTY;
+}
+{
+ { startPos = getPos(); }
+ifExists = IfExistsOpt()
+modelIdentifier = CompoundIdentifier()
+(
+LOOKAHEAD(2)
+
+newModelIdentifier = CompoundIdentifier()
+{
+return new SqlAlterModel(
+startPos.plus(getPos()),
+modelIdentifier,
+newModelIdentifier,
+ifExists);
+}
+|
+
+modelOptionList = TableProperties()
Review Comment:
rename `TableProperties()` to `ObjectOptions()` or just `Options()` for
consistency. I guess we are also using `TableProperties()` for the catalog WITH
clause? we should leave the code base in a better shape with every PR.
##
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java:
##
@@ -2996,6 +3011,144 @@ public void describeTo(Description description) {
};
}
+@Test
+void testShowModels() {
+sql("show models").ok("SHOW MODELS");
+sql("show models from db1").ok("SHOW MODELS FROM `DB1`");
+sql("show models from catalog1.db1").ok("SHOW MODELS FROM
`CATALOG1`.`DB1`");
+sql("show models in db1").ok("SHOW MODELS IN `DB1`");
+sql("show models in catalog1.db1").ok("SHOW MODELS IN
`CATALOG1`.`DB1`");
+}
+
+@Test
+void testDropModel() {
+sql("drop model m1").ok("DROP MODEL `M1`");
+sql("drop model db1.m1").ok("DROP MODEL `DB1`.`M1`");
+sql("drop model catalog1.db1.m1").ok("DROP MODEL
`CATALOG1`.`DB1`.`M1`");
+}
+
+@Test
+void testDropModelIfExists() {
+sql("drop model if exists catalog1.db1.m1")
+.ok("DROP MODEL IF EXISTS `CATALOG1`.`DB1`.`M1`");
+}
+
+@Test
+void testAlterModel() {
+final String sql = "alter model m1 set ('key1' = 'value1','key2' =
'value2')";
+final String expected =
+"ALTER MODEL `M1` SET (\n"
++ " 'key1' = 'value1',\n"
++ " 'key2' = 'value2'\n"
++ ")";
+sql(sql).ok(expected);
+}
+
+@Test
+void testAlterModelIfExists() {
+final String sql = "alter model if exists m1 set ('key1' =
'value1','key2' = 'value2')";
+final String expected =
+"ALTER MODEL IF EXISTS `M1` SET (\n"
++ " 'key1' = 'value1',\n"
++ " 'key2' = 'value2'\n"
++ ")";
+sql(sql).ok(expected);
+}
+
+@Test
+void testAlterModelRename() {
+final String sql = "alter model m1 rename to m2";
+final String expected = "ALTER MODEL `M1` RENAME TO `M2`";
+sql(sql).ok(expected);
+}
+
+@Test
+void testAlterModelRenameIfExists() {
+final String sql = "alter model if exists m1 rename to m2";
+final String expected = "ALTER MODEL IF EXISTS `M1` RENAME TO `M2`";
+sql(sql).ok(expected);
+}
+
+@Test
+void testCreateModel() {
+sql("create model m1\n"
++ " INPUT(col1 INT, col2 STRING)\n"
++ " OUTPUT(label DOUBLE)\n"
++ " COMMENT 'model_comment'\n"
++ " WITH (\n"
++ " 'key1'='value1',\n"
++ " 'key2'='value2'\n"
+
Re: [PR] [FLINK-34993] parser changes to support model CRUD sql [flink]
flinkbot commented on PR #24769: URL: https://github.com/apache/flink/pull/24769#issuecomment-2104149510 ## CI report: * add5f94934ccdbc9d2ad7e8624b00d27b61fb6a4 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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
Re: [PR] [FLINK-34993] parser changes to support model CRUD sql [flink]
lihaosky commented on PR #24769: URL: https://github.com/apache/flink/pull/24769#issuecomment-2104148834 cc @twalthr , @wuchong -- 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
