You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/04/23 22:52:26 UTC
[GitHub] [calcite] vineetgarg02 opened a new pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
vineetgarg02 opened a new pull request #1941:
URL: https://github.com/apache/calcite/pull/1941
----------------------------------------------------------------
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
[GitHub] [calcite] jinxing64 commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
jinxing64 commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r415249381
##########
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,
+ final RelNode input, int fetch, int offset) {
+ Double maxRowCount = mq.getMaxRowCount(input);
+ if (maxRowCount != null && maxRowCount <= 1) {
+ if (fetch == 0) {
+ return false;
+ } else if (fetch > 0) {
+ if (offset < 1) {
+ return true;
+ }
+ } else {
+ return true;
Review comment:
What if `fetch = -1` and `offset > 1`, should it return `true` ? If no, do we have test to cover ?
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r417718235
##########
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:
+1
----------------------------------------------------------------
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
[GitHub] [calcite] rubenada commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r420621049
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRule.java
##########
@@ -63,5 +80,30 @@ 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. If input's max row count is less than or equal to 1
+ // 2. If fetch is greater than 0
Review comment:
It seems that the method description does not match its logic (point 2 should read "If fetch is not 0").
I have the impression that the method could be simplified as:
```
if (inputMaxRowCount != null && inputMaxRowCount <= 1 && fetch != 0 && offset < 1)
return true;
return false;
```
----------------------------------------------------------------
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
[GitHub] [calcite] kgyrtkirk commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r420604462
##########
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:
only 2 of them...I think we might not be able to check core features of some rules because the RelBuiilder may fix it beforehand.
at some point we should either have to introduce a switch to disable `RelBuilder` being smart...or make the current one a derivative of a basic one.
but anyway...this is outside of the scope of this change - I'll file a ticket for it
----------------------------------------------------------------
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
[GitHub] [calcite] rubenada commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r420622826
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRule.java
##########
@@ -23,6 +23,7 @@
import org.apache.calcite.rel.RelCollationTraitDef;
import org.apache.calcite.rel.core.RelFactories;
import org.apache.calcite.rel.core.Sort;
+import org.apache.calcite.rex.RexLiteral;
import org.apache.calcite.tools.RelBuilderFactory;
/**
Review comment:
The SortRemoveRule's javadoc should be updated to reflect this new feature.
----------------------------------------------------------------
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
[GitHub] [calcite] vineetgarg02 commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
vineetgarg02 commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r418693129
##########
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,
+ final RelNode input, int fetch, int offset) {
+ Double maxRowCount = mq.getMaxRowCount(input);
+ if (maxRowCount != null && maxRowCount <= 1) {
+ if (fetch == 0) {
+ return false;
+ } else if (fetch > 0) {
+ if (offset < 1) {
+ return true;
+ }
+ } else {
+ return true;
Review comment:
I see that other databases like MySql, Hive etc do not allow OFFSET without LIMIT. But looks like Calcite does. I will add update the code to handle this case. Thanks @jinxing64
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1941:
URL: https://github.com/apache/calcite/pull/1941#discussion_r417037019
##########
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:
Both `fetch` and `offset` are public. So it seems not necessary.
----------------------------------------------------------------
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
[GitHub] [calcite] vineetgarg02 commented on a change in pull request #1941: [CALCITE-3952] Improve SortRemoveRule to remove Sort based on rowcount (Vineet Garg)
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org