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/06 16:20:03 UTC
[GitHub] [calcite] vlsi opened a new pull request #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
vlsi opened a new pull request #2141:
URL: https://github.com/apache/calcite/pull/2141
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r484732728
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
Agree, we should remove the assertion.
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485401212
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
I'm +1 to Julian's idea.
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485429428
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
> This isn't 'sudden'. For years I've been arguing in favor of expanding SQL IN lists to ORs.
One thing is expanding IN lists to ORs (which I think is totally fine, and Calcite should continue doing it), and a different thing is forbidding creating an IN list call (which is the "new" change introduced by CALCITE-4173).
> This isn't 'drastic'. Most people aren't using this.
I do not know about "most people", I know that my application is using it. In my specific example, even though IN lists are expanded as ORs, under certain circumstances my application re-contracts them again as an IN list. The reason for that is allowing to execute later an IndexScan exploiting some Lucene indices, where the IN list call can be implemented as a Lucene's TermInSetQuery.
Other applications can have other use cases, but I would say that having the possibly to define an IN list call in relational algebra (even though Calcite core does not do it) is something useful that should not be forbidden.
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2141:
URL: https://github.com/apache/calcite/pull/2141
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485321623
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
No, this assertion is needed. We do not support IN in RexCall.
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485392343
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
I address this in the JIRA case. We never officially supported IN in RexCall. We allowed it for limited cases only (e.g. RelToSql).
Calcite algebra isn't a free-for-all, where you can add your own built-in operator and then holler when it is de-supported. If it was, people would end up choking on each other's garbage.
This isn't 'drastic'. Most people aren't using this.
This isn't 'sudden'. For years I've been arguing in favor of expanding SQL IN lists to ORs.
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485429428
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
> This isn't 'sudden'. For years I've been arguing in favor of expanding SQL IN lists to ORs.
One thing is expanding IN lists to ORs (which I think is totally fine, and Calcite should continue doing it), and a different thing is forbidding creating an IN list call (which is the "new" change introduced by CALCITE-4173).
> This isn't 'drastic'. Most people aren't using this.
I do not know about "most people", I know that my application is using it. In my specific example, even though IN lists are expanded as ORs, under certain circumstances my application re-contracts them again as an IN list. The reason for that is allowing to execute later an IndexScan exploiting some Lucene indices, where the IN list call can be implemented as a Lucene's TermInSetQuery.
Other applications can have other use cases, but I would say that having the possibly to define an IN list call in relational algebra is something useful that should not be forbidden.
----------------------------------------------------------------
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 pull request #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
vlsi commented on pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#issuecomment-687890233
@julianhyde , this commit fixes the issues introduced in https://github.com/apache/calcite/commit/850f0f4a04fc2399b8fd1c1fed532cd8e1e39514
Would you please kindly review 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] vlsi commented on a change in pull request #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r484710192
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
Just in case, this assertion was added in `CALCITE-4173`, however, it breaks existing applications that might create their own IN RexCalls.
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485091849
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2690,7 +2694,14 @@ int complexity() {
}
@Override public RelDataType getType() {
- return rexBuilder.typeFactory.leastRestrictive(Util.distinctList(types));
+ List<RelDataType> distinctTypes = Util.distinctList(this.types);
+ assert distinctTypes.size() >= 1 : "at least one type expected";
+ if (distinctTypes.size() == 1) {
+ return distinctTypes.get(0);
Review comment:
It probably makes sense to move this `if size=1` check to the `leastRestrictive` method.
However, I do not know if `null` result is permissible here or if it is a catastrophic failure, so I added an assertion, except I put the relevant clarification 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] rubenada commented on a change in pull request #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485385731
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
@julianhyde IMHO the assertion is a drastic breaking change. As @vlsi says, what happens with existing applications that may create their own IN calls (that then are implemented by their own mechanisms / conventions)? Should this be suddenly unsupported in a minor version upgrade?
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485078224
##########
File path: core/src/main/java/org/apache/calcite/rex/RexSimplify.java
##########
@@ -2690,7 +2694,14 @@ int complexity() {
}
@Override public RelDataType getType() {
- return rexBuilder.typeFactory.leastRestrictive(Util.distinctList(types));
+ List<RelDataType> distinctTypes = Util.distinctList(this.types);
+ assert distinctTypes.size() >= 1 : "at least one type expected";
+ if (distinctTypes.size() == 1) {
+ return distinctTypes.get(0);
Review comment:
Not necessarily subject of this PR but couldn't this logic be inside `leastRestrictive`? If there is only one type then it is also the least restrictive so putting it inside we don't break anything in terms of API. Moreover, if we want to force users to pass more than one type then it would be better to check this via regular exceptions (e.g., `IllegalArgumentException`) instead of assertions that can even be disabled.
----------------------------------------------------------------
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 #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2141:
URL: https://github.com/apache/calcite/pull/2141#discussion_r485832224
##########
File path: core/src/main/java/org/apache/calcite/rex/RexCall.java
##########
@@ -80,7 +80,6 @@ protected RexCall(
this.nodeCount = RexUtil.nodeCount(1, this.operands);
assert op.getKind() != null : op;
assert op.validRexOperands(operands.size(), Litmus.THROW) : this;
- assert op.kind != SqlKind.IN || this instanceof RexSubQuery;
Review comment:
> having the possibly to define an IN list call in relational algebra (even though Calcite core does not do it) is something useful that should not be forbidden.
I agree. This feature is precisely that facility.
----------------------------------------------------------------
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