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 2021/04/15 08:02:34 UTC
[GitHub] [calcite] Aaaaaaron opened a new pull request #2398: [CALCITE-2338] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Aaaaaaron opened a new pull request #2398:
URL: https://github.com/apache/calcite/pull/2398
--
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] Aaaaaaron commented on pull request #2398: [CALCITE-2338] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#issuecomment-820238444
> The related Jira ([CALCITE-2338](https://issues.apache.org/jira/browse/CALCITE-2338)) is closed (released in 1.19), I'm not sure we can attach (and merge) a new PR to this ticket.
> Maybe you could create a new Jira and link it as related to [CALCITE-2338](https://issues.apache.org/jira/browse/CALCITE-2338)?
Sure!
--
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] Aaaaaaron commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r614009314
##########
File path: core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
##########
@@ -3612,19 +3615,23 @@ join sales.emp e on e.deptno = d.deptno and d.deptno not in (4, 6)]]>
<Resource name="planBefore">
<![CDATA[
LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], EMPNO0=[$9], ENAME0=[$10], JOB0=[$11], MGR0=[$12], HIREDATE0=[$13], SAL0=[$14], COMM0=[$15], DEPTNO0=[$16], SLACKER0=[$17])
- LogicalJoin(condition=[AND(=($16, $7), NOT(OR(=($7, 4), =($7, 6))))], joinType=[inner])
- LogicalTableScan(table=[[CATALOG, SALES, EMP]])
- LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalFilter(condition=[true])
Review comment:
Hi @zabetak , it these rule, simplification is not on:
--
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] zabetak closed pull request #2398: [CALCITE-4583] Make simplification controllable in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2398:
URL: https://github.com/apache/calcite/pull/2398
--
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] zabetak commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r614071886
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,28 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
+ final RexNode conjunctionPredicates;
+ if (config.simplify()) {
+ conjunctionPredicates = simplifier.simplifyFilterPredicates(predicates);
+ if (conjunctionPredicates == null) {
+ return empty();
+ }
+ if (conjunctionPredicates.isAlwaysTrue()) {
+ return this;
+ }
Review comment:
Move lines 1343 to 1348 after 1352.
--
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 #2398: [CALCITE-2338] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r613864592
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,27 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
- }
+ RexNode conjunctionPredicates =
Review comment:
This computation of `conjunctionPredicates = RexUtil.composeConjunction(simplifier.rexBuilder, predicates);` seems useless if RelBuilder has `config.simplify()` (which should be most of the cases). Maybe it could be computed only when required? Proposal:
```
final RexNode conjunctionPredicate;
if (config.simplify()) { //same code in the if block
conjunctionPredicates = simplifier.simplifyFilterPredicates(predicates);
if (conjunctionPredicates == null)
return empty();
if (conjunctionPredicates.isAlwaysTrue())
return this;
}
else {
conjunctionPredicates = RexUtil.composeConjunction(simplifier.rexBuilder, predicates);
}
...
```
--
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] Aaaaaaron commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r613876737
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,27 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
- }
+ RexNode conjunctionPredicates =
Review comment:
Sure, Revised, the original intention was to streamline the code. Thanks for your review.
--
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] Aaaaaaron commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r614010474
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,28 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
+ final RexNode conjunctionPredicates;
+ if (config.simplify()) {
+ conjunctionPredicates = simplifier.simplifyFilterPredicates(predicates);
+ if (conjunctionPredicates == null) {
+ return empty();
+ }
+ if (conjunctionPredicates.isAlwaysTrue()) {
+ return this;
+ }
Review comment:
Sorry, not get your point😂, can you desc more?
We should only simplifyFilterPredicates when config.simplify() is true.
--
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] zabetak commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r614000261
##########
File path: core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
##########
@@ -3612,19 +3615,23 @@ join sales.emp e on e.deptno = d.deptno and d.deptno not in (4, 6)]]>
<Resource name="planBefore">
<![CDATA[
LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], EMPNO0=[$9], ENAME0=[$10], JOB0=[$11], MGR0=[$12], HIREDATE0=[$13], SAL0=[$14], COMM0=[$15], DEPTNO0=[$16], SLACKER0=[$17])
- LogicalJoin(condition=[AND(=($16, $7), NOT(OR(=($7, 4), =($7, 6))))], joinType=[inner])
- LogicalTableScan(table=[[CATALOG, SALES, EMP]])
- LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalFilter(condition=[true])
Review comment:
Why do all these redundant filters appear? Should we possibly change the value of `config.simplify()` somewhere?
--
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] zabetak commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r614004179
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,28 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
+ final RexNode conjunctionPredicates;
+ if (config.simplify()) {
+ conjunctionPredicates = simplifier.simplifyFilterPredicates(predicates);
+ if (conjunctionPredicates == null) {
+ return empty();
+ }
+ if (conjunctionPredicates.isAlwaysTrue()) {
+ return this;
+ }
Review comment:
I think this nested ifs should not be here but outside the main if (as it was before) cause otherwise we might be changing unintentionally the old behavior.
--
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] Aaaaaaron commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r614010474
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,28 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
+ final RexNode conjunctionPredicates;
+ if (config.simplify()) {
+ conjunctionPredicates = simplifier.simplifyFilterPredicates(predicates);
+ if (conjunctionPredicates == null) {
+ return empty();
+ }
+ if (conjunctionPredicates.isAlwaysTrue()) {
+ return this;
+ }
Review comment:
Sorry, not get your point😂, can you desc more?
--
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] Aaaaaaron commented on a change in pull request #2398: [CALCITE-4583] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r614534822
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,28 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
+ final RexNode conjunctionPredicates;
+ if (config.simplify()) {
+ conjunctionPredicates = simplifier.simplifyFilterPredicates(predicates);
+ if (conjunctionPredicates == null) {
+ return empty();
+ }
+ if (conjunctionPredicates.isAlwaysTrue()) {
+ return this;
+ }
Review comment:
Nice point, this will still do simplification, but I think this degree of optimization is acceptable.
--
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] Aaaaaaron commented on a change in pull request #2398: [CALCITE-2338] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#discussion_r613876737
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -1334,22 +1334,27 @@ public RelBuilder filter(Iterable<CorrelationId> variablesSet,
*
* <p>The predicates are combined using AND,
* and optimized in a similar way to the {@link #and} method.
- * If the result is TRUE no filter is created. */
+ * If simplification is on and the result is TRUE, no filter is created. */
public RelBuilder filter(Iterable<CorrelationId> variablesSet,
Iterable<? extends RexNode> predicates) {
- final RexNode simplifiedPredicates =
- simplifier.simplifyFilterPredicates(predicates);
- if (simplifiedPredicates == null) {
- return empty();
- }
+ RexNode conjunctionPredicates =
Review comment:
Sure, Revised, the original intention was to streamline the code.
--
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 pull request #2398: [CALCITE-2338] Make simplification API more conservative in "RelBuilder#filter" (Jiatao Tao)
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2398:
URL: https://github.com/apache/calcite/pull/2398#issuecomment-820232396
The related Jira (CALCITE-2338) is closed (released in 1.19), I'm not sure we can attach (and merge) a new PR to this ticket.
Maybe you could create a new Jira and link it as related to CALCITE-2338?
--
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