Skip to content

Commit

Permalink
Improve plan matching for explain verification
Browse files Browse the repository at this point in the history 
Plain text comparison for JSON plan is inaccurate. There are too many
factors in two different deployments to cause query plan to not match
exactly.

Replace MatchType PLAN_CHANGED with STRUCTURE_MISMATCH and
DETAILS_MISMATCH.
  • Loading branch information
@caithagoras0
caithagoras0 committed Sep 23, 2020
1 parent fdfd9be commit 4d91a60
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 11 deletions.
1 change: 1 addition & 0 deletions presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/JsonRenderer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public static class JsonRenderedNode
private final List<JsonRenderedNode> children;
private final List<String> remoteSources;

@JsonCreator
public JsonRenderedNode(String id, String name, String identifier, String details, List<JsonRenderedNode> children, List<String> remoteSources)
{
this.id = requireNonNull(id, "id is null");
Expand Down
3 changes: 2 additions & 1 deletion presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainMatchResult.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ public class ExplainMatchResult
public enum MatchType
{
MATCH,
PLAN_CHANGED,
STRUCTURE_MISMATCH,
DETAILS_MISMATCH,
}

private final MatchType matchType;
Expand Down
43 changes: 35 additions & 8 deletions ...to-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
*/
package com.facebook.presto.verifier.framework;

import com.facebook.airlift.json.JsonCodec;
import com.facebook.presto.sql.parser.SqlParser;
import com.facebook.presto.sql.planner.planPrinter.JsonRenderer.JsonRenderedNode;
import com.facebook.presto.sql.tree.Explain;
import com.facebook.presto.sql.tree.ExplainFormat;
import com.facebook.presto.sql.tree.Statement;
Expand All @@ -24,15 +26,15 @@
import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier;
import com.google.common.collect.ImmutableList;

import java.util.Objects;
import java.util.Optional;

import static com.facebook.airlift.json.JsonCodec.jsonCodec;
import static com.facebook.presto.sql.tree.ExplainFormat.Type.JSON;
import static com.facebook.presto.verifier.framework.ClusterType.CONTROL;
import static com.facebook.presto.verifier.framework.ExplainMatchResult.MatchType;
import static com.facebook.presto.verifier.framework.ExplainMatchResult.MatchType.DETAILS_MISMATCH;
import static com.facebook.presto.verifier.framework.ExplainMatchResult.MatchType.MATCH;
import static com.facebook.presto.verifier.framework.ExplainMatchResult.MatchType.PLAN_CHANGED;
import static com.facebook.presto.verifier.framework.QueryType.CREATE_TABLE_AS_SELECT;
import static com.facebook.presto.verifier.framework.QueryType.INSERT;
import static com.facebook.presto.verifier.framework.QueryType.QUERY;
import static com.facebook.presto.verifier.framework.ExplainMatchResult.MatchType.STRUCTURE_MISMATCH;
import static com.facebook.presto.verifier.framework.VerifierUtil.PARSING_OPTIONS;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.Iterables.getOnlyElement;
Expand All @@ -42,6 +44,7 @@ public class ExplainVerification
extends AbstractVerification<QueryBundle, ExplainMatchResult, String>
{
private static final ResultSetConverter<String> QUERY_PLAN_RESULT_SET_CONVERTER = resultSet -> Optional.of(resultSet.getString("Query Plan"));
private static final JsonCodec<JsonRenderedNode> PLAN_CODEC = jsonCodec(JsonRenderedNode.class);
private final SqlParser sqlParser;

public ExplainVerification(
Expand Down Expand Up @@ -75,9 +78,11 @@ protected ExplainMatchResult verify(
{
checkArgument(controlQueryResult.isPresent(), "control query plan is missing");
checkArgument(testQueryResult.isPresent(), "test query plan is missing");
String controlPlan = getOnlyElement(controlQueryResult.get().getResults());
String testPlan = getOnlyElement(testQueryResult.get().getResults());
return new ExplainMatchResult(testPlan.equals(controlPlan) ? MATCH : PLAN_CHANGED);

JsonRenderedNode controlPlan = PLAN_CODEC.fromJson(getOnlyElement(controlQueryResult.get().getResults()));
JsonRenderedNode testPlan = PLAN_CODEC.fromJson(getOnlyElement(testQueryResult.get().getResults()));

return new ExplainMatchResult(match(controlPlan, testPlan));
}

@Override
Expand All @@ -97,4 +102,26 @@ protected void updateQueryInfo(QueryInfo.Builder queryInfo, Optional<QueryResult
{
queryResult.ifPresent(result -> queryInfo.setJsonPlan(getOnlyElement(result.getResults())));
}

private MatchType match(JsonRenderedNode controlPlan, JsonRenderedNode testPlan)
{
if (!Objects.equals(controlPlan.getName(), testPlan.getName())
|| !Objects.equals(controlPlan.getIdentifier(), testPlan.getIdentifier())
|| !Objects.equals(controlPlan.getRemoteSources(), testPlan.getRemoteSources())
|| controlPlan.getChildren().size() != testPlan.getChildren().size()) {
return STRUCTURE_MISMATCH;
}

boolean detailsMismatched = !Objects.equals(controlPlan.getDetails(), testPlan.getDetails());
for (int i = 0; i < controlPlan.getChildren().size(); i++) {
MatchType childMatchType = match(controlPlan.getChildren().get(i), testPlan.getChildren().get(i));
if (childMatchType == STRUCTURE_MISMATCH) {
return STRUCTURE_MISMATCH;
}
else if (childMatchType == DETAILS_MISMATCH) {
detailsMismatched = true;
}
}
return detailsMismatched ? DETAILS_MISMATCH : MATCH;
}
}
22 changes: 20 additions & 2 deletions ...erifier/src/test/java/com/facebook/presto/verifier/framework/TestExplainVerification.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,32 @@ public void testSuccess()
}

@Test
public void testPlanChanged()
public void testStructureMismatch()
{
queryRunner.execute("CREATE TABLE structure_mismatch (x int, ds varchar)");
Optional<VerifierQueryEvent> event = runVerification(
"SELECT count(*) FROM structure_mismatch",
"SELECT count(*) FROM structure_mismatch CROSS JOIN structure_mismatch");
assertTrue(event.isPresent());

// Explain verification do not fail in case of plan changes.
assertEvent(event.get(), SUCCEEDED);
assertEquals(event.get().getMatchType(), "STRUCTURE_MISMATCH");
assertEquals(event.get().getControlQueryInfo().getQuery().trim(), "EXPLAIN (FORMAT JSON)\nSELECT \"count\"(*)\nFROM\n structure_mismatch");
assertEquals(event.get().getTestQueryInfo().getQuery().trim(), "EXPLAIN (FORMAT JSON)\nSELECT \"count\"(*)\nFROM\n (structure_mismatch\nCROSS JOIN structure_mismatch)");
assertNotNull(event.get().getControlQueryInfo().getJsonPlan());
assertNotNull(event.get().getTestQueryInfo().getJsonPlan());
}

@Test
public void testDetailsMismatch()
{
Optional<VerifierQueryEvent> event = runVerification("SELECT 1", "SELECT 2");
assertTrue(event.isPresent());

// Explain verification do not fail in case of plan changes.
assertEvent(event.get(), SUCCEEDED);
assertEquals(event.get().getMatchType(), "PLAN_CHANGED");
assertEquals(event.get().getMatchType(), "DETAILS_MISMATCH");
assertEquals(event.get().getControlQueryInfo().getQuery().trim(), "EXPLAIN (FORMAT JSON)\nSELECT 1");
assertEquals(event.get().getTestQueryInfo().getQuery().trim(), "EXPLAIN (FORMAT JSON)\nSELECT 2");
assertNotNull(event.get().getControlQueryInfo().getJsonPlan());
Expand Down

0 comments on commit 4d91a60

Please sign in to comment.