[GitHub] [flink] danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r328414544 ## File path: flink-table/flink-table-planner-blink/src/test/resources/org/apache/flink/table/planner/plan/rules/logical/FlinkFilterJoinRuleTest.xml ## @@ -22,6 +22,7 @@ limitations under the License. SELECT * FROM (SELECT * FROM leftT WHERE a NOT IN (SELECT c FROM rightT WHERE c < 3)) T WHERE T.b > 2 ]]> + Review comment: Sure, thanks. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r328414311 ## File path: flink-table/flink-table-planner-blink/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/agg/DistinctAggregateTest.xml ## @@ -48,21 +48,23 @@ SortAggregate(isMerge=[true], select=[Final_COUNT(count$0) AS EXPR$0, Final_SUM(
[GitHub] [flink] danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r328414214
##
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/codegen/calls/ScalarOperatorGens.scala
##
@@ -390,6 +390,16 @@ object ScalarOperatorGens {
}
}
+ def generateIsNotDistinctFrom(
+ ctx: CodeGeneratorContext,
+ left: GeneratedExpression,
+ right: GeneratedExpression)
+ : GeneratedExpression = {
Review comment:
Yep, thanks.
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r328414087 ## File path: flink-table/flink-sql-parser/src/main/codegen/data/Parser.tdd ## @@ -394,23 +389,13 @@ ] # List of methods for parsing ddl supported data types. - # Return type of method implementation should be "SqlIdentifier". - # Example: SqlParseTimeStampZ(). - flinkDataTypeParserMethods: [ -"SqlArrayType()", -"SqlMultisetType()", -"SqlMapType()", -"SqlRowType()", -"SqlStringType()", -"SqlBytesType()", -"SqlTimestampType()", -"SqlTimeType()" - ] - - # List of methods for parsing custom data types. - # Return type of method implementation should be "SqlIdentifier". + # Return type of method implementation should be "SqlTypeNameSpec". # Example: SqlParseTimeStampZ(). dataTypeParserMethods: [ +"ExtendedSqlBasicTypeName()", +"CustomizedCollectionsTypeName()", Review comment: We already have a `ExtendedSqlCollectionsTypeName` parse block name, i name it `CustomizedXXX` because the syntax is totally customized and does not belong to standard SQL. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r328413899
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/calcite/sql/ExtendedSqlRowTypeNameSpec.java
##
@@ -0,0 +1,150 @@
+/*
+ * 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.calcite.sql;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.util.Litmus;
+import org.apache.calcite.util.Pair;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * A sql type name specification of ROW type.
+ *
+ * The difference with {@link SqlRowTypeNameSpec}:
+ *
+ * Support comment syntax for every field
+ * Field data type default is nullable
+ * Support ROW type with empty fields, e.g. ROW()
+ *
+ */
+public class ExtendedSqlRowTypeNameSpec extends SqlTypeNameSpec {
Review comment:
`SqlRowTypeNameSpec` has assertion that the row fields be must more than 1,
so i can not inherit from it, because we allow empty row: `ROW()`.
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r328413261 ## File path: flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl ## @@ -81,6 +66,33 @@ void TableColumn2(List list) : } } +/** +* Different with #DataType, we support a [ NULL | NOT NULL ] suffix syntax for both the Review comment: No, for `#DataType`, i mean the parser block name, maybe `#DataType()` is better. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r327917799
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/type/ExtendedSqlCollectionTypeNameSpec.java
##
@@ -0,0 +1,129 @@
+/*
+ * 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.type;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlCollectionTypeNameSpec;
+import org.apache.calcite.sql.SqlTypeNameSpec;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.util.Litmus;
+import org.apache.calcite.util.Util;
+
+/**
+ * A extended sql type name specification of collection type,
+ * different with {@link SqlCollectionTypeNameSpec},
+ * we support NULL or NOT NULL suffix for the element type name(this syntax
+ * does not belong to standard SQL).
+ */
+public class ExtendedSqlCollectionTypeNameSpec extends
SqlCollectionTypeNameSpec {
Review comment:
Sure, i have added a package-info file and address this info.
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r327917280 ## File path: flink-table/flink-table-planner-blink/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/join/ShuffledHashJoinTest.xml ## @@ -19,57 +19,60 @@ limitations under the License. + + + Review comment: Sure, thanks. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r327917230 ## File path: flink-table/flink-table-planner-blink/src/test/resources/org/apache/flink/table/planner/plan/rules/logical/FlinkFilterJoinRuleTest.xml ## @@ -1,435 +0,0 @@ - - - Review comment: Add them back. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r327917159
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/type/ExtendedSqlBuiltinTypeNameSpec.java
##
@@ -0,0 +1,65 @@
+/*
+ * 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.type;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlUserDefinedTypeNameSpec;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+
+import java.util.Locale;
+
+/**
+ * A sql type name specification of extended builtin data type, this is
different with
+ * the normal user defined data type, because there is no need to register
them into
+ * the catalog when deriving the data type.
+ */
+public class ExtendedSqlBuiltinTypeNameSpec extends SqlUserDefinedTypeNameSpec
{
Review comment:
I agree, have renamed `ExtendedSqlBuiltinTypeNameSpec` to
`ExtendedSqlBasicTypeNameSpec` and make it extend `SqlBasicTypeNameSpec`.
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r327908022
##
File path:
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl
##
@@ -375,210 +387,83 @@ SqlDrop SqlDropView(Span s, boolean replace) :
}
}
-SqlIdentifier FlinkCollectionsTypeName() :
-{
-}
-{
-LOOKAHEAD(2)
- {
-return new SqlIdentifier(SqlTypeName.MULTISET.name(), getPos());
-}
-|
- {
-return new SqlIdentifier(SqlTypeName.ARRAY.name(), getPos());
-}
-}
-
-SqlIdentifier FlinkTypeName() :
+SqlTypeNameSpec SqlUserDefinedTypeName() :
{
-final SqlTypeName sqlTypeName;
-final SqlIdentifier typeName;
-final Span s = Span.of();
+final String typeName;
}
{
(
-<#-- additional types are included here -->
-<#-- make custom data types in front of Calcite core data types -->
-<#list parser.flinkDataTypeParserMethods as method>
-<#if (method?index > 0)>
-|
-
-LOOKAHEAD(2)
-typeName = ${method}
-
+ { typeName = token.image; }
|
-LOOKAHEAD(2)
-sqlTypeName = SqlTypeName(s) {
-typeName = new SqlIdentifier(sqlTypeName.name(), s.end(this));
-}
-|
-LOOKAHEAD(2)
-typeName = FlinkCollectionsTypeName()
-|
-typeName = CompoundIdentifier() {
-throw new ParseException("UDT in DDL is not supported yet.");
-}
+ { typeName = token.image; }
)
{
-return typeName;
+return new ExtendedSqlBuiltinTypeNameSpec(token.image, getPos());
}
}
-/**
-* Parse a Flink data type with nullable options, NULL -> nullable, NOT NULL ->
not nullable.
-* Default to be nullable.
+/*
+* Parses collection type name that does not belong to standard SQL, i.e.
ARRAY.
*/
-SqlDataTypeSpec FlinkDataType() :
+SqlTypeNameSpec ExtendedCollectionsTypeName() :
Review comment:
Thanks, i have renamed `ExtendedCollectionsTypeName2` to
`CustomizedCollectionsTypeName`.
The `ExtendedCollectionsTypeName` supports NULL/NOT NULL options compared to
Calcite core `CollectionsTypeName`, i.e. "INT NOT NULL ARRAY", i have addressed
this in the comments.
The `CustomizedCollectionsTypeName` is totally customized by Flink, whose
syntax does not belong to standard SQL, i.e. "ARRAY"
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r327908022
##
File path:
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl
##
@@ -375,210 +387,83 @@ SqlDrop SqlDropView(Span s, boolean replace) :
}
}
-SqlIdentifier FlinkCollectionsTypeName() :
-{
-}
-{
-LOOKAHEAD(2)
- {
-return new SqlIdentifier(SqlTypeName.MULTISET.name(), getPos());
-}
-|
- {
-return new SqlIdentifier(SqlTypeName.ARRAY.name(), getPos());
-}
-}
-
-SqlIdentifier FlinkTypeName() :
+SqlTypeNameSpec SqlUserDefinedTypeName() :
{
-final SqlTypeName sqlTypeName;
-final SqlIdentifier typeName;
-final Span s = Span.of();
+final String typeName;
}
{
(
-<#-- additional types are included here -->
-<#-- make custom data types in front of Calcite core data types -->
-<#list parser.flinkDataTypeParserMethods as method>
-<#if (method?index > 0)>
-|
-
-LOOKAHEAD(2)
-typeName = ${method}
-
+ { typeName = token.image; }
|
-LOOKAHEAD(2)
-sqlTypeName = SqlTypeName(s) {
-typeName = new SqlIdentifier(sqlTypeName.name(), s.end(this));
-}
-|
-LOOKAHEAD(2)
-typeName = FlinkCollectionsTypeName()
-|
-typeName = CompoundIdentifier() {
-throw new ParseException("UDT in DDL is not supported yet.");
-}
+ { typeName = token.image; }
)
{
-return typeName;
+return new ExtendedSqlBuiltinTypeNameSpec(token.image, getPos());
}
}
-/**
-* Parse a Flink data type with nullable options, NULL -> nullable, NOT NULL ->
not nullable.
-* Default to be nullable.
+/*
+* Parses collection type name that does not belong to standard SQL, i.e.
ARRAY.
*/
-SqlDataTypeSpec FlinkDataType() :
+SqlTypeNameSpec ExtendedCollectionsTypeName() :
Review comment:
Thanks, i have renamed `ExtendedCollectionsTypeName2` to
`CustomizedCollectionsTypeName`.
The `ExtendedCollectionsTypeName` supports NULL/NOT NULL options compared to
Calcite core `CollectionsTypeName`, i.e. "INT NOT NULL ARRAY", i have addressed
this in the comments.
The `CustomizedCollectionsTypeName` is totally customized by Flink, whose
syntax does not belong to standard SQL, i.e. "ARRAY
[GitHub] [flink] danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r327910279 ## File path: flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java ## @@ -290,6 +292,12 @@ private final SqlValidatorImpl.ValidationErrorFunction validationErrorFunction = new SqlValidatorImpl.ValidationErrorFunction(); + // TypeCoercion instance used for implicit type coercion. + private TypeCoercion typeCoercion; Review comment: Yes, this instance is default there, it does not bring in any overhead. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r327910106 ## File path: flink-table/flink-table-planner-blink/src/test/resources/org/apache/flink/table/planner/plan/rules/logical/FlinkFilterJoinRuleTest.xml ## @@ -1,435 +0,0 @@ - - - Review comment: There are already some tests in Calcite, maybe we should add tests only for new added Flink rules. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r327909838
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/type/ExtendedSqlCollectionTypeNameSpec.java
##
@@ -0,0 +1,129 @@
+/*
+ * 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.type;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlCollectionTypeNameSpec;
+import org.apache.calcite.sql.SqlTypeNameSpec;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.util.Litmus;
+import org.apache.calcite.util.Util;
+
+/**
+ * A extended sql type name specification of collection type,
+ * different with {@link SqlCollectionTypeNameSpec},
+ * we support NULL or NOT NULL suffix for the element type name(this syntax
+ * does not belong to standard SQL).
+ */
+public class ExtendedSqlCollectionTypeNameSpec extends
SqlCollectionTypeNameSpec {
Review comment:
Because the constructor of `SqlTypeNameSpec` constructor is package scope,
previously i think Calcite's `SqlTypeNameSpec` is enough for Flink extension,
but it's not, i have fixed this in CALCITE-3360, we can move them into
`org.apache.flink.sql.parser.type` for the next Calcite version.
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r327909362
##
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/type/ExtendedSqlBuiltinTypeNameSpec.java
##
@@ -0,0 +1,65 @@
+/*
+ * 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.type;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlUserDefinedTypeNameSpec;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.sql.validate.SqlValidator;
+
+import java.util.Locale;
+
+/**
+ * A sql type name specification of extended builtin data type, this is
different with
+ * the normal user defined data type, because there is no need to register
them into
+ * the catalog when deriving the data type.
+ */
+public class ExtendedSqlBuiltinTypeNameSpec extends SqlUserDefinedTypeNameSpec
{
Review comment:
We need this class to customize the unparse the logic, `STRING` is always
unparsed to `STRING` not `VARCHAR(INT_MAX)`.
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r327908141 ## File path: flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl ## @@ -644,26 +532,35 @@ void FieldNameTypeCommaList( } /** -* Parse Row type, we support both Row(name1 type1, name2 type2) and Row. -* Every item type can have suffix of `NULL` or `NOT NULL` to indicate if this type is nullable. -* i.e. Row(f0 int not null, f1 varchar null). +* Parse Row type, we support both Row(name1 type1, name2 type2) +* and Row. +* Every item type can have a suffix of `NULL` or `NOT NULL` to indicate if this type is nullable. +* i.e. Row(f0 int not null, f1 varchar null). Default is nullable. */ -SqlIdentifier SqlRowType() : +SqlTypeNameSpec ExtendedSqlRowTypeName() : Review comment: Thanks, i have added the comments. 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656]
[sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to
1.21.0
URL: https://github.com/apache/flink/pull/9712#discussion_r327908022
##
File path:
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl
##
@@ -375,210 +387,83 @@ SqlDrop SqlDropView(Span s, boolean replace) :
}
}
-SqlIdentifier FlinkCollectionsTypeName() :
-{
-}
-{
-LOOKAHEAD(2)
- {
-return new SqlIdentifier(SqlTypeName.MULTISET.name(), getPos());
-}
-|
- {
-return new SqlIdentifier(SqlTypeName.ARRAY.name(), getPos());
-}
-}
-
-SqlIdentifier FlinkTypeName() :
+SqlTypeNameSpec SqlUserDefinedTypeName() :
{
-final SqlTypeName sqlTypeName;
-final SqlIdentifier typeName;
-final Span s = Span.of();
+final String typeName;
}
{
(
-<#-- additional types are included here -->
-<#-- make custom data types in front of Calcite core data types -->
-<#list parser.flinkDataTypeParserMethods as method>
-<#if (method?index > 0)>
-|
-
-LOOKAHEAD(2)
-typeName = ${method}
-
+ { typeName = token.image; }
|
-LOOKAHEAD(2)
-sqlTypeName = SqlTypeName(s) {
-typeName = new SqlIdentifier(sqlTypeName.name(), s.end(this));
-}
-|
-LOOKAHEAD(2)
-typeName = FlinkCollectionsTypeName()
-|
-typeName = CompoundIdentifier() {
-throw new ParseException("UDT in DDL is not supported yet.");
-}
+ { typeName = token.image; }
)
{
-return typeName;
+return new ExtendedSqlBuiltinTypeNameSpec(token.image, getPos());
}
}
-/**
-* Parse a Flink data type with nullable options, NULL -> nullable, NOT NULL ->
not nullable.
-* Default to be nullable.
+/*
+* Parses collection type name that does not belong to standard SQL, i.e.
ARRAY.
*/
-SqlDataTypeSpec FlinkDataType() :
+SqlTypeNameSpec ExtendedCollectionsTypeName() :
Review comment:
Thanks, i have renamed `ExtendedCollectionsTypeName2` to
`CustomizedCollectionsTypeName`.
The `ExtendedCollectionsTypeName` supports NULL/NOT NULL options compared to
Calcite core `CollectionsTypeName`, i.e. "INT NOT NULL ARRAY", i have addressed
this in the comments.
The `CustomizedCollectionsTypeName` is totally customized by Flink, whose
syntax does not belong to standard SQL, i.e. "ARRAY"
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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r327905909 ## File path: flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl ## @@ -81,6 +66,33 @@ void TableColumn2(List list) : } } +/** +* Different with #DataType, we support a [ NULL | NOT NULL ] suffix syntax for both the Review comment: Sure, let me update 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 #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0
danny0405 commented on a change in pull request #9712: [FLINK-13656] [sql-parser][table-planner][table-planner-blink] Bump Calcite dependency to 1.21.0 URL: https://github.com/apache/flink/pull/9712#discussion_r327905534 ## File path: flink-table/flink-sql-parser/src/main/codegen/data/Parser.tdd ## @@ -439,6 +424,14 @@ "SqlDropView" ] + # Binary operators tokens Review comment: This is extended from the core parser which is used for customizing the binary operator tokens, for example, in PostgreSQL, we can have a `::` token to implies `CAST` operator: `a :: INT` is equals to `CAST(a as INT)`. For Flink, we does not need this now, the parser just complains if we does not have such section. 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
