vineetgarg02 commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r418693219
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Sort.java
##########
@@ -143,6 +143,14 @@ public abstract Sort copy(RelTraitSet traitSet, RelNode
newInput,
return fieldExps;
}
+ public RexNode getFetch() {
+ return fetch;
+ }
+
+ public RexNode getOffset() {
+ return offset;
+ }
Review comment:
@chunweilei I think it is still not a good idea to access the members
directly. Also in future we can thinking of changing the access level in which
case changes will be minimum and confined to the class itself.
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRule.java
##########
@@ -63,5 +83,27 @@ public SortRemoveRule(RelBuilderFactory relBuilderFactory) {
.getTrait(RelCollationTraitDef.INSTANCE);
final RelTraitSet traits =
sort.getInput().getTraitSet().replace(collation);
call.transformTo(convert(sort.getInput(), traits));
+ return true;
+ }
+
+ // Sort is removed if all of following conditions are met
+ // 1. Sort's input is guaranteed to produce at most one row
+ // 2. If fetch is greater than 0
+ // 3. If offset is less than 1
+ public static boolean shouldRemoveSortBasedOnRowCount(final RelMetadataQuery
mq,
Review comment:
@kgyrtkirk Make sense. I will update the code.
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -2483,6 +2484,11 @@ public RelBuilder sortLimit(int offset, int fetch,
RexNode... nodes) {
*/
public RelBuilder sortLimit(int offset, int fetch,
Iterable<? extends RexNode> nodes) {
+ if (SortRemoveRule.shouldRemoveSortBasedOnRowCount(
+ peek().getCluster().getMetadataQuery(), peek(), fetch, offset)) {
Review comment:
Make sense, i will update the code.
##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -1010,6 +1011,39 @@ private void
basePushFilterPastAggWithGroupingSets(boolean unchanged)
sql(sql).with(program).check();
}
+ @Test void testSortRemoveRowCountBased() {
+ final HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(SortRemoveRule.INSTANCE)
+ .build();
+ final String sql = "select count(*) as c\n"
+ + "from sales.emp\n"
+ + "where deptno = 10\n"
+ + "limit 10";
+ sql(sql).with(program).check();
+ }
+
+ @Test void testSortRemoveRowCountBasedZeroLimit() {
+ final HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(SortRemoveRule.INSTANCE)
Review comment:
@kgyrtkirk Do you mean this particular test or all tests added in
`RelOptRulesTest` in this pull request? I quickly checked other tests by
removing the rule but they fail so looks like `RelBuilder` is not being used.
----------------------------------------------------------------
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:
[email protected]
