Laurent, Michael had started reviewing this and had some objections. Please don’t summarily commit.
Julian > On Feb 27, 2019, at 1:07 PM, [email protected] wrote: > > This is an automated email from the ASF dual-hosted git repository. > > laurent pushed a commit to branch master > in repository https://gitbox.apache.org/repos/asf/calcite.git > > > The following commit(s) were added to refs/heads/master by this push: > new e3a319e [CALCITE-2827] Allow Convention.NONE planning with > VolcanoPlanner > e3a319e is described below > > commit e3a319e1f79bc807327bd443433c50c4bbf20866 > Author: Juhwan Kim <[email protected]> > AuthorDate: Thu Feb 21 14:31:51 2019 -0800 > > [CALCITE-2827] Allow Convention.NONE planning with VolcanoPlanner > > By default, cost for Convention.NONE is infinity. > - Add option to allow Convention.None planning with VolcanoPlanner > - Add a unit test > > Change-Id: Idcee2ca923c9b10b58cc9352a390a87f899b64ee > > [CALCITE-2827] Fix checkstyle error > > Change-Id: I50702e6d50d9fe916e15b5bd42f8ec7b12bf0c6f > > [CALCITE-2827] Fix grammar errors > > Change-Id: Icca105184888ff9cc01b2b71f597f3fbd235b638 > --- > .../calcite/plan/volcano/VolcanoPlanner.java | 18 +++++++++-- > .../plan/volcano/VolcanoPlannerTraitTest.java | 36 ++++++++++++++++++++++ > 2 files changed, 52 insertions(+), 2 deletions(-) > > diff --git > a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java > b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java > index bf8336d..54f5049 100644 > --- a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java > +++ b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java > @@ -227,6 +227,11 @@ public class VolcanoPlanner extends > AbstractRelOptPlanner { > */ > private boolean locked; > > + /** > + * Whether rels with Convention.NONE has infinite cost. > + */ > + private boolean noneConventionHasInfiniteCost = true; > + > private final List<RelOptMaterialization> materializations = > new ArrayList<>(); > > @@ -931,13 +936,22 @@ public class VolcanoPlanner extends > AbstractRelOptPlanner { > } > } > > + /** > + * Sets whether this planner should consider rel nodes with Convention.NONE > + * to have inifinte cost or not. > + * @param infinite Whether to make none convention rel nodes inifite cost > + */ > + public void setNoneConventionHasInfiniteCost(boolean infinite) { > + this.noneConventionHasInfiniteCost = infinite; > + } > + > public RelOptCost getCost(RelNode rel, RelMetadataQuery mq) { > assert rel != null : "pre-condition: rel != null"; > if (rel instanceof RelSubset) { > return ((RelSubset) rel).bestCost; > } > - if (rel.getTraitSet().getTrait(ConventionTraitDef.INSTANCE) > - == Convention.NONE) { > + if (noneConventionHasInfiniteCost > + && rel.getTraitSet().getTrait(ConventionTraitDef.INSTANCE) == > Convention.NONE) { > return costFactory.makeInfiniteCost(); > } > RelOptCost cost = mq.getNonCumulativeCost(rel); > diff --git > a/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTraitTest.java > > b/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTraitTest.java > index 66704ed..2c42b3c 100644 > --- > a/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTraitTest.java > +++ > b/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTraitTest.java > @@ -258,6 +258,21 @@ public class VolcanoPlannerTraitTest { > assertTrue(child instanceof PhysLeafRel); > } > > + @Test public void testPlanWithNoneConvention() { > + VolcanoPlanner planner = new VolcanoPlanner(); > + planner.addRelTraitDef(ConventionTraitDef.INSTANCE); > + RelOptCluster cluster = newCluster(planner); > + NoneTinyLeafRel leaf = new NoneTinyLeafRel(cluster, "noneLeafRel"); > + planner.setRoot(leaf); > + RelOptCost cost = planner.getCost(leaf, RelMetadataQuery.instance()); > + > + assertTrue(cost.isInfinite()); > + > + planner.setNoneConventionHasInfiniteCost(false); > + cost = planner.getCost(leaf, RelMetadataQuery.instance()); > + assertTrue(!cost.isInfinite()); > + } > + > //~ Inner Classes ---------------------------------------------------------- > > /** Implementation of {@link RelTrait} for testing. */ > @@ -553,6 +568,27 @@ public class VolcanoPlannerTraitTest { > } > } > > + /** Relational expression with zero input, of NONE convention, and tiny > cost. */ > + private static class NoneTinyLeafRel extends TestLeafRel { > + protected NoneTinyLeafRel( > + RelOptCluster cluster, > + String label) { > + super( > + cluster, > + cluster.traitSetOf(Convention.NONE), > + label); > + } > + > + @Override public RelNode copy(RelTraitSet traitSet, List<RelNode> > inputs) { > + return new NoneTinyLeafRel(getCluster(), getLabel()); > + } > + > + public RelOptCost computeSelfCost(RelOptPlanner planner, > + RelMetadataQuery mq) { > + return planner.getCostFactory().makeTinyCost(); > + } > + } > + > /** Planner rule to convert a {@link NoneSingleRel} to ENUMERABLE > * convention. */ > private static class IterSingleRule extends RelOptRule { >
