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

["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