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 2022/10/14 18:23:06 UTC
[GitHub] [calcite] l4wei opened a new pull request, #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
l4wei opened a new pull request, #2938:
URL: https://github.com/apache/calcite/pull/2938
Remove unnecessary parentheses around SELECT in WITH body
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996592477
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
|| (frame.frameType == FrameTypeEnum.SETOP);
}
+ @Override public boolean inWithBody() {
+ return frame != null && frame.frameType == FrameTypeEnum.WITH_BODY;
Review Comment:
Maybe not, NPE will be thrown on some unit tests, e.g. `SqlParserTest#testNewSpecification`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996593733
##########
core/src/main/java/org/apache/calcite/sql/SqlWith.java:
##########
@@ -103,8 +103,9 @@ private SqlWithOperator() {
}
writer.endList(frame1);
final SqlWriter.Frame frame2 =
- writer.startList(SqlWriter.FrameTypeEnum.SIMPLE);
- with.body.unparse(writer, 100, 100);
+ writer.startList(SqlWriter.FrameTypeEnum.WITH_BODY);
+ // leftPrec: 2, rightPrec: 3 are the low bound to WITH operator
+ with.body.unparse(writer, 2, 3);
Review Comment:
You are right, i agree.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996593539
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -2241,6 +2241,34 @@ void checkPeriodPredicate(Checker checker) {
sql(sql).fails("(?s)Encountered \"with\" at .*");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithSelect() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithOrderBy() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2 order by deptno\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`\n"
+ + "ORDER BY `DEPTNO`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
Review Comment:
I just want to show these two cases should be related to this issue, maybe i should not change past cases? if so, i will change 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996591355
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
`SqlWriter#inQuery` 's java doc says: "whether we are currently in a query context", not "whether we are currently in a sub-query context", so I think current java doc is appropriate?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r1002851565
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
Actually, I think I have not extended the meaning of `SqlWriter#inQuery`. Current java doc: "Returns whether we are currently in a query context" is still appropriate now. Before this PR, `inQuery` covers "in WITH", I think "WITH" operator should be split into two operators: WITH_ITEM and WITH_BODY, so I change "WITH" to "WITH_BODY", that's the only change on this method.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996443682
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
|| (frame.frameType == FrameTypeEnum.SETOP);
}
+ @Override public boolean inWithBody() {
+ return frame != null && frame.frameType == FrameTypeEnum.WITH_BODY;
Review Comment:
`frame != null` can be removed.
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
The `SqlWriter#inQuery`'s java doc should also be updated. IIUC, `SqlWriter#inQuery` is for deciding whether a SELECT is a sub-query. Hence the `inQuery` actually means that "a select is a top-most query". But now, the `inQuery`'s meaning has expanded, we may need to reflect it in the java doc, and may change it's name to a more proper one in the future.
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -2241,6 +2241,34 @@ void checkPeriodPredicate(Checker checker) {
sql(sql).fails("(?s)Encountered \"with\" at .*");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithSelect() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithOrderBy() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2 order by deptno\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`\n"
+ + "ORDER BY `DEPTNO`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
Review Comment:
Why the existing `testWithNestedInSubQuery`/`testWithUnion` is also a test case for CALCITE-5299
##########
core/src/main/java/org/apache/calcite/sql/SqlCall.java:
##########
@@ -118,7 +118,7 @@ public int operandCount() {
final SqlDialect dialect = writer.getDialect();
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
- || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
+ || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION) && !writer.inWithBody()) {
Review Comment:
Why must you add this condition? I know `testWithUnion` will fail without this, however, it's surprising that `SqlKind.SET_QUERY` is a `SqlKind.EXPRESSION` while `SELECT`/`ORDER_BY`/`WITH`/`JOIN` etc. are not.
##########
core/src/main/java/org/apache/calcite/sql/SqlWith.java:
##########
@@ -103,8 +103,9 @@ private SqlWithOperator() {
}
writer.endList(frame1);
final SqlWriter.Frame frame2 =
- writer.startList(SqlWriter.FrameTypeEnum.SIMPLE);
- with.body.unparse(writer, 100, 100);
+ writer.startList(SqlWriter.FrameTypeEnum.WITH_BODY);
+ // leftPrec: 2, rightPrec: 3 are the low bound to WITH operator
+ with.body.unparse(writer, 2, 3);
Review Comment:
Hard coded `2` and `3` can be replaced with `getLeftPrec()` and `getRightPrec()`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996599562
##########
core/src/main/java/org/apache/calcite/sql/SqlCall.java:
##########
@@ -118,7 +118,7 @@ public int operandCount() {
final SqlDialect dialect = writer.getDialect();
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
- || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
+ || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION) && !writer.inWithBody()) {
Review Comment:
Yes, it's very strange that `SqlKind.EXPRESSION` contains UNION, and according to declaration of `SqlKind.EXPRESSION`, it should contains `SELECT/ORDER_BY/WITH/JOIN` etc, and not contains `UNION`, but when i debug, something is different, you can not find `SELECT/ORDER_BY/WITH/JOIN` in `SqlKind.EXPRESSION`, and `UNION` will appear. Let's back to the topic, might putting `inWithBody()` into `SqlWriter#isAlwaysUseParentheses ` will be 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996676210
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
So what's your suggestion on `SqlWriter#inQuery`'s java doc?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996611975
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
|| (frame.frameType == FrameTypeEnum.SETOP);
}
+ @Override public boolean inWithBody() {
+ return frame != null && frame.frameType == FrameTypeEnum.WITH_BODY;
Review Comment:
Sorry about this one, I thought it was `frame.frameType != null`
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -2241,6 +2241,34 @@ void checkPeriodPredicate(Checker checker) {
sql(sql).fails("(?s)Encountered \"with\" at .*");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithSelect() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithOrderBy() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2 order by deptno\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`\n"
+ + "ORDER BY `DEPTNO`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
Review Comment:
IMHO, we'd better to not comment other test cases to this issue cause it may be added for other purpose.
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
Maybe I'm not clear on this. I'm not saying that is should be changed from "a query context" to "a sub-query context".
##########
core/src/main/java/org/apache/calcite/sql/SqlCall.java:
##########
@@ -118,7 +118,7 @@ public int operandCount() {
final SqlDialect dialect = writer.getDialect();
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
- || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
+ || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION) && !writer.inWithBody()) {
Review Comment:
`SqlKind.EXPRESSION` uses `EnumSet.complementOf`, hence its document and its result is consistent. The question is "should SET_QUERY be 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996675145
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -2241,6 +2241,34 @@ void checkPeriodPredicate(Checker checker) {
sql(sql).fails("(?s)Encountered \"with\" at .*");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithSelect() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
+ @Test void testWithOrderBy() {
+ final String sql = "with emp2 as (select * from emp)\n"
+ + "select * from emp2 order by deptno\n";
+ final String expected = "WITH `EMP2` AS (SELECT *\n"
+ + "FROM `EMP`) SELECT *\n"
+ + "FROM `EMP2`\n"
+ + "ORDER BY `DEPTNO`";
+ sql(sql).ok(expected);
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5299">[CALCITE-5299]
+ * JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body</a>. */
Review Comment:
OK, i will change them back on next commits.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao closed pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
libenchao closed pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
URL: https://github.com/apache/calcite/pull/2938
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on PR #2938:
URL: https://github.com/apache/calcite/pull/2938#issuecomment-1280237370
>
OH~, thank you very much for your tip, i have already changed the email in my local git config.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996677165
##########
core/src/main/java/org/apache/calcite/sql/SqlCall.java:
##########
@@ -118,7 +118,7 @@ public int operandCount() {
final SqlDialect dialect = writer.getDialect();
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
- || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
+ || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION) && !writer.inWithBody()) {
Review Comment:
IMHO, SET_QUERY should not be 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r1004026696
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -3749,26 +3774,26 @@ void checkPeriodPredicate(Checker checker) {
+ "select * from e except "
+ "select * from f union "
+ "select * from g";
- final String expected = "((((SELECT *\n"
+ final String expected = "SELECT *\n"
Review Comment:
`SqlParserTest#testUnionIntersect` has been added. The reason I have not added CALCITE-5299's java doc on this test case is I think this test case is a complement for past precedence testing instead of for current CALCITE-5299.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r1003027206
##########
core/src/main/java/org/apache/calcite/sql/SqlCall.java:
##########
@@ -118,7 +118,7 @@ public int operandCount() {
final SqlDialect dialect = writer.getDialect();
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
- || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
+ || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION) && !writer.inWithBody()) {
Review Comment:
You are right, exclude "SET_QUERY" from EXPRESSION will be better, but many test cases may fail if do that, I will improve it in next commit.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r1003932604
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
Per the changes in this PR, you are right that the semantic for `inQuery` does not change too much, I'm fine to leave it is for now.
The concern I raised is that `FrameTypeEnum#SELECT` is used vaguely to my understanding, e.g., we both use `SELECT` frame in `SqlInsert` and `SqlSelectOperator`, but they are different in my opinion: `SqlInsert` use `SELECT` frame to indicate that we'll go to a `SELECT QUERY`, and `SqlSelectOperator` use `SELECT` frame to indicate that we are in a `SELECT QUERY`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] l4wei commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
l4wei commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r996591355
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
SqlWriter#inQuery 's java doc says: "whether we are currently in a query context", not "whether we are currently in a sub-query context", so I think current java doc is appropriate?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r1002626396
##########
core/src/main/java/org/apache/calcite/sql/pretty/SqlPrettyWriter.java:
##########
@@ -398,10 +398,14 @@ public void setWindowDeclListNewline(boolean windowDeclListNewline) {
return (frame == null)
|| (frame.frameType == FrameTypeEnum.SELECT)
|| (frame.frameType == FrameTypeEnum.ORDER_BY)
- || (frame.frameType == FrameTypeEnum.WITH)
+ || (frame.frameType == FrameTypeEnum.WITH_BODY)
Review Comment:
I'm not sure about this, as I said above, I'm confused about `inQuery` now, could you explain your understanding of this method?
##########
core/src/main/java/org/apache/calcite/sql/SqlCall.java:
##########
@@ -118,7 +118,7 @@ public int operandCount() {
final SqlDialect dialect = writer.getDialect();
if (leftPrec > operator.getLeftPrec()
|| (operator.getRightPrec() <= rightPrec && (rightPrec != 0))
- || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION)) {
+ || writer.isAlwaysUseParentheses() && isA(SqlKind.EXPRESSION) && !writer.inWithBody()) {
Review Comment:
> IMHO, SET_QUERY should not be a EXPRESSION.
Then you can add `SET_QUERY` to `SqlKind.EXPRESSION`, and you do not need to add this condition `!writer.inWithBody()` here 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2938: [CALCITE-5299] JDBC adapter sometimes adds unnecessary parentheses around SELECT in WITH body
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2938:
URL: https://github.com/apache/calcite/pull/2938#discussion_r1003928315
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -3749,26 +3774,26 @@ void checkPeriodPredicate(Checker checker) {
+ "select * from e except "
+ "select * from f union "
+ "select * from g";
- final String expected = "((((SELECT *\n"
+ final String expected = "SELECT *\n"
Review Comment:
Since we are changing `SET_QUERY` to non-expression, could you also add a test case to make sure the precedence for set operators could work well? such as `(select * from a union select * from b) intersect select * from c` should be unparsed as is.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org