[ https://issues.apache.org/jira/browse/CALCITE-5503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17681481#comment-17681481 ]
Julian Hyde commented on CALCITE-5503: -------------------------------------- [~mosche], I've assigned to you, as requested. The PR looks good. Two changes needed: First, change the case summary (and commit message) to describe the problem (not reusing nodes in a DAG plan) rather than the solution (memoization). Second, add a SQL test that shows the improved plan. The `!plan` command in a .iq file should do it. > Memoize visited nodes in CheapestPlanReplacer > --------------------------------------------- > > Key: CALCITE-5503 > URL: https://issues.apache.org/jira/browse/CALCITE-5503 > Project: Calcite > Issue Type: Improvement > Components: core > Reporter: Moritz Mack > Assignee: Moritz Mack > Priority: Minor > Labels: pull-request-available > Time Spent: 20m > Remaining Estimate: 0h > > When using CheapestPlanReplacer, semantics of a RelNode tree change if it > contains the same node multiple times in case this node has inputs itself > that have to be replaced. During replacement such nodes get copied on each > occurrence. > Instead CheapestPlanReplacer should memoize previously visited nodes and emit > the same result again in the that case. > The test case below illustrates the issue and fails on the current main > branch. > > {code:java} > @Test void testMemoizeInputRelNodes() { > VolcanoPlanner planner = new VolcanoPlanner(); > planner.addRelTraitDef(ConventionTraitDef.INSTANCE); > RelOptCluster cluster = newCluster(planner); > // The rule that triggers the assert rule > planner.addRule(PhysLeafRule.INSTANCE); > planner.addRule(GoodSingleRule.INSTANCE); > // Leaf RelNode > NoneLeafRel leafRel = new NoneLeafRel(cluster, "a"); > RelNode leafPhy = planner > .changeTraits(leafRel, cluster.traitSetOf(PHYS_CALLING_CONVENTION)); > // RelNode with leaf RelNode as single input > NoneSingleRel singleRel = new NoneSingleRel(cluster, leafPhy); > RelNode singlePhy = planner > .changeTraits(singleRel, cluster.traitSetOf(PHYS_CALLING_CONVENTION)); > // Binary RelNode with identical input on either side > PhysBiRel parent = new PhysBiRel( > cluster, cluster.traitSetOf(PHYS_CALLING_CONVENTION), singlePhy, > singlePhy); > planner.setRoot(parent); > RelNode result = planner.chooseDelegate().findBestExp(); > // Expect inputs to remain identical > assertEquals(result.getInput(0), result.getInput(1)); > } {code} > -- This message was sent by Atlassian Jira (v8.20.10#820010)