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/05/20 14:51:17 UTC
[GitHub] [calcite] hsyuan opened a new pull request #1986: [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule
hsyuan opened a new pull request #1986:
URL: https://github.com/apache/calcite/pull/1986
CALCITE-4003
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r429032954
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
Good point, we can use annotations instead.
But logging is not what I want, I just want to add a debug point, check the returned subset for a specific rule match. Logging will print a lot of subsets for all the rule matches that I am not interested in.
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r428807839
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
Saying this just because the comment says this assert is for debugging, so I assumed some context will be more useful for people who don't have know this much.
----------------------------------------------------------------
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 #1986: [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r428905232
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
I find it confusing to use an assertion just to silence some warnings. Usually there are annotations for this kind of things. Apart from that if the content is useful for debugging then why not logging it for real.
----------------------------------------------------------------
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] mikasaakeman removed a comment on pull request #1986: [CALCITE-4003] Disallow cross convention matching in TransformationRule
Posted by GitBox <gi...@apache.org>.
mikasaakeman removed a comment on pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#issuecomment-709021586
after udpate, this exception happend: java.lang.RuntimeException: rel#3291:EnumerableHashJoin.ENUMERABLE.[](left=RelSubset#88,right=EnumerableNestedLoopJoin#3290,condition==($0, $3),joinType=inner) is a PhysicalNode, which is not allowed in JoinAssociateRule
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r428807293
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
Nit: why not use `checkArgument` or something similar from Guava? Such that it can both check expected result, stop execute when not satisfying, and provide detailed description.
----------------------------------------------------------------
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] mikasaakeman commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching in TransformationRule
Posted by GitBox <gi...@apache.org>.
mikasaakeman commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r505380316
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
after udpate, this exception happend: java.lang.RuntimeException: rel#3291:EnumerableHashJoin.ENUMERABLE.[](left=RelSubset#88,right=EnumerableNestedLoopJoin#3290,condition==($0, $3),joinType=inner) is a PhysicalNode, which is not allowed in JoinAssociateRule
----------------------------------------------------------------
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] mikasaakeman commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching in TransformationRule
Posted by GitBox <gi...@apache.org>.
mikasaakeman commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r505380316
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
after udpate, this exception happend, this exception should not break OPTIMIZE process: java.lang.RuntimeException: rel#3291:EnumerableHashJoin.ENUMERABLE.[](left=RelSubset#88,right=EnumerableNestedLoopJoin#3290,condition==($0, $3),joinType=inner) is a PhysicalNode, which is not allowed in JoinAssociateRule
----------------------------------------------------------------
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] amaliujia commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r428824274
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
Gotcha. Now I see it is to suppress IDE, style checker, etc.
Thanks for clarification.
----------------------------------------------------------------
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] mikasaakeman commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching in TransformationRule
Posted by GitBox <gi...@apache.org>.
mikasaakeman commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r505380316
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
after udpate, this exception happend, this exception should not break OPTIMIZE process, JoinAssociateRule enable match EnumerableNestedLoopJoin. java.lang.RuntimeException: rel#3291:EnumerableHashJoin.ENUMERABLE.[](left=RelSubset#88,right=EnumerableNestedLoopJoin#3290,condition==($0, $3),joinType=inner) is a PhysicalNode, which is not allowed in JoinAssociateRule
----------------------------------------------------------------
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] hsyuan commented on a change in pull request #1986: [CALCITE-4003] Disallow cross convention matching and generation in TransformationRule
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#discussion_r428808638
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -136,7 +155,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
volcanoPlanner.ensureRegistered(
entry.getKey(), entry.getValue());
}
- volcanoPlanner.ensureRegistered(rel, rels[0]);
+ RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
+ // The subset is not used, but we need it, just for debugging
+ assert subset != null;
Review comment:
It is not intended to check null or not, just to silence the IDE that the 'subset' is used. But in fact it is useful to add a breakpoint there to see the content of subset.
----------------------------------------------------------------
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] mikasaakeman commented on pull request #1986: [CALCITE-4003] Disallow cross convention matching in TransformationRule
Posted by GitBox <gi...@apache.org>.
mikasaakeman commented on pull request #1986:
URL: https://github.com/apache/calcite/pull/1986#issuecomment-709021586
after udpate, this exception happend: java.lang.RuntimeException: rel#3291:EnumerableHashJoin.ENUMERABLE.[](left=RelSubset#88,right=EnumerableNestedLoopJoin#3290,condition==($0, $3),joinType=inner) is a PhysicalNode, which is not allowed in JoinAssociateRule
----------------------------------------------------------------
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] hsyuan merged pull request #1986: [CALCITE-4003] Disallow cross convention matching in TransformationRule
Posted by GitBox <gi...@apache.org>.
hsyuan merged pull request #1986:
URL: https://github.com/apache/calcite/pull/1986
----------------------------------------------------------------
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