You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/05/24 03:08:45 UTC
[GitHub] [calcite] xy2953396112 opened a new pull request #1988: [CALCITE-4019] Fix visit SqlInsert with SqlShuttle cause NullPointerE…
xy2953396112 opened a new pull request #1988:
URL: https://github.com/apache/calcite/pull/1988
…xception
https://issues.apache.org/jira/browse/CALCITE-4019
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1988: [CALCITE-4019] Fix visit SqlInsert with SqlShuttle cause NullPointerE…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r429699722
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -261,6 +261,32 @@ public static boolean isLiteralChain(SqlNode node) {
}
}
+ /**
+ * Unparses a call to an operator which has special syntax.
+ *
+ * @param operator The operator
+ * @param writer Writer
+ * @param call List of 0 or more operands
+ */
+ public static void unparseSpecialSyntax(SqlOperator operator,
Review comment:
This is just for insert, better change the function name
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1988: [CALCITE-4019] Visit SqlInsert with SqlShuttle cause NullPointerException
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r430830028
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlInsert.java
##########
@@ -62,7 +62,11 @@ public SqlOperator getOperator() {
}
public List<SqlNode> getOperandList() {
- return ImmutableNullableList.of(keywords, targetTable, source, columnList);
+ if (columnList == null) {
Review comment:
Is it possible that `targetTable` is `null` or `source` is `null`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #1988: [CALCITE-4019] Visit SqlInsert with SqlShuttle cause NullPointerException
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r430880649
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlInsert.java
##########
@@ -62,7 +62,11 @@ public SqlOperator getOperator() {
}
public List<SqlNode> getOperandList() {
- return ImmutableNullableList.of(keywords, targetTable, source, columnList);
+ if (columnList == null) {
Review comment:
@DonnyZone Thansk for review. targetTable and source should be not null.
For example a sql such as `insert into emps` , it can’t be parsed.
`insert into emps VALUES (true, 1)` be parsed as SqlInsert, source not null.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1988: [CALCITE-4019] Fix visit SqlInsert with SqlShuttle cause NullPointerE…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r429699722
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -261,6 +261,32 @@ public static boolean isLiteralChain(SqlNode node) {
}
}
+ /**
+ * Unparses a call to an operator which has special syntax.
+ *
+ * @param operator The operator
+ * @param writer Writer
+ * @param call List of 0 or more operands
+ */
+ public static void unparseSpecialSyntax(SqlOperator operator,
Review comment:
This is just for insert, how about change the function name to `unparseSqlInsert`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1988: [CALCITE-4019] Visit SqlInsert with SqlShuttle cause NullPointerException
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r430893526
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlInsert.java
##########
@@ -62,7 +62,11 @@ public SqlOperator getOperator() {
}
public List<SqlNode> getOperandList() {
- return ImmutableNullableList.of(keywords, targetTable, source, columnList);
+ if (columnList == null) {
Review comment:
Thanks for clarification.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1988: [CALCITE-4019] Fix visit SqlInsert with SqlShuttle cause NullPointerE…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r429699722
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -261,6 +261,32 @@ public static boolean isLiteralChain(SqlNode node) {
}
}
+ /**
+ * Unparses a call to an operator which has special syntax.
+ *
+ * @param operator The operator
+ * @param writer Writer
+ * @param call List of 0 or more operands
+ */
+ public static void unparseSpecialSyntax(SqlOperator operator,
Review comment:
This is just for insert, how about change the function name to `unparseSqlInsert`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on pull request #1988: [CALCITE-4019] Change SqlInsert getOperandList method
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#issuecomment-633775868
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1988: [CALCITE-4019] Fix visit SqlInsert with SqlShuttle cause NullPointerE…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r429701209
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -261,6 +261,32 @@ public static boolean isLiteralChain(SqlNode node) {
}
}
+ /**
+ * Unparses a call to an operator which has special syntax.
+ *
+ * @param operator The operator
+ * @param writer Writer
+ * @param call List of 0 or more operands
+ */
+ public static void unparseSpecialSyntax(SqlOperator operator,
+ SqlWriter writer,
+ SqlCall call) {
+ final List<SqlNode> operands = call.getOperandList();
+ switch (operator.getKind()) {
+ case INSERT:
+ assert operands.size() == 3 || operands.size() == 4;
+ final SqlInsert insert = new SqlInsert(call.getParserPosition(),
+ (SqlNodeList) operands.get(0), operands.get(1), operands.get(2),
+ operands.size() == 3 ? null : (SqlNodeList) call.getOperandList().get(3));
+ insert.unparse(writer, operator.getLeftPrec(), operator.getRightPrec());
Review comment:
why create a new SqlInsert instance, can we just make a cast from call object?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on pull request #1988: [CALCITE-4019] Fix visit SqlInsert with SqlShuttle cause NullPointerE…
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#issuecomment-633337722
Commonly, we do not use `fix` or `bug fix` in commit log
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn commented on a change in pull request #1988: [CALCITE-4019] Change SqlInsert getOperandList method
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r430126757
##########
File path: core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java
##########
@@ -5747,6 +5748,17 @@ public void subTestIntervalMonthFailsValidation() {
.ok("INTERVAL '0' MONTH(0)");
}
+ @Test void testSqlParserPosPlus() throws Exception {
+ final String sql = "insert into emps select * from emps";
+ final SqlNode sqlNode = getSqlParser(sql).parseStmt();
+ sqlNode.accept(new SqlShuttle() {
+ @Override public SqlNode visit(SqlIdentifier identifier) {
+ return new SqlIdentifier(identifier.names,
+ identifier.getParserPosition());
+ }
+ });
+ }
Review comment:
how about add some checks for the result of accept
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] yanlin-Lynn merged pull request #1988: [CALCITE-4019] Visit SqlInsert with SqlShuttle cause NullPointerException
Posted by GitBox <gi...@apache.org>.
yanlin-Lynn merged pull request #1988:
URL: https://github.com/apache/calcite/pull/1988
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on a change in pull request #1988: [CALCITE-4019] Visit SqlInsert with SqlShuttle cause NullPointerException
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#discussion_r430128274
##########
File path: core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java
##########
@@ -5747,6 +5748,17 @@ public void subTestIntervalMonthFailsValidation() {
.ok("INTERVAL '0' MONTH(0)");
}
+ @Test void testSqlParserPosPlus() throws Exception {
+ final String sql = "insert into emps select * from emps";
+ final SqlNode sqlNode = getSqlParser(sql).parseStmt();
+ sqlNode.accept(new SqlShuttle() {
+ @Override public SqlNode visit(SqlIdentifier identifier) {
+ return new SqlIdentifier(identifier.names,
+ identifier.getParserPosition());
+ }
+ });
+ }
Review comment:
Thanks, add check 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:
users@infra.apache.org
[GitHub] [calcite] xy2953396112 commented on pull request #1988: [CALCITE-4019] Fix visit SqlInsert with SqlShuttle cause NullPointerE…
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #1988:
URL: https://github.com/apache/calcite/pull/1988#issuecomment-633356837
> Commonly, we do not use `fix` or `bug fix` in commit log
Thanks for review, 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:
users@infra.apache.org