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/08/11 09:50:11 UTC
[GitHub] [calcite] julianhyde opened a new pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
julianhyde opened a new pull request #2105:
URL: https://github.com/apache/calcite/pull/2105
----------------------------------------------------------------
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] julianhyde commented on a change in pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r469740562
##########
File path: core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml
##########
@@ -3763,7 +3765,7 @@ LogicalProject(DEPTNO=[$7])
</TestCase>
<TestCase name="testReduceConstExpr">
<Resource name="sql">
- <![CDATA[select sum(case when 'y' = 'n' then ename else 1 end) from emp]]>
+ <![CDATA[select sum(case when 'y' = 'n' then ename else 0.1 end) from emp]]>
</Resource>
Review comment:
Yes, the 'sql' originates in `SqlToRelConverterTest.java`. I just updated `SqlToRelConverterTest.xml` to match what was already in the java.
----------------------------------------------------------------
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 #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r469659100
##########
File path: core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml
##########
@@ -3763,7 +3765,7 @@ LogicalProject(DEPTNO=[$7])
</TestCase>
<TestCase name="testReduceConstExpr">
<Resource name="sql">
- <![CDATA[select sum(case when 'y' = 'n' then ename else 1 end) from emp]]>
+ <![CDATA[select sum(case when 'y' = 'n' then ename else 0.1 end) from emp]]>
</Resource>
Review comment:
Got it, 3q.
##########
File path: core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml
##########
@@ -3763,7 +3765,7 @@ LogicalProject(DEPTNO=[$7])
</TestCase>
<TestCase name="testReduceConstExpr">
<Resource name="sql">
- <![CDATA[select sum(case when 'y' = 'n' then ename else 1 end) from emp]]>
+ <![CDATA[select sum(case when 'y' = 'n' then ename else 0.1 end) from emp]]>
</Resource>
Review comment:
Got it, thanks.
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r469611797
##########
File path: core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml
##########
@@ -3763,7 +3765,7 @@ LogicalProject(DEPTNO=[$7])
</TestCase>
<TestCase name="testReduceConstExpr">
<Resource name="sql">
- <![CDATA[select sum(case when 'y' = 'n' then ename else 1 end) from emp]]>
+ <![CDATA[select sum(case when 'y' = 'n' then ename else 0.1 end) from emp]]>
</Resource>
Review comment:
In fact, the current test case is using using `0.1`: https://github.com/apache/calcite/blob/37b8cdbb381369e773229a81ae69ab5c1df34f3e/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java#L2649
##########
File path: core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml
##########
@@ -3763,7 +3765,7 @@ LogicalProject(DEPTNO=[$7])
</TestCase>
<TestCase name="testReduceConstExpr">
<Resource name="sql">
- <![CDATA[select sum(case when 'y' = 'n' then ename else 1 end) from emp]]>
+ <![CDATA[select sum(case when 'y' = 'n' then ename else 0.1 end) from emp]]>
</Resource>
Review comment:
In fact, the current test case is using `0.1`: https://github.com/apache/calcite/blob/37b8cdbb381369e773229a81ae69ab5c1df34f3e/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java#L2649
----------------------------------------------------------------
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] julianhyde commented on a change in pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r469741577
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2284,6 +2291,12 @@ static Predicate of(RexNode t) {
}
return IsPredicate.of(t);
}
+
+ /** Returns whether this predicate can while simplifying other OR
+ * operands. */
+ default boolean allowedInOr(RelOptPredicateList predicates) {
+ return true;
Review comment:
good catch. i'll fix.
----------------------------------------------------------------
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] julianhyde commented on a change in pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r474877330
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptPredicateList.java
##########
@@ -180,4 +203,27 @@ public RelOptPredicateList shift(RexBuilder rexBuilder, int offset) {
RexUtil.shift(leftInferredPredicates, offset),
RexUtil.shift(rightInferredPredicates, offset));
}
+
+ /** Returns whether an expression is effectively NOT NULL due to an
+ * {@code e IS NOT NULL} condition in this predicate list. */
+ public boolean isEffectivelyNotNull(RexNode e) {
+ if (!e.getType().isNullable()) {
+ return true;
+ }
+ for (RexNode p : pulledUpPredicates) {
+ if (p.getKind() == SqlKind.IS_NOT_NULL
+ && ((RexCall) p).getOperands().get(0).equals(e)) {
+ return true;
+ }
+ }
+ if (SqlKind.COMPARISON.contains(e.getKind())) {
+ // A comparison with a literal, such as 'ref < 10', is not null if 'ref'
+ // is not null.
+ RexCall call = (RexCall) e;
+ if (call.getOperands().get(1) instanceof RexLiteral) {
+ return isEffectivelyNotNull(call.getOperands().get(0));
Review comment:
I missed this comment. I'll address during CALCITE-4173.
----------------------------------------------------------------
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] julianhyde commented on a change in pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r469741260
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramBuilderBase.java
##########
@@ -306,6 +306,14 @@ protected RexNode gt(RexNode n1, RexNode n2) {
return rexBuilder.makeCall(SqlStdOperatorTable.GREATER_THAN, n1, n2);
}
+ protected RexNode like(RexNode ref, RexNode pattern) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.LIKE, ref, pattern);
+ }
+
+ protected RexNode like(RexNode ref, RexNode pattern, RexNode escape) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.LIKE, ref, pattern, escape);
+ }
+
Review comment:
I disagree. There are lots of surrounding methods with a similar pattern. They are protected methods in a test case. Javadoc would not add anything.
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r469611797
##########
File path: core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml
##########
@@ -3763,7 +3765,7 @@ LogicalProject(DEPTNO=[$7])
</TestCase>
<TestCase name="testReduceConstExpr">
<Resource name="sql">
- <![CDATA[select sum(case when 'y' = 'n' then ename else 1 end) from emp]]>
+ <![CDATA[select sum(case when 'y' = 'n' then ename else 0.1 end) from emp]]>
</Resource>
Review comment:
+1. The test case should be using `1`: https://github.com/apache/calcite/blob/37b8cdbb381369e773229a81ae69ab5c1df34f3e/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java#L2649
----------------------------------------------------------------
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] asfgit closed pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2105:
URL: https://github.com/apache/calcite/pull/2105
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r472066153
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptPredicateList.java
##########
@@ -180,4 +203,27 @@ public RelOptPredicateList shift(RexBuilder rexBuilder, int offset) {
RexUtil.shift(leftInferredPredicates, offset),
RexUtil.shift(rightInferredPredicates, offset));
}
+
+ /** Returns whether an expression is effectively NOT NULL due to an
+ * {@code e IS NOT NULL} condition in this predicate list. */
+ public boolean isEffectivelyNotNull(RexNode e) {
+ if (!e.getType().isNullable()) {
+ return true;
+ }
+ for (RexNode p : pulledUpPredicates) {
+ if (p.getKind() == SqlKind.IS_NOT_NULL
+ && ((RexCall) p).getOperands().get(0).equals(e)) {
+ return true;
+ }
+ }
+ if (SqlKind.COMPARISON.contains(e.getKind())) {
+ // A comparison with a literal, such as 'ref < 10', is not null if 'ref'
+ // is not null.
+ RexCall call = (RexCall) e;
+ if (call.getOperands().get(1) instanceof RexLiteral) {
+ return isEffectivelyNotNull(call.getOperands().get(0));
Review comment:
do we also need to consider cases such as '10 > ref' here?
----------------------------------------------------------------
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 #2105: [CALCITE-4159] Simplify always-true expressions (such as LIKE '%') to TRUE
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2105:
URL: https://github.com/apache/calcite/pull/2105#discussion_r469027988
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramBuilderBase.java
##########
@@ -306,6 +306,14 @@ protected RexNode gt(RexNode n1, RexNode n2) {
return rexBuilder.makeCall(SqlStdOperatorTable.GREATER_THAN, n1, n2);
}
+ protected RexNode like(RexNode ref, RexNode pattern) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.LIKE, ref, pattern);
+ }
+
+ protected RexNode like(RexNode ref, RexNode pattern, RexNode escape) {
+ return rexBuilder.makeCall(SqlStdOperatorTable.LIKE, ref, pattern, escape);
+ }
+
Review comment:
What does this method mean? Could you add some comments?
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2284,6 +2291,12 @@ static Predicate of(RexNode t) {
}
return IsPredicate.of(t);
}
+
+ /** Returns whether this predicate can while simplifying other OR
+ * operands. */
+ default boolean allowedInOr(RelOptPredicateList predicates) {
+ return true;
Review comment:
It seems to lack some words between `can` and `while`.
##########
File path: core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml
##########
@@ -3763,7 +3765,7 @@ LogicalProject(DEPTNO=[$7])
</TestCase>
<TestCase name="testReduceConstExpr">
<Resource name="sql">
- <![CDATA[select sum(case when 'y' = 'n' then ename else 1 end) from emp]]>
+ <![CDATA[select sum(case when 'y' = 'n' then ename else 0.1 end) from emp]]>
</Resource>
Review comment:
Is this change correct?
----------------------------------------------------------------
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