[GitHub] [calcite] rubenada commented on a change in pull request #2141: [CALCITE-4173] Fix assertion error when RexSimplify generates Sarg with single null only
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: us...@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
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: us...@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
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: us...@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
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: us...@infra.apache.org
