julianhyde commented on code in PR #3206:
URL: https://github.com/apache/calcite/pull/3206#discussion_r1214942298
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1119,6 +1119,34 @@ public List<SqlNode> fieldList() {
};
}
+ /**
+ * Checks if a generated ORDER BY node is a literal value.
+ * @param node to check
+ * @return true if node is representing a literal value, false otherwise.
+ */
+ boolean isLiteral(SqlNode node) {
+ // In dialects that use ordinals, NumericLiterals are ordinals and
should be kept.
+ if (dialect.getConformance().isSortByOrdinal() && node instanceof
SqlNumericLiteral) {
+ return false;
+ }
+ if (node instanceof SqlLiteral) {
+ return true;
+ } else {
+ // The literal value may be wrapped in SqlBasicCalls applying postfix
operators.
+ while (node instanceof SqlBasicCall) {
Review Comment:
testing for SqlBasicCall is too broad. `isLiteral('a' || x)` would currently
return true, and that's wrong.
I think you're only interested in stripping away DESC, NULLS FIRST, NULLS
LAST. Is that true?
If that is true, then you might do better checking for literal in the
`RelFieldCollation` (i.e. before you call `toSql(field)`)
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1838,11 +1838,11 @@ private SqlDialect nonOrdinalDialect() {
}
/** Test case for
- * <a
href="https://issues.apache.org/jira/browse/CALCITE-5044";>[CALCITE-5044]
- * JDBC adapter generates integer literal in ORDER BY, which some dialects
- * wrongly interpret as a reference to a field</a>. */
Review Comment:
Maybe they can coexist. Add a method
`SqlConformance.isSortByLiteralAllowed()`, default true, false in BigQuery. The
test would have the old behavior if `isSortByLiteralAllowed` is true, the new
behavior if it is false.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1119,6 +1119,34 @@ public List<SqlNode> fieldList() {
};
}
+ /**
+ * Checks if a generated ORDER BY node is a literal value.
+ * @param node to check
+ * @return true if node is representing a literal value, false otherwise.
+ */
+ boolean isLiteral(SqlNode node) {
Review Comment:
method should be private, or name of method should indicate that tests
whether a node is a literal for purposes of the ORDER BY clause
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1854,19 +1854,25 @@ private SqlDialect nonOrdinalDialect() {
new RelFieldCollation(5, Direction.DESCENDING,
NullDirection.LAST))))
.project(b.field(2), b.field(1))
.build();
- // Default dialect rewrite numeric constant keys to string literal in the
order-by.
- // case1: numeric constant - rewrite it.
- // case2: string constant - no need rewrite it.
- // case3: wrap alias to numeric constant - rewrite it.
- // case4: wrap collation's info to numeric constant - rewrite it.
- relFn(relFn)
+ final Function<RelBuilder, RelNode> relFn2 = b -> b
+ .scan("EMP")
+ .project(b.literal(1), b.field(1), b.field(2), b.literal("23"),
+ b.alias(b.literal(12), "col1"), b.literal(34))
+ .sort(
+ RelCollations.of(
+ ImmutableList.of(
+ new RelFieldCollation(0), new RelFieldCollation(3), new
RelFieldCollation(4),
+ new RelFieldCollation(5, Direction.DESCENDING,
NullDirection.LAST))))
+ .project(b.field(2), b.field(1))
+ .build();
+ relFn(relFn1)
.ok("SELECT \"JOB\", \"ENAME\"\n"
+ "FROM \"scott\".\"EMP\"\n"
- + "ORDER BY '1', '23', '12', \"ENAME\", '34' DESC NULLS LAST")
- .dialect(nonOrdinalDialect())
- .ok("SELECT JOB, ENAME\n"
- + "FROM scott.EMP\n"
- + "ORDER BY 1, '23', 12, ENAME, 34 DESC NULLS LAST");
+ + "ORDER BY \"ENAME\"");
+ relFn(relFn2)
+ .ok("SELECT \"JOB\", \"ENAME\"\n"
+ + "FROM \"scott\".\"EMP\"");
+
}
Review Comment:
add a test case where the ORDER BY clause has one literal, that literal is
skipped by your code, and the generated query has no ORDER BY clause
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1838,11 +1838,11 @@ private SqlDialect nonOrdinalDialect() {
}
/** Test case for
- * <a
href="https://issues.apache.org/jira/browse/CALCITE-5044";>[CALCITE-5044]
- * JDBC adapter generates integer literal in ORDER BY, which some dialects
- * wrongly interpret as a reference to a field</a>. */
Review Comment:
You seem to have stomped on a unit test for CALCITE-5044. Does that mean
that you think that the functionality added by CALCITE-5044 is wrong, and
should be reverted?
If so, you should reference CALCITE-5044 in bug CALCITE-5724 and explain why
they are in conflict.
(My gut is that they are not in conflict, and you should keep the existing
test method and add a new one.)
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1854,19 +1854,25 @@ private SqlDialect nonOrdinalDialect() {
new RelFieldCollation(5, Direction.DESCENDING,
NullDirection.LAST))))
.project(b.field(2), b.field(1))
.build();
- // Default dialect rewrite numeric constant keys to string literal in the
order-by.
- // case1: numeric constant - rewrite it.
- // case2: string constant - no need rewrite it.
- // case3: wrap alias to numeric constant - rewrite it.
- // case4: wrap collation's info to numeric constant - rewrite it.
- relFn(relFn)
+ final Function<RelBuilder, RelNode> relFn2 = b -> b
+ .scan("EMP")
+ .project(b.literal(1), b.field(1), b.field(2), b.literal("23"),
+ b.alias(b.literal(12), "col1"), b.literal(34))
+ .sort(
+ RelCollations.of(
+ ImmutableList.of(
+ new RelFieldCollation(0), new RelFieldCollation(3), new
RelFieldCollation(4),
+ new RelFieldCollation(5, Direction.DESCENDING,
NullDirection.LAST))))
+ .project(b.field(2), b.field(1))
+ .build();
+ relFn(relFn1)
.ok("SELECT \"JOB\", \"ENAME\"\n"
+ "FROM \"scott\".\"EMP\"\n"
- + "ORDER BY '1', '23', '12', \"ENAME\", '34' DESC NULLS LAST")
- .dialect(nonOrdinalDialect())
- .ok("SELECT JOB, ENAME\n"
- + "FROM scott.EMP\n"
- + "ORDER BY 1, '23', 12, ENAME, 34 DESC NULLS LAST");
+ + "ORDER BY \"ENAME\"");
+ relFn(relFn2)
+ .ok("SELECT \"JOB\", \"ENAME\"\n"
+ + "FROM \"scott\".\"EMP\"");
+
Review Comment:
remove this blank line
##########
piglet/src/main/java/org/apache/calcite/piglet/PigRelToSqlConverter.java:
##########
@@ -102,7 +102,8 @@ public class PigRelToSqlConverter extends RelToSqlConverter
{
final List<SqlNode> orderList = Expressions.list();
for (RelFieldCollation orderKey :
winGroup.collation().getFieldCollations()) {
- orderList.add(builder.context.toSql(orderKey));
+ builder.addOrderItem(orderList, orderKey);
+
Review Comment:
remove this blank line
--
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]
