[GitHub] [calcite] zabetak commented on a change in pull request #1884: [CALCITE-3972] Allow RelBuilder to create RelNode with convention and use it for trait convert

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

zabetak commented on a change in pull request #1884:
URL: https://github.com/apache/calcite/pull/1884#discussion_r422704715



##########
File path: core/src/main/java/org/apache/calcite/rel/RelCollationTraitDef.java
##########
@@ -69,10 +70,14 @@ public RelNode convert(
       return null;
     }
 
-    // Create a logical sort, then ask the planner to convert its remaining
-    // traits (e.g. convert it to an EnumerableSortRel if rel is enumerable
-    // convention)
-    final Sort sort = LogicalSort.create(rel, toCollation, null, null);
+    // Create a sort operator based on given convention
+    Convention toConvention = rel.getConvention();
+    RelBuilder builder =
+        RelFactories.LOGICAL_BUILDER.create(rel.getCluster(), null);
+    RelNode sort = builder.push(rel)
+                          .adoptConvention(toConvention)
+                          .sort(toCollation)
+                          .build();

Review comment:
       Based on the new APIs, isn't simpler to use the following code:
   ```
   RelFactories.SortFactory sortFactory = toConvention.getRelFactories().sortFactory;
   Sort sort = (Sort) sortFactory.createSort(rel, toCollation, null, null);
   ```
   It also avoids creating a builder that you will just throw away.




----------------------------------------------------------------
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.

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