[GitHub] [calcite] julianhyde commented on a change in pull request #2679: [CALCITE-4323] View query should be top node avoid losing sorting

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org