[CALCITE-2537] Use litmus for VolcanoPlanner#validate VolcanoPlanner#validate() throws exception if validation fail, but this only happens if the logger level is debug or finer. As changing log level should not have control side effects, this might throw users off.
This change changes `void VolcanoPlanner#validate()` into `boolean VolcanoPlanner#isValid(Litmus)` and this method is only called if assertions are enabled. Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/a5378a32 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/a5378a32 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/a5378a32 Branch: refs/heads/master Commit: a5378a3210760ff94933755b716b676b50992db3 Parents: c69e1bc Author: Laurent Goujon <[email protected]> Authored: Wed Sep 5 21:42:25 2018 -0700 Committer: Julian Hyde <[email protected]> Committed: Mon Sep 17 09:43:02 2018 -0700 ---------------------------------------------------------------------- .../calcite/plan/volcano/VolcanoPlanner.java | 23 +++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/a5378a32/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java ---------------------------------------------------------------------- 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 f7b1601..20eadfa 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 @@ -858,8 +858,10 @@ public class VolcanoPlanner extends AbstractRelOptPlanner { } final RelSubset subset = registerImpl(rel, set); + // Checking if tree is valid considerably slows down planning + // Only doing it if logger level is debug or finer if (LOGGER.isDebugEnabled()) { - validate(); + assert isValid(Litmus.THROW); } return subset; @@ -883,31 +885,26 @@ public class VolcanoPlanner extends AbstractRelOptPlanner { /** * Checks internal consistency. */ - protected void validate() { + protected boolean isValid(Litmus litmus) { for (RelSet set : allSets) { if (set.equivalentSet != null) { - throw new AssertionError( - "set [" + set - + "] has been merged: it should not be in the list"); + return litmus.fail("set [{}] has been merged: it should not be in the list", set); } for (RelSubset subset : set.subsets) { if (subset.set != set) { - throw new AssertionError( - "subset [" + subset.getDescription() - + "] is in wrong set [" + set + "]"); + return litmus.fail("subset [{}] is in wrong set [{}]", + subset.getDescription(), set); } for (RelNode rel : subset.getRels()) { RelOptCost relCost = getCost(rel, rel.getCluster().getMetadataQuery()); if (relCost.isLt(subset.bestCost)) { - throw new AssertionError( - "rel [" + rel.getDescription() - + "] has lower cost " + relCost - + " than best cost " + subset.bestCost - + " of subset [" + subset.getDescription() + "]"); + return litmus.fail("rel [{}] has lower cost {} than best cost {} of subset [{}]", + rel.getDescription(), relCost, subset.bestCost, subset.getDescription()); } } } } + return litmus.succeed(); } public void registerAbstractRelationalRules() {
