Hi Julian, Vladimir I remove the equiv rel( *mapRel2Subset.remove(rel)*) in VolcanoPlanner#rename, https://github.com/apache/calcite/pull/2290/files#diff-008c6d52bfd93bbe963a23c264bc412c68cac3b4837e3f10b8d5e4858cd4acb8 :
When I run the test again, it throws NPE in RuleQueue#skipMatch, in method RuleQueue#checkDuplicateSubsets, we want a non-null subset: "*final RelSubset subset = planner.getSubsetNonNull(rels[operand.ordinalInRule]);*" Cuz we remove the rel from mapRel2Subset, so it failed here, here's my questions. 1. How about I use getSubset in checkDuplicateSubsets, if it is null, just breakdown, you can take a look at this PR: https://github.com/apache/calcite/pull/2290/files. 2. Seems we will skip the match in multiple places(RuleQueue#skipMatch, VolcanoRuleCall#onMatch), I found some work are duplicate like we all judge the prunedNodes(0 importance), the logic seems not cohesive. Regards! Aron Tao Julian Hyde <[email protected]> 于2020年12月4日周五 上午11:49写道： > I think Vladimir is very likely correct that the performance > regression was due to algorithmic changes, that getRelList should not > be used, and that improving its performance is not solving the real > problem. So, I will not merge a fix for getRelList performance until > after the algorithmic problem has been resolved. > > On Thu, Dec 3, 2020 at 6:54 PM JiaTao Tao <[email protected]> wrote: > > > > Hi Julian > > > > Thanks, Julian, your advice is really helpful! Do you mind I pull a > request > > to merge your code so that we can improve the performance very quickly? > > About " stored the list of RelNodes ", do you concern about the duplicate > > "relnode" store in "relsubset"? IMO, what we store is a reference, seems > OK. > > > > Regards! > > > > Aron Tao > > > > > > Vladimir Sitnikov <[email protected]> 于2020年12月4日周五 上午5:13写道： > > > > The improvement to RelTraitSet.satisfies would be nice, however, I still > > believe getRelList must not be used in 2222. > > > > It looks like the only possibility of the case when rel points to a > relset > > which does not include the rel is the case when the rel > > has been replaced with an equivalent. > > > > In other words, the planner finds that the rel is equivalent to another > > rel, and it removes the new one from its set. > > Should we remove the entry from mapRel2Subset as well? > > Then RuleCall could treat "missing entry in mapRel2Subset" as a sign of > the > > stale rel. > > > > Vladimir >
