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/09/09 07:32:14 UTC
[GitHub] [calcite] julianhyde opened a new pull request #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
julianhyde opened a new pull request #2144:
URL: https://github.com/apache/calcite/pull/2144
----------------------------------------------------------------
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 #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2144:
URL: https://github.com/apache/calcite/pull/2144#discussion_r485806842
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2690,7 +2691,14 @@ int complexity() {
}
@Override public RelDataType getType() {
- return rexBuilder.typeFactory.leastRestrictive(Util.distinctList(types));
+ if (this.types.isEmpty()) {
+ // Expression is "x IS NULL"
+ return ref.getType();
Review comment:
I agree it's a bit strange. It's tricky because the assignment rules for the left-hand type are different from the right-hand types.
The type assignment rules are complicated (e.g. when you compare a REAL to a DECIMAL, do you convert the REAL to a DECIMAL or vice versa?; and hundred other things. I decided to not spend effort addressing it at this point. If you can find a SQL example where it matters, log a bug.
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2690,7 +2691,14 @@ int complexity() {
}
@Override public RelDataType getType() {
- return rexBuilder.typeFactory.leastRestrictive(Util.distinctList(types));
+ if (this.types.isEmpty()) {
+ // Expression is "x IS NULL"
+ return ref.getType();
Review comment:
I agree it's a bit strange. It's tricky because the assignment rules for the left-hand type are different from the right-hand types.
The type assignment rules are complicated (e.g. when you compare a REAL to a DECIMAL, do you convert the REAL to a DECIMAL or vice versa?; and hundred other things). I decided to not spend effort addressing it at this point. If you can find a SQL example where it matters, log a bug.
----------------------------------------------------------------
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 #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2144:
URL: https://github.com/apache/calcite/pull/2144#discussion_r485468654
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2690,7 +2691,14 @@ int complexity() {
}
@Override public RelDataType getType() {
- return rexBuilder.typeFactory.leastRestrictive(Util.distinctList(types));
+ if (this.types.isEmpty()) {
+ // Expression is "x IS NULL"
+ return ref.getType();
Review comment:
It looks strange that `ref.getType()` is considered only in the case of `null-only` sarg.
Should it be included to `this.types` in the constructor instead, so the types are handled in a uniform way?
----------------------------------------------------------------
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 #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2144:
URL: https://github.com/apache/calcite/pull/2144#discussion_r485439222
##########
File path: plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java
##########
@@ -367,15 +366,13 @@ protected void foo(CalciteAssert.AssertThat with, String tableName,
builder.equals(builder.field("CD_EDUCATION_STATUS"),
builder.literal("HIGH SCHOOL")),
builder.equals(builder.field("D_YEAR"), builder.literal(1998)),
- builder.call(SqlStdOperatorTable.IN,
- builder.field("S_STATE"),
Review comment:
This is a breaking change in a minor release.
----------------------------------------------------------------
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] danny0405 commented on a change in pull request #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2144:
URL: https://github.com/apache/calcite/pull/2144#discussion_r485404856
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -1640,6 +1640,21 @@ private void checkExponentialCnf(int n) {
.expandedSearch(expanded);
}
+ @Test void testSimplifyRange6() {
+ // An IS NULL condition would not usually become a Sarg,
+ // but here it is combined with another condition, and together they cross
+ // the complexity threshold.
+ final RexNode aRef = input(tInt(true), 0);
+ final RexNode bRef = input(tInt(true), 1);
+ // a in (1, 2) or b is null
+ RexNode expr = or(eq(aRef, literal(1)), eq(aRef, literal(2)), isNull(bRef));
+ final String simplified =
+ "OR(SEARCH($1, Sarg[, null]), SEARCH($0, Sarg[1, 2]))";
+ final String expanded = "OR(IS NULL($1), OR(=($0, 1), =($0, 2)))";
+ checkSimplify(expr, simplified)
Review comment:
Can `Sarg[, null]` be promoted to `Sarg[null]` ?
----------------------------------------------------------------
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 #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2144:
URL: https://github.com/apache/calcite/pull/2144#discussion_r485407995
##########
File path: core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
##########
@@ -1640,6 +1640,21 @@ private void checkExponentialCnf(int n) {
.expandedSearch(expanded);
}
+ @Test void testSimplifyRange6() {
+ // An IS NULL condition would not usually become a Sarg,
+ // but here it is combined with another condition, and together they cross
+ // the complexity threshold.
+ final RexNode aRef = input(tInt(true), 0);
+ final RexNode bRef = input(tInt(true), 1);
+ // a in (1, 2) or b is null
+ RexNode expr = or(eq(aRef, literal(1)), eq(aRef, literal(2)), isNull(bRef));
+ final String simplified =
+ "OR(SEARCH($1, Sarg[, null]), SEARCH($0, Sarg[1, 2]))";
+ final String expanded = "OR(IS NULL($1), OR(=($0, 1), =($0, 2)))";
+ checkSimplify(expr, simplified)
Review comment:
NULL isn't a value on the line -infinity to infinity, so it cannot be part of the range-set.
Ideally we wouldn't create a Sarg with an empty range-set that allows NULL. It happened in this case because the complexity threshold isn't perfect (maybe we'll improve that later). We will always want to allow `Sarg[, null]` - it might occur if you intersect `Sarg[ < 2, null]` with `Sarg[ > 10, null]` - but hopefully we won't use it very often.
----------------------------------------------------------------
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 #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2144:
URL: https://github.com/apache/calcite/pull/2144#discussion_r485449641
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2690,7 +2691,14 @@ int complexity() {
}
@Override public RelDataType getType() {
- return rexBuilder.typeFactory.leastRestrictive(Util.distinctList(types));
+ if (this.types.isEmpty()) {
+ // Expression is "x IS NULL"
+ return ref.getType();
+ }
+ final List<RelDataType> distinctTypes = Util.distinctList(this.types);
Review comment:
I wonder if this can be integrated into `addRange`, so it skips adding duplicate items in the first place rather than add duplicates and de-duplicate it later.
----------------------------------------------------------------
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 merged pull request #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #2144:
URL: https://github.com/apache/calcite/pull/2144
----------------------------------------------------------------
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] Dominick1993 commented on a change in pull request #2144: [CALCITE-4237] Sarg with single IS NULL operand causes AssertionError
Posted by GitBox <gi...@apache.org>.
Dominick1993 commented on a change in pull request #2144:
URL: https://github.com/apache/calcite/pull/2144#discussion_r521284926
##########
File path: plus/src/test/java/org/apache/calcite/adapter/tpcds/TpcdsTest.java
##########
@@ -367,15 +366,13 @@ protected void foo(CalciteAssert.AssertThat with, String tableName,
builder.equals(builder.field("CD_EDUCATION_STATUS"),
builder.literal("HIGH SCHOOL")),
builder.equals(builder.field("D_YEAR"), builder.literal(1998)),
- builder.call(SqlStdOperatorTable.IN,
- builder.field("S_STATE"),
Review comment:
Hi, I stumbled upon this breaking change as well and unfortunately it is not categorized as breaking change in [Calcite release notes](https://calcite.apache.org/docs/history.html#v1-26-0).
----------------------------------------------------------------
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