xndai commented on a change in pull request #1884:
URL: https://github.com/apache/calcite/pull/1884#discussion_r422814362
##########
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:
This was the very first proposal from Haisheng. We debated a lot and
came to agreement to use RelBuilder due to a few benefits - 1. reuse existing
RelBuilder logics. For example, it would be very easy to create sort limit as
well. 2. support more scenarios, not just the enforcement. 3. support building
rel tree with multiple conventions (as discussed in JIRA)
----------------------------------------------------------------
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:
us...@infra.apache.org
