Skip to content

Commit

Permalink
Add intermediate plan validatation
Browse files Browse the repository at this point in the history
Check if plan is valid before applying any optimizers.
If plan is invalid then some optimizer may throw very misleading
exception. For example please see the #7773 issue, where access control
error was throw, but it was caused unexpected subquery expression
within an plan node tree.

No the query from #7773 would throw something like below:
java.lang.IllegalStateException: Unexpected subquery expression in
logical plan: (SELECT 1

    )
        at
        com.facebook.presto.sql.planner.sanity.NoSubqueryExpressionLeftChecker$1.visitSubqueryExpression(NoSubqueryExpressionLeftChecker.java:43)
        ...
        com.facebook.presto.sql.planner.sanity.PlanSanityChecker.validateIntermediatePlan(PlanSanityChecker.java:54)
        ...
  • Loading branch information
kokosing committed Jun 21, 2017
1 parent 802f94e commit 178f9f5
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 14 deletions.
Expand Up @@ -125,6 +125,8 @@ public Plan plan(Analysis analysis, Stage stage)
{
PlanNode root = planStatement(analysis, analysis.getStatement());

PlanSanityChecker.validateIntermediatePlan(root, session, metadata, sqlParser, symbolAllocator.getTypes());

if (stage.ordinal() >= Stage.OPTIMIZED.ordinal()) {
for (PlanOptimizer optimizer : planOptimizers) {
root = optimizer.optimize(root, session, symbolAllocator.getTypes(), symbolAllocator, idAllocator);
Expand All @@ -134,7 +136,7 @@ public Plan plan(Analysis analysis, Stage stage)

if (stage.ordinal() >= Stage.OPTIMIZED_AND_VALIDATED.ordinal()) {
// make sure we produce a valid plan after optimizations run. This is mainly to catch programming errors
PlanSanityChecker.validate(root, session, metadata, sqlParser, symbolAllocator.getTypes());
PlanSanityChecker.validateFinalPlan(root, session, metadata, sqlParser, symbolAllocator.getTypes());
}

Map<PlanNodeId, PlanNodeCost> planNodeCosts = costCalculator.calculateCostForPlan(session, symbolAllocator.getTypes(), root);
Expand Down
Expand Up @@ -19,34 +19,54 @@
import com.facebook.presto.sql.parser.SqlParser;
import com.facebook.presto.sql.planner.Symbol;
import com.facebook.presto.sql.planner.plan.PlanNode;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Multimap;

import java.util.List;
import java.util.Map;

/**
* It is going to be executed at the end of logical planner, to verify its correctness
* It is going to be executed to verify logical planner correctness
*/
public final class PlanSanityChecker
{
private static final List<Checker> CHECKERS = ImmutableList.of(
new ValidateDependenciesChecker(),
new TypeValidator(),
new NoSubqueryExpressionLeftChecker(),
new NoDuplicatePlanNodeIdsChecker(),
new NoSubqueryRelatedNodeLeftChecker(),
new VerifyNoFilteredAggregations(),
new VerifyOnlyOneOutputNode());
private static final Multimap<Stage, Checker> CHECKERS = ImmutableListMultimap.<Stage, Checker>builder()
.putAll(
Stage.INTERMEDIATE,
new ValidateDependenciesChecker(),
new NoDuplicatePlanNodeIdsChecker(),
new TypeValidator(),
new NoSubqueryExpressionLeftChecker(),
new VerifyOnlyOneOutputNode())
.putAll(
Stage.FINAL,
new ValidateDependenciesChecker(),
new NoDuplicatePlanNodeIdsChecker(),
new TypeValidator(),
new NoSubqueryExpressionLeftChecker(),
new VerifyOnlyOneOutputNode(),
new NoSubqueryRelatedNodeLeftChecker(),
new VerifyNoFilteredAggregations())
.build();

private PlanSanityChecker() {}

public static void validate(PlanNode planNode, Session session, Metadata metadata, SqlParser sqlParser, Map<Symbol, Type> types)
public static void validateFinalPlan(PlanNode planNode, Session session, Metadata metadata, SqlParser sqlParser, Map<Symbol, Type> types)
{
CHECKERS.forEach(checker -> checker.validate(planNode, session, metadata, sqlParser, types));
CHECKERS.get(Stage.FINAL).forEach(checker -> checker.validate(planNode, session, metadata, sqlParser, types));
}

public static void validateIntermediatePlan(PlanNode planNode, Session session, Metadata metadata, SqlParser sqlParser, Map<Symbol, Type> types)
{
CHECKERS.get(Stage.INTERMEDIATE).forEach(checker -> checker.validate(planNode, session, metadata, sqlParser, types));
}

public interface Checker
{
void validate(PlanNode planNode, Session session, Metadata metadata, SqlParser sqlParser, Map<Symbol, Type> types);
}

private enum Stage
{
INTERMEDIATE, FINAL
};
}

0 comments on commit 178f9f5

Please sign in to comment.