You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "rubenada (via GitHub)" <gi...@apache.org> on 2023/06/05 11:38:33 UTC

[GitHub] [calcite] rubenada opened a new pull request, #3243: [CALCITE-5401] Rule fired by HepPlanner can return Volcano's RelSubset

rubenada opened a new pull request, #3243:
URL: https://github.com/apache/calcite/pull/3243

   See problem description in:
   [CALCITE-5401](https://issues.apache.org/jira/browse/CALCITE-5401) Rule fired by HepPlanner can return Volcano's RelSubset.
   
   Issue solved by  adding new `RelOptRule#convert` with an additional planner parameter, and use it in potentially sensitive locations for this issue to occur (basically rules' `onMatch`)


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] zabetak commented on a diff in pull request #3243: [CALCITE-5401] Rule fired by HepPlanner can return Volcano's RelSubset

Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3243:
URL: https://github.com/apache/calcite/pull/3243#discussion_r1224080318


##########
core/src/main/java/org/apache/calcite/plan/RelOptRule.java:
##########
@@ -614,7 +616,10 @@ public static RelNode convert(RelNode rel, RelTraitSet toTraits) {
    * @return a relational expression with the desired trait; never null
    */
   public static RelNode convert(RelNode rel, @Nullable RelTrait toTrait) {

Review Comment:
   OK sounds good. Thanks for clarifying.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada commented on a diff in pull request #3243: [CALCITE-5401] Rule fired by HepPlanner can return Volcano's RelSubset

Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3243:
URL: https://github.com/apache/calcite/pull/3243#discussion_r1223280874


##########
core/src/main/java/org/apache/calcite/plan/RelOptRule.java:
##########
@@ -614,7 +616,10 @@ public static RelNode convert(RelNode rel, RelTraitSet toTraits) {
    * @return a relational expression with the desired trait; never null
    */
   public static RelNode convert(RelNode rel, @Nullable RelTrait toTrait) {

Review Comment:
   Thanks for the feedback @zabetak .
   At this stage we cannot do that, because the "old" `convert` method is still used by *many* converter rules inside their `public RelNode convert(RelNode rel)` implementations. The problem is that, inside that method, we do not have the `RelOptRuleCall call` to get the planner (it is not passed by `ConverterRule#onMatch` into `#convert`, so in order deprecate this method, we would need a huge refactoring on ConverterRule and all the implementations (which would also impact the downstream ones)... all to fix this rare issue.
   On top of that, this problem ("Rule fired by HepPlanner can return Volcano's RelSubset") should normally never occur inside a ConverterRule, because HepPlanner does not care about Conventions, so normally Converter rules would not be applied in there, would they?
   All things considered, I think we should correct the problem where it can reasonably happen (Calcite's non-converter rules onMatch implems), going beyond that seems too much disruption for a very, very unlikely problem. Wdyt?



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3243: [CALCITE-5401] Rule fired by HepPlanner can return Volcano's RelSubset

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3243:
URL: https://github.com/apache/calcite/pull/3243#issuecomment-1576652969

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3243)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3243&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3243&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3243&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3243&resolved=false&types=CODE_SMELL)
   
   [![92.9%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90-16px.png '92.9%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3243&metric=new_coverage&view=list) [92.9% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3243&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3243&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3243&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada merged pull request #3243: [CALCITE-5401] Rule fired by HepPlanner can return Volcano's RelSubset

Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada merged PR #3243:
URL: https://github.com/apache/calcite/pull/3243


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] zabetak commented on a diff in pull request #3243: [CALCITE-5401] Rule fired by HepPlanner can return Volcano's RelSubset

Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3243:
URL: https://github.com/apache/calcite/pull/3243#discussion_r1222905230


##########
core/src/main/java/org/apache/calcite/plan/RelOptRule.java:
##########
@@ -614,7 +616,10 @@ public static RelNode convert(RelNode rel, RelTraitSet toTraits) {
    * @return a relational expression with the desired trait; never null
    */
   public static RelNode convert(RelNode rel, @Nullable RelTrait toTrait) {

Review Comment:
   Should we deprecate the old methods so that we avoid the problem from re-appearing?



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org