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/02/19 11:21:01 UTC
[GitHub] [calcite] JiajunBernoulli opened a new pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
JiajunBernoulli opened a new pull request #2724:
URL: https://github.com/apache/calcite/pull/2724
In standard SQL, the operand of union should not have limit or order by.So parse will fail in SqlParserTetest#testLimitUnion and SqlParserTetest#OrderUnion.
However, when users use parentheses, most engines allow it. We should retain the parentheses written by users.
--
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] JiajunBernoulli commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r819519606
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,71 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5013">[CALCITE-5013]
+ * Unparse SqlSetOperator should be retained parentheses
+ * when the operand has limit or offset</a>. */
+ @Test void testSetOpRetainParentheses() {
Review comment:
Do you mean we should retain it when there is a `where` condition? Or we always retain parentheses?
--
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] julianhyde commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r812444357
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -417,24 +418,52 @@ public static void unparseSqlIdentifierSyntax(
writer.endList(frame);
}
+ private static void unparseSetOpSyntax(
Review comment:
It's not obvious what this method does. Can you add brief javadoc.
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
+ // If the subquery parentheses of limit are discarded,
+ // the semantics of parsing will be affected.
+ // Parentheses should be retained, otherwise not required.
+ final String query = "select \"product_id\" from \"product\""
+ + "union all\n"
+ + "(select \"product_id\" from \"product\" limit 10)"
+ + "intersect all\n"
+ + "(select \"product_id\" from \"product\")";
+ final String expected = "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "UNION ALL\n"
+ + "SELECT *\n"
+ + "FROM ((SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "FETCH NEXT 10 ROWS ONLY)\n"
+ + "INTERSECT ALL\n"
+ + "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\")";
+ sql(query).ok(expected);
+
+ // Offset is the same as limit.
+ final String unionOffsetQuery = "select \"product_id\" from(\n"
+ + "(select \"product_id\" from \"product\" limit 10)\n"
+ + "union all\n"
+ + "(select \"product_id\" from \"product\" offset 5)\n"
+ + "union all\n"
+ + "(select \"product_id\" from \"product\" order by \"product_id\" limit 5 offset 5))";
+ final String unionOffsetRes = "SELECT *\n"
+ + "FROM (SELECT *\n"
+ + "FROM ((SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "FETCH NEXT 10 ROWS ONLY)\n"
+ + "UNION ALL\n"
+ + "(SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "OFFSET 5 ROWS))\n"
+ + "UNION ALL\n"
+ + "(SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "ORDER BY \"product_id\"\n"
+ + "OFFSET 5 ROWS\n"
+ + "FETCH NEXT 5 ROWS ONLY)) AS \"t6\"";
+ sql(unionOffsetQuery).ok(unionOffsetRes);
+ }
+
+ @Test void testSetOpOrderMaybeRetainParentheses() {
+ // Order by without limit/offset will be removed, so parentheses are not required.
Review comment:
this test is testing RelToSql, not SqlToRel. So I would remove this case (the first half of this method).
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -417,24 +418,52 @@ public static void unparseSqlIdentifierSyntax(
writer.endList(frame);
}
+ private static void unparseSetOpSyntax(
Review comment:
why is the function named `unparseSetOpSyntax`? The `Syntax` seems redundant, in which case it would be `unparseSetOp`. But then I see that it's unparsing a `SqlSelect`, not a set op. So I'm thoroughly confused.
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
Review comment:
capitalize 'limit' and other sql keywords
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
Review comment:
add javadoc referencing jira
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
Review comment:
There are only tests for UNION currently. Add a test for INTERSECT or EXCEPT, or perhaps a combination of set-ops.
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -417,24 +418,52 @@ public static void unparseSqlIdentifierSyntax(
writer.endList(frame);
}
+ private static void unparseSetOpSyntax(
+ SqlNode sqlNode,
+ SqlWriter writer,
+ int leftPrec,
+ int rightPrec) {
+ SqlSelect sqlSelect = (SqlSelect) sqlNode;
+ if (sqlSelect.getOrderList() != null
+ || sqlSelect.getFetch() != null
+ || sqlSelect.getOffset() != null) {
+ final SqlWriter.Frame operandFrame0 = writer.startList(FrameTypeEnum.SETOP, "(", ")");
+ sqlSelect.unparse(writer, leftPrec, rightPrec);
+ writer.endList(operandFrame0);
+ } else {
+ sqlSelect.unparse(writer, leftPrec, rightPrec);
+ }
+ }
+
public static void unparseBinarySyntax(
SqlOperator operator,
SqlCall call,
SqlWriter writer,
int leftPrec,
int rightPrec) {
assert call.operandCount() == 2;
+ boolean isSetOp = operator instanceof SqlSetOperator;
final SqlWriter.Frame frame =
writer.startList(
- (operator instanceof SqlSetOperator)
+ isSetOp
? SqlWriter.FrameTypeEnum.SETOP
: SqlWriter.FrameTypeEnum.SIMPLE);
- call.operand(0).unparse(writer, leftPrec, operator.getLeftPrec());
+ SqlNode operand0 = call.operand(0);
+ if (isSetOp && operand0.getKind() == SqlKind.SELECT) {
+ unparseSetOpSyntax(operand0, writer, leftPrec, operator.getLeftPrec());
+ } else {
+ operand0.unparse(writer, leftPrec, operator.getLeftPrec());
+ }
final boolean needsSpace = operator.needsSpace();
writer.setNeedWhitespace(needsSpace);
writer.sep(operator.getName());
writer.setNeedWhitespace(needsSpace);
- call.operand(1).unparse(writer, operator.getRightPrec(), rightPrec);
+ SqlNode operand1 = call.operand(1);
Review comment:
Is the logic of the `if` statement below repeated inside the `unparseSetOpSyntax` function?
--
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] JiajunBernoulli commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r819519606
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,71 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5013">[CALCITE-5013]
+ * Unparse SqlSetOperator should be retained parentheses
+ * when the operand has limit or offset</a>. */
+ @Test void testSetOpRetainParentheses() {
Review comment:
Do you mean to we should retain it when there is a `where` condition? Or we always retain parentheses?
--
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] JiajunBernoulli commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r812579983
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -417,24 +418,52 @@ public static void unparseSqlIdentifierSyntax(
writer.endList(frame);
}
+ private static void unparseSetOpSyntax(
Review comment:
Sorry, the previous naming was really inappropriate.
I renamed it to `unparseBinaryOperand`(Write parentheses for special operands in 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r819937457
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,71 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5013">[CALCITE-5013]
+ * Unparse SqlSetOperator should be retained parentheses
+ * when the operand has limit or offset</a>. */
+ @Test void testSetOpRetainParentheses() {
Review comment:
I disagree that we should add parentheses for aesthetics. See my comment in https://issues.apache.org/jira/browse/CALCITE-5013 and let's discuss further there.
--
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] JiajunBernoulli commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r812579606
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
Review comment:
Done, and I renamed this function to `testSetOpRetainParentheses`.
--
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] JiajunBernoulli commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r812579519
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -417,24 +418,52 @@ public static void unparseSqlIdentifierSyntax(
writer.endList(frame);
}
+ private static void unparseSetOpSyntax(
Review comment:
Ok, I've added a doc to explain it.
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
Review comment:
I changed it, but I'm a little confused about how to choose case.
In `testUnionWrappedInASelect` and `testAliasOnStarNoExcessQuotes`, sql keywords is lowercase.
--
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] NobiGo commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r819230289
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,71 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5013">[CALCITE-5013]
+ * Unparse SqlSetOperator should be retained parentheses
+ * when the operand has limit or offset</a>. */
+ @Test void testSetOpRetainParentheses() {
+ // Parentheses will be discarded, because semantics not be affected.
+ final String discardedParenthesesQuery = "SELECT \"product_id\" FROM \"product\""
+ + "UNION ALL\n"
+ + "(SELECT \"product_id\" FROM \"product\" WHERE \"product_id\" > 10)\n"
+ + "INTERSECT ALL\n"
+ + "(SELECT \"product_id\" FROM \"product\" )";
+ final String discardedParenthesesRes = "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "UNION ALL\n"
+ + "SELECT *\n"
+ + "FROM (SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "WHERE \"product_id\" > 10\n"
+ + "INTERSECT ALL\n"
+ + "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\")";
+ sql(discardedParenthesesQuery).ok(discardedParenthesesRes);
+
+ // Parentheses will be retained because subquery has limit or offset.
+ // If parentheses are discarded the semantics of parsing will be affected.
Review comment:
redundant blank space after the parentheses
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,71 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5013">[CALCITE-5013]
+ * Unparse SqlSetOperator should be retained parentheses
+ * when the operand has limit or offset</a>. */
+ @Test void testSetOpRetainParentheses() {
Review comment:
I'm suspect whether we should remove the parentheses?
If the SQL is :
```
"SELECT \"product_id\" FROM \"product\""
+ "UNION ALL\n"
+ "(SELECT \"product_id\" FROM \"product\")\n"
+ "INTERSECT ALL\n"
+ "(SELECT \"product_id\" FROM \"product\" WHERE \"product_id\" > 10)"
```
```
SELECT \"product_id\"
FROM \"foodmart\".\"product\"
UNION ALL
SELECT *
FROM (SELECT \"product_id\"
FROM \"foodmart\".\"product\"
INTERSECT ALL
SELECT \"product_id\"
FROM \"foodmart\".\"product\"
WHERE \"product_id\" > 10)
```
I think it has become unclear? So maybe we should stay the parentheses?
--
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] JiajunBernoulli commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r825255397
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,71 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5013">[CALCITE-5013]
+ * Unparse SqlSetOperator should be retained parentheses
+ * when the operand has limit or offset</a>. */
+ @Test void testSetOpRetainParentheses() {
+ // Parentheses will be discarded, because semantics not be affected.
+ final String discardedParenthesesQuery = "SELECT \"product_id\" FROM \"product\""
+ + "UNION ALL\n"
+ + "(SELECT \"product_id\" FROM \"product\" WHERE \"product_id\" > 10)\n"
+ + "INTERSECT ALL\n"
+ + "(SELECT \"product_id\" FROM \"product\" )";
+ final String discardedParenthesesRes = "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "UNION ALL\n"
+ + "SELECT *\n"
+ + "FROM (SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "WHERE \"product_id\" > 10\n"
+ + "INTERSECT ALL\n"
+ + "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\")";
+ sql(discardedParenthesesQuery).ok(discardedParenthesesRes);
+
+ // Parentheses will be retained because subquery has limit or offset.
+ // If parentheses are discarded the semantics of parsing will be affected.
Review comment:
done
--
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] JiajunBernoulli commented on a change in pull request #2724: [CACLITE-5013] Retain parentheses if a setop has an LIMIT clause
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on a change in pull request #2724:
URL: https://github.com/apache/calcite/pull/2724#discussion_r812579698
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
+ // If the subquery parentheses of limit are discarded,
+ // the semantics of parsing will be affected.
+ // Parentheses should be retained, otherwise not required.
+ final String query = "select \"product_id\" from \"product\""
+ + "union all\n"
+ + "(select \"product_id\" from \"product\" limit 10)"
+ + "intersect all\n"
+ + "(select \"product_id\" from \"product\")";
+ final String expected = "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "UNION ALL\n"
+ + "SELECT *\n"
+ + "FROM ((SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "FETCH NEXT 10 ROWS ONLY)\n"
+ + "INTERSECT ALL\n"
+ + "SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\")";
+ sql(query).ok(expected);
+
+ // Offset is the same as limit.
+ final String unionOffsetQuery = "select \"product_id\" from(\n"
+ + "(select \"product_id\" from \"product\" limit 10)\n"
+ + "union all\n"
+ + "(select \"product_id\" from \"product\" offset 5)\n"
+ + "union all\n"
+ + "(select \"product_id\" from \"product\" order by \"product_id\" limit 5 offset 5))";
+ final String unionOffsetRes = "SELECT *\n"
+ + "FROM (SELECT *\n"
+ + "FROM ((SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "FETCH NEXT 10 ROWS ONLY)\n"
+ + "UNION ALL\n"
+ + "(SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "OFFSET 5 ROWS))\n"
+ + "UNION ALL\n"
+ + "(SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "ORDER BY \"product_id\"\n"
+ + "OFFSET 5 ROWS\n"
+ + "FETCH NEXT 5 ROWS ONLY)) AS \"t6\"";
+ sql(unionOffsetQuery).ok(unionOffsetRes);
+ }
+
+ @Test void testSetOpOrderMaybeRetainParentheses() {
+ // Order by without limit/offset will be removed, so parentheses are not required.
Review comment:
Ok, I've deleted this test case.
##########
File path: core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
##########
@@ -3301,6 +3301,85 @@ private void checkHavingAliasSameAsColumn(boolean upperAlias) {
sql(query).ok(expected);
}
+ @Test void testSetOpLimitShouldRetainParentheses() {
Review comment:
There has been a case of UNION and INTERSECT before, I've added a combination of set-ops.
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##########
@@ -417,24 +418,52 @@ public static void unparseSqlIdentifierSyntax(
writer.endList(frame);
}
+ private static void unparseSetOpSyntax(
+ SqlNode sqlNode,
+ SqlWriter writer,
+ int leftPrec,
+ int rightPrec) {
+ SqlSelect sqlSelect = (SqlSelect) sqlNode;
+ if (sqlSelect.getOrderList() != null
+ || sqlSelect.getFetch() != null
+ || sqlSelect.getOffset() != null) {
+ final SqlWriter.Frame operandFrame0 = writer.startList(FrameTypeEnum.SETOP, "(", ")");
+ sqlSelect.unparse(writer, leftPrec, rightPrec);
+ writer.endList(operandFrame0);
+ } else {
+ sqlSelect.unparse(writer, leftPrec, rightPrec);
+ }
+ }
+
public static void unparseBinarySyntax(
SqlOperator operator,
SqlCall call,
SqlWriter writer,
int leftPrec,
int rightPrec) {
assert call.operandCount() == 2;
+ boolean isSetOp = operator instanceof SqlSetOperator;
final SqlWriter.Frame frame =
writer.startList(
- (operator instanceof SqlSetOperator)
+ isSetOp
? SqlWriter.FrameTypeEnum.SETOP
: SqlWriter.FrameTypeEnum.SIMPLE);
- call.operand(0).unparse(writer, leftPrec, operator.getLeftPrec());
+ SqlNode operand0 = call.operand(0);
+ if (isSetOp && operand0.getKind() == SqlKind.SELECT) {
+ unparseSetOpSyntax(operand0, writer, leftPrec, operator.getLeftPrec());
+ } else {
+ operand0.unparse(writer, leftPrec, operator.getLeftPrec());
+ }
final boolean needsSpace = operator.needsSpace();
writer.setNeedWhitespace(needsSpace);
writer.sep(operator.getName());
writer.setNeedWhitespace(needsSpace);
- call.operand(1).unparse(writer, operator.getRightPrec(), rightPrec);
+ SqlNode operand1 = call.operand(1);
Review comment:
I moved it to `unparseBinaryOperand`.
--
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