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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
