Laurent did answer my objection and indicate he's willing to revert. I'm ok with the provided explanation. (Although an unrelated note, things like "Fix checkstyle error" and "Fix grammar errors" should not be part of the final commit message.) -- Michael Mior mm...@apache.org
Le mer. 27 févr. 2019 à 16:16, Julian Hyde <jh...@apache.org> a écrit : > > Laurent, > > Michael had started reviewing this and had some objections. Please don’t > summarily commit. > > Julian > > > > On Feb 27, 2019, at 1:07 PM, laur...@apache.org 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 <j...@dremio.com> > > 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 { > > >
