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/04/07 11:27:32 UTC
[GitHub] [calcite] hannerwang opened a new pull request, #2768: [CALCITE-5086] Calcite supports OFFSET start LIMIT count expression
hannerwang opened a new pull request, #2768:
URL: https://github.com/apache/calcite/pull/2768
support trino style query like: SELECT * FROM users OFFSET 10 LIMIT 10
--
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] hannerwang commented on a diff in pull request #2768: [CALCITE-5086] Core parser should allow "OFFSET" to occur before "LIMIT"
Posted by GitBox <gi...@apache.org>.
hannerwang commented on code in PR #2768:
URL: https://github.com/apache/calcite/pull/2768#discussion_r855746350
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -623,8 +630,7 @@ SqlNode OrderedQueryOrExpr(ExprContext exprContext) :
{
SqlNode e;
SqlNodeList orderBy = null;
- SqlNode start = null;
- SqlNode count = null;
+ SqlNode[] startCount = {null, null};
}
{
Review Comment:
Hi @chunweilei , thanks for your review, I'm glad to hear your suggestions to improve 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] hannerwang commented on a diff in pull request #2768: [CALCITE-5086] Core parser should allow "OFFSET" to occur before "LIMIT"
Posted by GitBox <gi...@apache.org>.
hannerwang commented on code in PR #2768:
URL: https://github.com/apache/calcite/pull/2768#discussion_r855748201
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -3369,6 +3369,48 @@ void checkPeriodPredicate(Checker checker) {
.fails("(?s).*Encountered \"all\" at line 1.*");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5086">[CALCITE-5086]
+ * Calcite supports OFFSET start LIMIT count expression</a>. */
+ @Test void testOffsetStartLimitCount() {
+ final String error = "'OFFSET start LIMIT count' is not allowed under the "
+ + "current SQL conformance level";
+ sql("select a from foo order by b, c offset 1 limit 2")
+ .withConformance(SqlConformanceEnum.LENIENT)
+ .ok("SELECT `A`\n"
+ + "FROM `FOO`\n"
+ + "ORDER BY `B`, `C`\n"
+ + "OFFSET 1 ROWS\n"
+ + "FETCH NEXT 2 ROWS ONLY");
+
+ sql("select a from foo order by b, c offset 1 limit ^2^")
+ .withConformance(SqlConformanceEnum.DEFAULT)
+ .fails(error);
+
+ // "limit 2" overrides the earlier "offset 4"
+ final String expected3 = "SELECT `A`\n"
+ + "FROM `FOO`\n"
+ + "OFFSET 2 ROWS\n"
+ + "FETCH NEXT 3 ROWS ONLY";
+ sql("select a from foo offset 4 limit 2,3")
+ .withConformance(SqlConformanceEnum.LENIENT)
Review Comment:
Calcite supports limit xx,xx offset xx now, and the offset will override previous limit, the syntax looks strange also, however, I think it will be ok in lenient mode.
--
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] chunweilei commented on a diff in pull request #2768: [CALCITE-5086] Core parser should allow "OFFSET" to occur before "LIMIT"
Posted by GitBox <gi...@apache.org>.
chunweilei commented on code in PR #2768:
URL: https://github.com/apache/calcite/pull/2768#discussion_r855707344
##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -3369,6 +3369,48 @@ void checkPeriodPredicate(Checker checker) {
.fails("(?s).*Encountered \"all\" at line 1.*");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5086">[CALCITE-5086]
+ * Calcite supports OFFSET start LIMIT count expression</a>. */
+ @Test void testOffsetStartLimitCount() {
+ final String error = "'OFFSET start LIMIT count' is not allowed under the "
+ + "current SQL conformance level";
+ sql("select a from foo order by b, c offset 1 limit 2")
+ .withConformance(SqlConformanceEnum.LENIENT)
+ .ok("SELECT `A`\n"
+ + "FROM `FOO`\n"
+ + "ORDER BY `B`, `C`\n"
+ + "OFFSET 1 ROWS\n"
+ + "FETCH NEXT 2 ROWS ONLY");
+
+ sql("select a from foo order by b, c offset 1 limit ^2^")
+ .withConformance(SqlConformanceEnum.DEFAULT)
+ .fails(error);
+
+ // "limit 2" overrides the earlier "offset 4"
+ final String expected3 = "SELECT `A`\n"
+ + "FROM `FOO`\n"
+ + "OFFSET 2 ROWS\n"
+ + "FETCH NEXT 3 ROWS ONLY";
+ sql("select a from foo offset 4 limit 2,3")
+ .withConformance(SqlConformanceEnum.LENIENT)
Review Comment:
Do other systems support 'offset xx limit xx,xx'? It looks a little wired.
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -623,8 +630,7 @@ SqlNode OrderedQueryOrExpr(ExprContext exprContext) :
{
SqlNode e;
SqlNodeList orderBy = null;
- SqlNode start = null;
- SqlNode count = null;
+ SqlNode[] startCount = {null, null};
}
{
Review Comment:
Is it necessary to use Array to indicate the first value is the offset and the second value is the limit? It maybe not a good practice.
--
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 closed pull request #2768: [CALCITE-5086] Core parser should allow "OFFSET" to occur before "LIMIT"
Posted by GitBox <gi...@apache.org>.
julianhyde closed pull request #2768: [CALCITE-5086] Core parser should allow "OFFSET" to occur before "LIMIT"
URL: https://github.com/apache/calcite/pull/2768
--
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