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/06/13 21:29:06 UTC
[GitHub] [calcite] amaliujia opened a new pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
amaliujia opened a new pull request #2025:
URL: https://github.com/apache/calcite/pull/2025
…NNEST (Rui Wang).
Before this change, Calcite parser produces "(UNEST(...))". However, Calcite parser fails to parse UNNEST as parenthesized table expressions: "JOIN (SELECT * FROM ^(UNNEST(...))^)". This change stops produces parenthesized table expressions for UNNEST to fix this problem.
----------------------------------------------------------------
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] danny0405 commented on a change in pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2025:
URL: https://github.com/apache/calcite/pull/2025#discussion_r439913573
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlCall.java
##########
@@ -97,7 +97,13 @@ public void unparse(
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
|| writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
- final SqlWriter.Frame frame = writer.startList("(", ")");
+ final SqlWriter.Frame frame;
+ if (operator instanceof SqlUnnestOperator) {
+ frame = writer.startList(SqlWriter.FrameTypeEnum.SIMPLE);
+ } else {
Review comment:
Use `SqlKind.UNNEST` instead of `instanceOf`.
----------------------------------------------------------------
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] XuQianJin-Stars commented on pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
Posted by GitBox <gi...@apache.org>.
XuQianJin-Stars commented on pull request #2025:
URL: https://github.com/apache/calcite/pull/2025#issuecomment-643721711
+1
----------------------------------------------------------------
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] amaliujia commented on pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2025:
URL: https://github.com/apache/calcite/pull/2025#issuecomment-652069459
@danny0405 is it now the right time to merge this PR?
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2025:
URL: https://github.com/apache/calcite/pull/2025#discussion_r439931545
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlCall.java
##########
@@ -97,7 +97,13 @@ public void unparse(
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
|| writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
- final SqlWriter.Frame frame = writer.startList("(", ")");
+ final SqlWriter.Frame frame;
+ if (operator instanceof SqlUnnestOperator) {
+ frame = writer.startList(SqlWriter.FrameTypeEnum.SIMPLE);
+ } else {
Review comment:
thanks Danny. I changed to another approach now (mark UNENST as not a expression).
----------------------------------------------------------------
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] danny0405 closed pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
Posted by GitBox <gi...@apache.org>.
danny0405 closed pull request #2025:
URL: https://github.com/apache/calcite/pull/2025
----------------------------------------------------------------
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] bozzzzo commented on pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
Posted by GitBox <gi...@apache.org>.
bozzzzo commented on pull request #2025:
URL: https://github.com/apache/calcite/pull/2025#issuecomment-643825132
Thank you for having a look at this!
[Here](https://github.com/bozzzzo/calcite/commit/cd966361f23c41d5f987a2b7ab85b60a1731df10) is a patch for the `unparse` tests that shows the `UNNEST` is now indeed behaving correctly.
The above shows that `MATCH_RECOGNIZE` tests fail in a suspiciously similar way due to extra parentheses (if I read the exception correctly, I'm not familiar with `MATCH_RECOGNIZE` syntax)
What I found is that there is a class `SqlFunctionCategory` that already marks `MATCH_RECOGNIZE` as a `TABLE_FUNCTION`. Could a better fix be made by adding `UNNEST` to the `SqlFunctionCategory` and then one could check the `isTableFunction()` instead of hardcoding a check for `SqlUnnestOperator`. There is a missing link in my approach that `SqlUnnestOperator` does not derive or implement anything that would expose the `SqlFunctionCategory` which seems a pity.
----------------------------------------------------------------
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] amaliujia commented on pull request #2025: [CALCITE-4033] Does not produce parenthesized table expressions for U…
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2025:
URL: https://github.com/apache/calcite/pull/2025#issuecomment-643904684
@bozzzzo I am using similar idea now in this PR (not hardcode UNNEST check anymore).
----------------------------------------------------------------
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