mihaibudiu commented on code in PR #5238:
URL: https://github.com/apache/calcite/pull/5238#discussion_r3919236897
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -15262,6 +15283,36 @@ private static SqlIdentifier
rewriteIdentifier(SqlIdentifier sqlIdentifier) {
}
}
+ /** Validator that simulates rewriting a ROWNUM predicate to FETCH. */
Review Comment:
I think @julianhyde asked for a quidem test: a SQL program in a .iq file.
If that exercises this case, it's better than a test such as this one.
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -1904,9 +1904,12 @@ && requireNonNull(((SqlNumericLiteral)
node).bigDecimalValue())
if (orderBy.query instanceof SqlSelect) {
SqlSelect select = (SqlSelect) orderBy.query;
- // Don't clobber existing ORDER BY. It may be needed for
- // an order-sensitive function like RANK.
- if (select.getOrderList() == null) {
+ // Don't clobber existing ORDER BY, OFFSET, or FETCH. ORDER BY may be
+ // needed for an order-sensitive function like RANK; OFFSET and FETCH
+ // establish a separate pagination level.
Review Comment:
I don't know what "pagination" means. this term does not appear in the
codebase.
The fix looks safe, though.
--
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]
