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