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/07 05:24:29 UTC
[GitHub] [calcite] liyafan82 opened a new pull request #2098: [CALCITE-4155] Simplify IN expression of discrete values
liyafan82 opened a new pull request #2098:
URL: https://github.com/apache/calcite/pull/2098
For example, given expressions like
a in (1, 2, 3, 4, 5)
or
a = 1 or a = 2 or a = 3 or a = 4 or a = 5,
we can simplify it to
a >= 1 and a <= 5
Such simplification reduces the number of value comparisons from 5 to 2.
----------------------------------------------------------------
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 #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2098:
URL: https://github.com/apache/calcite/pull/2098#discussion_r468445089
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -2634,6 +2634,127 @@ private static String getString(ImmutableMap<RexNode, RexNode> map) {
assertThat(simplifiedExpr, is(origExpr));
}
+ @Test void testSimplifyInValues() {
+ RexNode a = vIntNotNull(1);
+ RexNode literal1 = rexBuilder.makeLiteral(1, a.getType(), true);
+ RexNode literal2 = rexBuilder.makeLiteral(2, a.getType(), true);
+ RexNode literal3 = rexBuilder.makeLiteral(3, a.getType(), true);
+ RexNode literal4 = rexBuilder.makeLiteral(4, a.getType(), true);
+ RexNode literal5 = rexBuilder.makeLiteral(5, a.getType(), true);
+ RexNode literal6 = rexBuilder.makeLiteral(6, a.getType(), true);
+
+ // expr: a in (1, 2, 3, 4, 5)
+ RexNode expr = rexBuilder.makeCall(
+ SqlStdOperatorTable.IN, a, literal1, literal2, literal3, literal4, literal5);
+ // expected: a >= 1 and a <= 5
+ RexNode expected = rexBuilder.makeCall(SqlStdOperatorTable.AND,
+ rexBuilder.makeCall(SqlStdOperatorTable.GREATER_THAN_OR_EQUAL, a, literal1),
+ rexBuilder.makeCall(SqlStdOperatorTable.LESS_THAN_OR_EQUAL, a, literal5));
Review comment:
@vlsi Revised accordingly, and the tests look much clearer. Thanks a lot for your good suggestion.
----------------------------------------------------------------
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 closed pull request #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
liyafan82 closed pull request #2098:
URL: https://github.com/apache/calcite/pull/2098
----------------------------------------------------------------
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 #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2098:
URL: https://github.com/apache/calcite/pull/2098#discussion_r468519527
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -2634,6 +2634,86 @@ private static String getString(ImmutableMap<RexNode, RexNode> map) {
assertThat(simplifiedExpr, is(origExpr));
}
+ @Test void testSimplifyInValues() {
+ RexNode a = vIntNotNull(1);
+ RexNode literal1 = literal(1);
+ RexNode literal2 = literal(2);
+ RexNode literal3 = literal(3);
+ RexNode literal4 = literal(4);
+ RexNode literal5 = literal(5);
+ RexNode literal6 = literal(6);
+
+ // expr: a in (1, 2, 3, 4, 5)
+ RexNode expr = in(a, literal1, literal2, literal3, literal4, literal5);
+ // expected: a >= 1 and a <= 5
+ RexNode expected = and(ge(a, literal1), le(a, literal5));
+ RexNode simplifiedExpr = simplify.simplifyUnknownAs(expr, RexUnknownAs.UNKNOWN);
+ assertThat(simplifiedExpr, is(expected));
Review comment:
Thanks for the good suggestion, and thanks for the explanation.
I have revised the PR accordingly.
----------------------------------------------------------------
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 #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2098:
URL: https://github.com/apache/calcite/pull/2098#discussion_r468519845
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -2634,6 +2634,86 @@ private static String getString(ImmutableMap<RexNode, RexNode> map) {
assertThat(simplifiedExpr, is(origExpr));
}
+ @Test void testSimplifyInValues() {
+ RexNode a = vIntNotNull(1);
+ RexNode literal1 = literal(1);
+ RexNode literal2 = literal(2);
+ RexNode literal3 = literal(3);
+ RexNode literal4 = literal(4);
+ RexNode literal5 = literal(5);
+ RexNode literal6 = literal(6);
+
+ // expr: a in (1, 2, 3, 4, 5)
+ RexNode expr = in(a, literal1, literal2, literal3, literal4, literal5);
Review comment:
Agreed. Thank you for the good suggestion.
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2098:
URL: https://github.com/apache/calcite/pull/2098#discussion_r468122162
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -2634,6 +2634,127 @@ private static String getString(ImmutableMap<RexNode, RexNode> map) {
assertThat(simplifiedExpr, is(origExpr));
}
+ @Test void testSimplifyInValues() {
+ RexNode a = vIntNotNull(1);
+ RexNode literal1 = rexBuilder.makeLiteral(1, a.getType(), true);
+ RexNode literal2 = rexBuilder.makeLiteral(2, a.getType(), true);
+ RexNode literal3 = rexBuilder.makeLiteral(3, a.getType(), true);
+ RexNode literal4 = rexBuilder.makeLiteral(4, a.getType(), true);
+ RexNode literal5 = rexBuilder.makeLiteral(5, a.getType(), true);
+ RexNode literal6 = rexBuilder.makeLiteral(6, a.getType(), true);
+
+ // expr: a in (1, 2, 3, 4, 5)
+ RexNode expr = rexBuilder.makeCall(
+ SqlStdOperatorTable.IN, a, literal1, literal2, literal3, literal4, literal5);
+ // expected: a >= 1 and a <= 5
+ RexNode expected = rexBuilder.makeCall(SqlStdOperatorTable.AND,
+ rexBuilder.makeCall(SqlStdOperatorTable.GREATER_THAN_OR_EQUAL, a, literal1),
+ rexBuilder.makeCall(SqlStdOperatorTable.LESS_THAN_OR_EQUAL, a, literal5));
Review comment:
Please use `RexProgramTestBase` DSL when writing tests (`and`, `or` and similar methods instead of `makeCall` and similar `rexBuilder`)
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2098:
URL: https://github.com/apache/calcite/pull/2098#discussion_r468449479
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -2634,6 +2634,86 @@ private static String getString(ImmutableMap<RexNode, RexNode> map) {
assertThat(simplifiedExpr, is(origExpr));
}
+ @Test void testSimplifyInValues() {
+ RexNode a = vIntNotNull(1);
+ RexNode literal1 = literal(1);
+ RexNode literal2 = literal(2);
+ RexNode literal3 = literal(3);
+ RexNode literal4 = literal(4);
+ RexNode literal5 = literal(5);
+ RexNode literal6 = literal(6);
+
+ // expr: a in (1, 2, 3, 4, 5)
+ RexNode expr = in(a, literal1, literal2, literal3, literal4, literal5);
+ // expected: a >= 1 and a <= 5
+ RexNode expected = and(ge(a, literal1), le(a, literal5));
+ RexNode simplifiedExpr = simplify.simplifyUnknownAs(expr, RexUnknownAs.UNKNOWN);
+ assertThat(simplifiedExpr, is(expected));
Review comment:
Please use `checkSimplify(...)` functions rather than simple `assertThat`.
The issue with `assertThat` is produces hard to understand error, because it fails to include the original (before simplify) expression to the message.
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2098:
URL: https://github.com/apache/calcite/pull/2098#discussion_r468449904
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -2634,6 +2634,86 @@ private static String getString(ImmutableMap<RexNode, RexNode> map) {
assertThat(simplifiedExpr, is(origExpr));
}
+ @Test void testSimplifyInValues() {
+ RexNode a = vIntNotNull(1);
+ RexNode literal1 = literal(1);
+ RexNode literal2 = literal(2);
+ RexNode literal3 = literal(3);
+ RexNode literal4 = literal(4);
+ RexNode literal5 = literal(5);
+ RexNode literal6 = literal(6);
+
+ // expr: a in (1, 2, 3, 4, 5)
+ RexNode expr = in(a, literal1, literal2, literal3, literal4, literal5);
Review comment:
It might be ok, however, I would use `n1`, `n2`, ... `n5` naming to avoid repeating `literal` again and again.
----------------------------------------------------------------
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 pull request #2098: [CALCITE-4155][WIP] Simplify IN expression of discrete values
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #2098:
URL: https://github.com/apache/calcite/pull/2098#issuecomment-693126829
I am closing this, as the patch should be based on the new Sarg framework.
Thank you for reviewing it @vlsi
----------------------------------------------------------------
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