[FLINK-34993] parser changes to support model CRUD sql #24769
Conversation
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-checkstyle-plugin</artifactId> | ||
|
|
||
| <configuration> |
There was a problem hiding this comment.
Should not be required. See comment below.
| @@ -2412,6 +2440,8 @@ SqlDrop SqlDropExtended(Span s, boolean replace) : | |||
| drop = SqlDropDatabase(s, replace) | |||
| | | |||
| drop = SqlDropFunction(s, replace, isTemporary) | |||
| | | |||
| drop = SqlDropModel(s) /** temporary model is not supported **/ | |||
There was a problem hiding this comment.
Why don't we add this as part of this PR? Should be straight forward to add.
| } | ||
|
|
||
| /** | ||
| * CREATE MODEL [IF NOT EXIST] modelName |
There was a problem hiding this comment.
add temporary as part of this PR, should be easily copyable from CREATE TABLE?
| } | ||
| | | ||
| <SET> | ||
| modelOptionList = TableProperties() |
There was a problem hiding this 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.
|
|
||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** {@link SqlNode} to describe the CREATE MODEL syntax. */ |
There was a problem hiding this comment.
also give a full syntax example in the JavaDoc here similar to SqlAlterModel
| * <pre>{@code | ||
| * CREATE MODEL my_model WITH ( | ||
| * ‘task’ = ‘regression’, | ||
| * ‘algorithm’: ‘boosted_tree’, |
There was a problem hiding this comment.
colon is also not supported. A similar to SqlAlterModel would be more appropriate here.
|
|
||
| /** | ||
| * DESCRIBE MODEL [EXTENDED] [[catalogName.] dataBasesName].sqlIdentifier sql call. Here we add Rich | ||
| * in className to follow the convention of SqlDescribeTable, which only had it to distinguish from |
There was a problem hiding this comment.
use {@link when referring to a class like SqlDescribeTable this help the reader to quickly navigate the code base
| + " 'key2'='value2'\n" | ||
| + " ) as select f1, f2 from t1\n") | ||
| .ok( | ||
| "CREATE MODEL IF NOT EXISTS `M1` WITH (\n" |
There was a problem hiding this comment.
Also test the error behavior when a user adds explicit columns to input and output using CREATE MODEL AS
| "-//Puppy Crawl//DTD Suppressions 1.1//EN" | ||
| "http://www.puppycrawl.com/dtds/suppressions_1_1.dtd"> | ||
|
|
||
| <suppressions> |
There was a problem hiding this comment.
Why do we need a dedicated file? Shouldn't adding this to the regular Flink suppressions be enough?
| @@ -147,6 +148,7 @@ class FlinkPlannerImpl( | |||
| || sqlNode.isInstanceOf[SqlShowProcedures] | |||
| || sqlNode.isInstanceOf[SqlShowJobs] | |||
| || sqlNode.isInstanceOf[SqlDescribeJob] | |||
| || sqlNode.isInstanceOf[SqlRichDescribeModel] | |||
There was a problem hiding this comment.
What is the current error behavior when people start using the syntax without an implementation for it? Does Flink generate a helpful message? Like "Unsupported operatin: CREATE MODEL AS"? It's unlikely that we will provide implementation for all keywords until feature freeze in 3 weeks.
What is the purpose of the change
Support model CRUD sql syntax
Brief change log
Add following syntax for model
create modeldrop modelshow modelshow create modelalter modelVerifying this change
Unit test for parser
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation