[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334900882
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java
##
@@ -129,7 +133,7 @@ public boolean isIfNotExists() {
public void validate() throws SqlValidateException {
Set columnNames = new HashSet<>();
- if (columnList != null) {
+ if (columnList.size() > 0) {
Review comment:
Yes, they were.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334781062
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateView.java
##
@@ -54,9 +61,9 @@ public SqlCreateView(
boolean replace,
SqlCharStringLiteral comment) {
super(OPERATOR, pos, replace, false);
- this.viewName = viewName;
- this.fieldList = fieldList;
- this.query = query;
+ this.viewName = requireNonNull(viewName, "View name is
missing");
+ this.fieldList = requireNonNull(fieldList, "FieldList should
not be null");
+ this.query = requireNonNull(query, "Query is missing");
Review comment:
`FieldList` is not a valid noun or verb.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334780592
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java
##
@@ -76,11 +80,11 @@ public SqlCreateTable(
SqlCharStringLiteral comment) {
super(OPERATOR, pos, false, false);
this.tableName = requireNonNull(tableName, "Table name is
missing");
- this.columnList = requireNonNull(columnList, "Column list
should not be null");
- this.primaryKeyList = primaryKeyList;
- this.uniqueKeysList = uniqueKeysList;
- this.propertyList = propertyList;
- this.partitionKeyList = partitionKeyList;
+ this.columnList = requireNonNull(columnList, "ColumnList should
not be null");
+ this.primaryKeyList = requireNonNull(primaryKeyList,
"PrimayKeyList should not be null");
Review comment:
`ColumnList` is not a term or word or class name, it is somehow confusing
with the thrown message. Same as `PrimayKeyList` and the others.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module URL: https://github.com/apache/flink/pull/9843#discussion_r334774864 ## File path: flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java ## @@ -57,12 +60,15 @@ private final SqlNodeList propertyList; + @Nullable Review comment: No, can we make the `primaryKeyList` and `uniqueKeysList` default as `SqlNodeList.EMPTY` instead of null ? So that in `SqlCreateTable`, we can mark them as 'requireNotNull'. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334341019
##
File path:
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java
##
@@ -687,6 +687,18 @@ public void testCastAsRowType() {
"CAST(`A` AS ROW(`F0` VARCHAR NOT NULL, `F1` TIMESTAMP)
MULTISET)");
}
+ @Test
+ public void testValidateSqlCreateTable() {
+ String sql = "CREATE TABLE tbl1";
Review comment:
`SqlCreateTable` is a sql node name, we should avoid to use such name and
use the sql term instead.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module URL: https://github.com/apache/flink/pull/9843#discussion_r334332274 ## File path: flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java ## @@ -57,12 +60,15 @@ private final SqlNodeList propertyList; + @Nullable Review comment: Currently we did support create empty columns table with our parser, i have no strong objections to add test cases for 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module URL: https://github.com/apache/flink/pull/9843#discussion_r334319990 ## File path: flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java ## @@ -57,12 +60,15 @@ private final SqlNodeList propertyList; + @Nullable Review comment: Well, exactly, the `TableCreationContext` only was initialized when we encounter the `(`. I think we can give a `SqlNodeList.EMPTY` as default value for `primaryKeyList` and `uniqueKeysList` instead of null. BTW, what is the use case for table without column definitions ? Can you explain a little more ? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334311449
##
File path:
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java
##
@@ -687,6 +687,18 @@ public void testCastAsRowType() {
"CAST(`A` AS ROW(`F0` VARCHAR NOT NULL, `F1` TIMESTAMP)
MULTISET)");
}
+ @Test
+ public void testValidateSqlCreateTable() {
+ String sql = "CREATE TABLE tbl1";
Review comment:
testValidateCreateTableWithNakedTableName
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334311321
##
File path:
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkDDLValidationTest.java
##
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.sql.parser;
+
+import org.apache.flink.sql.parser.ddl.SqlCreateTable;
+import org.apache.flink.sql.parser.ddl.SqlCreateView;
+import org.apache.flink.sql.parser.error.SqlValidateException;
+import org.apache.flink.sql.parser.impl.FlinkSqlParserImpl;
+
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.avatica.util.Quoting;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.sql.parser.SqlParserImplFactory;
+import org.apache.calcite.util.SourceStringReader;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * Validation tests for ddl fields.
+ */
+public class FlinkDDLValidationTest {
+
+ protected SqlParserImplFactory parserImplFactory() {
+ return FlinkSqlParserImpl.FACTORY;
+ }
+
+ private SqlParser getSqlParser(String sql) {
+ SourceStringReader source = new SourceStringReader(sql);
+ return SqlParser.create(source,
+ SqlParser.configBuilder()
Review comment:
Yes, it is, we may add an exception assertion when we passed in the error
message explicitly.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module URL: https://github.com/apache/flink/pull/9843#discussion_r334311147 ## File path: flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java ## @@ -57,12 +60,15 @@ private final SqlNodeList propertyList; + @Nullable Review comment: `primaryKeyList` default is an empty list[1], also is `uniqueKeysList`[2], i agree the code is not that intuitive, i would make some refactoring if i have time. [1] https://github.com/apache/flink/blob/dbe1bfa31db4a561b6faa9c1235f02dc130825ca/flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl#L100 [2]https://github.com/apache/flink/blob/dbe1bfa31db4a561b6faa9c1235f02dc130825ca/flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java#L316 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module URL: https://github.com/apache/flink/pull/9843#discussion_r334222831 ## File path: flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateTable.java ## @@ -57,12 +60,15 @@ private final SqlNodeList propertyList; + @Nullable Review comment: In current parser, `primaryKeyList` and `uniqueKeysList ` would never be null, they could be empty if there are no definitions. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334222975
##
File path:
flink-table/flink-table-planner-blink/src/main/java/org/apache/flink/table/planner/operations/SqlToOperationConverter.java
##
@@ -107,29 +106,21 @@ private Operation convertCreateTable(SqlCreateTable
sqlCreateTable) {
}
// set with properties
- SqlNodeList propertyList = sqlCreateTable.getPropertyList();
Map properties = new HashMap<>();
- if (propertyList != null) {
- propertyList.getList().forEach(p ->
- properties.put(((SqlTableOption)
p).getKeyString().toLowerCase(),
- ((SqlTableOption) p).getValueString()));
- }
+ sqlCreateTable.getPropertyList().getList().forEach(p ->
+ properties.put(((SqlTableOption)
p).getKeyString().toLowerCase(),
+ ((SqlTableOption) p).getValueString()));
TableSchema tableSchema = createTableSchema(sqlCreateTable);
- String tableComment = "";
- if (sqlCreateTable.getComment() != null) {
- tableComment =
sqlCreateTable.getComment().getNlsString().getValue();
- }
+ String tableComment = sqlCreateTable.getComment().map(comment ->
+ comment.getNlsString().getValue()).orElse("");
Review comment:
Well, i think we should keep it null as the default value.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table SQL] Use Optional for optional parameters in parser module
danny0405 commented on a change in pull request #9843: [FLINK-14296] [Table
SQL] Use Optional for optional parameters in parser module
URL: https://github.com/apache/flink/pull/9843#discussion_r334222935
##
File path:
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkDDLValidationTest.java
##
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.sql.parser;
+
+import org.apache.flink.sql.parser.ddl.SqlCreateTable;
+import org.apache.flink.sql.parser.ddl.SqlCreateView;
+import org.apache.flink.sql.parser.error.SqlValidateException;
+import org.apache.flink.sql.parser.impl.FlinkSqlParserImpl;
+
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.avatica.util.Quoting;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.sql.parser.SqlParserImplFactory;
+import org.apache.calcite.util.SourceStringReader;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * Validation tests for ddl fields.
+ */
+public class FlinkDDLValidationTest {
+
+ protected SqlParserImplFactory parserImplFactory() {
+ return FlinkSqlParserImpl.FACTORY;
+ }
+
+ private SqlParser getSqlParser(String sql) {
+ SourceStringReader source = new SourceStringReader(sql);
+ return SqlParser.create(source,
+ SqlParser.configBuilder()
Review comment:
In `FlinkSqlParserImplTest` there is a `ValidationMatcher` which can be used
to validate failed cases. The case you gave are all positive tests, i think
with code `sql(sqlStr).node(new ValidationMatcher());` you can trigger the
validation, so you can move these test cases into `FlinkSqlParserImplTest`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
