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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
