julianhyde commented on a change in pull request #2679:
URL: https://github.com/apache/calcite/pull/2679#discussion_r787273389
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -683,6 +683,13 @@ protected void convertSelectImpl(
convertFrom(
bb,
select.getFrom());
+ if (RelOptUtil.isOrder(bb.root)
Review comment:
this complex 'if' needs a comment explaining what it is doing.
Remove the space before ')'.
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -5898,17 +5965,32 @@ private void checkMapSchemaDirectConnection(String s)
throws SQLException {
+ "order by \"empid\" limit 2", null);
with
.query("select \"name\" from \"adhoc\".V order by \"name\"")
+ .withHook(Hook.SQL2REL_CONVERTER_CONFIG_BUILDER,
+ (Consumer<Holder<Config>>) configHolder ->
+
configHolder.set(configHolder.get().withRemoveSortInSubQuery(false)))
.returns("name=Bill\n"
+ "name=Theodore\n");
// Now a sub-query with ORDER BY and LIMIT clauses. (Same net effect, but
// ORDER BY and LIMIT in sub-query were not standard SQL until SQL:2008.)
+ with
+ .query("select \"name\" from (\n"
+ + "select * from \"adhoc\".\"EMPLOYEES\" where \"deptno\" = 10\n"
+ + "order by \"empid\" limit 2)\n"
+ + "order by \"name\"")
+ .withHook(Hook.SQL2REL_CONVERTER_CONFIG_BUILDER,
+ (Consumer<Holder<SqlToRelConverter.Config>>) configHolder ->
+
configHolder.set(configHolder.get().withRemoveSortInSubQuery(false)))
+ .returns("name=Bill\n"
Review comment:
Same comments as previously. I would want the test to succeed even
without the hook.
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -5898,17 +5965,32 @@ private void checkMapSchemaDirectConnection(String s)
throws SQLException {
+ "order by \"empid\" limit 2", null);
with
.query("select \"name\" from \"adhoc\".V order by \"name\"")
+ .withHook(Hook.SQL2REL_CONVERTER_CONFIG_BUILDER,
+ (Consumer<Holder<Config>>) configHolder ->
+
configHolder.set(configHolder.get().withRemoveSortInSubQuery(false)))
.returns("name=Bill\n"
+ "name=Theodore\n");
// Now a sub-query with ORDER BY and LIMIT clauses. (Same net effect, but
// ORDER BY and LIMIT in sub-query were not standard SQL until SQL:2008.)
+ with
+ .query("select \"name\" from (\n"
+ + "select * from \"adhoc\".\"EMPLOYEES\" where \"deptno\" = 10\n"
+ + "order by \"empid\" limit 2)\n"
+ + "order by \"name\"")
+ .withHook(Hook.SQL2REL_CONVERTER_CONFIG_BUILDER,
+ (Consumer<Holder<SqlToRelConverter.Config>>) configHolder ->
+
configHolder.set(configHolder.get().withRemoveSortInSubQuery(false)))
+ .returns("name=Bill\n"
+ + "name=Theodore\n");
+
with
.query("select \"name\" from (\n"
+ "select * from \"adhoc\".\"EMPLOYEES\" where \"deptno\" = 10\n"
+ "order by \"empid\" limit 2)\n"
+ "order by \"name\"")
.returns("name=Bill\n"
+ + "name=Sebastian\n"
+ "name=Theodore\n");
Review comment:
This is wrong. The LIMIT should not be removed. The sub-query should
continue to return two rows.
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -5898,17 +5965,32 @@ private void checkMapSchemaDirectConnection(String s)
throws SQLException {
+ "order by \"empid\" limit 2", null);
with
.query("select \"name\" from \"adhoc\".V order by \"name\"")
+ .withHook(Hook.SQL2REL_CONVERTER_CONFIG_BUILDER,
+ (Consumer<Holder<Config>>) configHolder ->
+
configHolder.set(configHolder.get().withRemoveSortInSubQuery(false)))
Review comment:
I don't understand why this hook is still needed. The view is not
top-level, so its results should not be sorted, but the LIMIT on the sorted
data should still apply. Does the test succeed if you remove the hook?
--
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]
