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 2021/05/25 12:52:47 UTC
[GitHub] [calcite] rubenada opened a new pull request #2418: [CALCITE-4621] SemiJoinRule throws AssertionError on ANTI join
rubenada opened a new pull request #2418:
URL: https://github.com/apache/calcite/pull/2418
Jira: [CALCITE-4621](https://issues.apache.org/jira/browse/CALCITE-4621)
--
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 #2418: [CALCITE-4621] SemiJoinRule throws AssertionError on ANTI join
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2418:
URL: https://github.com/apache/calcite/pull/2418#discussion_r639810446
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java
##########
@@ -45,8 +46,9 @@
public abstract class SemiJoinRule
extends RelRule<SemiJoinRule.Config>
implements TransformationRule {
- private static boolean notGenerateNullsOnLeft(Join join) {
- return !join.getJoinType().generatesNullsOnLeft();
+ private static boolean isJoinTypeSupported(Join join) {
+ final JoinRelType type = join.getJoinType();
+ return type != JoinRelType.RIGHT && type != JoinRelType.FULL && type != JoinRelType.ANTI;
Review comment:
I guess you mean [CALCITE-2969](https://issues.apache.org/jira/browse/CALCITE-2969) (yes, it seems there was an error and the commit message of [4809393](https://github.com/apache/calcite/commit/48093937ae4db179884d1111fa9d12e978e57e1f) actually referenced a wrong Jira :( )
I'll create a new Jira to open this discussion.
--
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 #2418: [CALCITE-4621] SemiJoinRule throws AssertionError on ANTI join
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2418:
URL: https://github.com/apache/calcite/pull/2418#discussion_r639730115
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java
##########
@@ -45,8 +46,9 @@
public abstract class SemiJoinRule
extends RelRule<SemiJoinRule.Config>
implements TransformationRule {
- private static boolean notGenerateNullsOnLeft(Join join) {
- return !join.getJoinType().generatesNullsOnLeft();
+ private static boolean isJoinTypeSupported(Join join) {
+ final JoinRelType type = join.getJoinType();
+ return type != JoinRelType.RIGHT && type != JoinRelType.FULL && type != JoinRelType.ANTI;
Review comment:
The code could be slightly more readable (given the current name of the method) if you formulated the condition using `==` instead of `!=` operator. The equivalent of the current code would be:
`return type == JoinRelType.INNER || type == JoinRelType.LEFT || type == JoinRelType.SEMI`
Moreover, I am not sure if it is intended that you consider `SEMI` as supported join type. I was thinking that it doesn't make sense to use the rule if the join is already `SEMI` join.
--
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 #2418: [CALCITE-4621] SemiJoinRule throws AssertionError on ANTI join
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2418:
URL: https://github.com/apache/calcite/pull/2418#discussion_r639764325
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java
##########
@@ -45,8 +46,9 @@
public abstract class SemiJoinRule
extends RelRule<SemiJoinRule.Config>
implements TransformationRule {
- private static boolean notGenerateNullsOnLeft(Join join) {
- return !join.getJoinType().generatesNullsOnLeft();
+ private static boolean isJoinTypeSupported(Join join) {
+ final JoinRelType type = join.getJoinType();
+ return type != JoinRelType.RIGHT && type != JoinRelType.FULL && type != JoinRelType.ANTI;
Review comment:
Ok, I can re-write the code using `==`.
Regarding `SEMI`, I agree it is a bit bizarre that this rule processes this type, it may be useful to transform plans with SemiJoin + Aggregate => SemiJoin (not sure, I haven't looked in detail this scenario).
In any case, this is out of the scope of the current patch. We can create a separate Jira to study this situation and eventually considering removing `SEMI` from this rule.
--
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 merged pull request #2418: [CALCITE-4621] SemiJoinRule throws AssertionError on ANTI join
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #2418:
URL: https://github.com/apache/calcite/pull/2418
--
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 #2418: [CALCITE-4621] SemiJoinRule throws AssertionError on ANTI join
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2418:
URL: https://github.com/apache/calcite/pull/2418#discussion_r639788794
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java
##########
@@ -45,8 +46,9 @@
public abstract class SemiJoinRule
extends RelRule<SemiJoinRule.Config>
implements TransformationRule {
- private static boolean notGenerateNullsOnLeft(Join join) {
- return !join.getJoinType().generatesNullsOnLeft();
+ private static boolean isJoinTypeSupported(Join join) {
+ final JoinRelType type = join.getJoinType();
+ return type != JoinRelType.RIGHT && type != JoinRelType.FULL && type != JoinRelType.ANTI;
Review comment:
I checked a bit the history and it seems that before CALCITE-2696 the rule didn't match `SEMI` joins which kind of makes sense to me. Anyways its true that we can follow up the discussion in another JIRA.
--
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 #2418: [CALCITE-4621] SemiJoinRule throws AssertionError on ANTI join
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2418:
URL: https://github.com/apache/calcite/pull/2418#discussion_r639810446
##########
File path: core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java
##########
@@ -45,8 +46,9 @@
public abstract class SemiJoinRule
extends RelRule<SemiJoinRule.Config>
implements TransformationRule {
- private static boolean notGenerateNullsOnLeft(Join join) {
- return !join.getJoinType().generatesNullsOnLeft();
+ private static boolean isJoinTypeSupported(Join join) {
+ final JoinRelType type = join.getJoinType();
+ return type != JoinRelType.RIGHT && type != JoinRelType.FULL && type != JoinRelType.ANTI;
Review comment:
I guess you mean [CALCITE-2969](https://issues.apache.org/jira/browse/CALCITE-2969) (yes, it seems there was an error and the commit message of [4809393](https://github.com/apache/calcite/commit/48093937ae4db179884d1111fa9d12e978e57e1f) actually referenced a wrong Jira :( )
I'll create a new Jira to open this discussion. UPDATE: done -> [CALCITE-4623](https://issues.apache.org/jira/browse/CALCITE-4623)
--
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