[
https://issues.apache.org/jira/browse/CALCITE-5503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17690927#comment-17690927
]
Julian Hyde commented on CALCITE-5503:
--------------------------------------
[~libenchao], [~mosche] Several things wrong with the commit message,
{quote}
Address issue in CheapestPlanReplacer where repeated nodes in a DAG plan are
not reused.
{quote}
* It ends in a '.'.
* The word 'fix' in a commit message is a red flag. And 'Address issue in' is a
verbose way of saying 'fix'. Rewrite the sentence of make CheapestPlanReplacer
the subject, rather than the repeated nodes.
* Passive voice ("are not reused") makes a sentence longer an less informative
than active voice, "CheapestPlanReplacer does not reuse repeated nodes in a DAG
plan"
* If there's any ambiguity about which is the desired behavior (reusing nodes
or not reusing nodes), the word "should" can clarify. "CheapestPlanReplacer
should reuse repeated nodes in a DAG plan"
* If the commit fixes a bug (which this does), make sure the bug has a good
summary, and use the summary. This bug had a pretty good summary but the commit
message didn't use it.
I force-pushed to improve the message. Now fixed in
[3427c201|https://github.com/apache/calcite/commit/3427c2016be2f4dbb3d74c65a81dcb0099b258cb].
> Repeated nodes in a DAG plan are not reused 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
> Fix For: 1.34.0
>
> Time Spent: 1h 10m
> 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)
