Skip to content

Commit

Permalink
Extract class QueryBundle
Browse files Browse the repository at this point in the history
QueryBundle contained a field tableName, but not all query rewrites
result in queries with a target table. Move the field to a subclass
DataQueryBundle.
  • Loading branch information
caithagoras0 committed Sep 10, 2020
1 parent d385583 commit b388a1b
Show file tree
Hide file tree
Showing 18 changed files with 99 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public abstract class AbstractVerification<R extends MatchResult>
public abstract class AbstractVerification<B extends QueryBundle, R extends MatchResult>
implements Verification
{
private static final String INTERNAL_ERROR = "VERIFIER_INTERNAL_ERROR";
Expand Down Expand Up @@ -98,15 +98,15 @@ public AbstractVerification(
this.skipControl = verifierConfig.isSkipControl();
}

protected abstract QueryBundle getQueryRewrite(ClusterType clusterType);
protected abstract B getQueryRewrite(ClusterType clusterType);

protected abstract R verify(QueryBundle control, QueryBundle test, ChecksumQueryContext controlContext, ChecksumQueryContext testContext);
protected abstract R verify(B control, B test, ChecksumQueryContext controlContext, ChecksumQueryContext testContext);

protected abstract DeterminismAnalysisDetails analyzeDeterminism(QueryBundle control, R matchResult);
protected abstract DeterminismAnalysisDetails analyzeDeterminism(B control, R matchResult);

protected abstract Optional<String> resolveFailure(
Optional<QueryBundle> control,
Optional<QueryBundle> test,
Optional<B> control,
Optional<B> test,
QueryContext controlQueryContext,
Optional<R> matchResult,
Optional<Throwable> throwable);
Expand All @@ -131,8 +131,8 @@ public VerificationContext getVerificationContext()
@Override
public VerificationResult run()
{
Optional<QueryBundle> control = Optional.empty();
Optional<QueryBundle> test = Optional.empty();
Optional<B> control = Optional.empty();
Optional<B> test = Optional.empty();
QueryContext controlQueryContext = new QueryContext();
QueryContext testQueryContext = new QueryContext();
ChecksumQueryContext controlChecksumQueryContext = new ChecksumQueryContext();
Expand Down Expand Up @@ -245,8 +245,8 @@ private EventStatus getEventStatus(
}

private PartialVerificationResult concludeVerificationPartial(
Optional<QueryBundle> control,
Optional<QueryBundle> test,
Optional<B> control,
Optional<B> test,
QueryContext controlQueryContext,
QueryContext testQueryContext,
Optional<R> matchResult,
Expand All @@ -261,8 +261,8 @@ private PartialVerificationResult concludeVerificationPartial(

private VerificationResult concludeVerification(
PartialVerificationResult partialResult,
Optional<QueryBundle> control,
Optional<QueryBundle> test,
Optional<B> control,
Optional<B> test,
QueryContext controlQueryContext,
QueryContext testQueryContext,
Optional<R> matchResult,
Expand Down Expand Up @@ -311,11 +311,11 @@ private VerificationResult concludeVerification(
return new VerificationResult(this, false, Optional.of(event));
}

private static QueryInfo buildQueryInfo(
private QueryInfo buildQueryInfo(
QueryConfiguration configuration,
String originalQuery,
ChecksumQueryContext checksumQueryContext,
Optional<QueryBundle> queryBundle,
Optional<B> queryBundle,
QueryContext queryContext)
{
return new QueryInfo(
Expand All @@ -325,9 +325,9 @@ private static QueryInfo buildQueryInfo(
queryContext.getSetupQueryIds(),
queryContext.getTeardownQueryIds(),
checksumQueryContext.getChecksumQueryId(),
queryBundle.map(QueryBundle::getQuery).map(AbstractVerification::formatSql),
queryBundle.map(QueryBundle::getSetupQueries).map(AbstractVerification::formatSqls),
queryBundle.map(QueryBundle::getTeardownQueries).map(AbstractVerification::formatSqls),
queryBundle.map(B::getQuery).map(AbstractVerification::formatSql),
queryBundle.map(B::getSetupQueries).map(AbstractVerification::formatSqls),
queryBundle.map(B::getTeardownQueries).map(AbstractVerification::formatSqls),
checksumQueryContext.getChecksumQuery(),
queryContext.getMainQueryStats());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.facebook.presto.verifier.framework;

import com.facebook.presto.sql.tree.QualifiedName;
import com.facebook.presto.sql.tree.Statement;

import java.util.List;

import static java.util.Objects.requireNonNull;

public class DataQueryBundle
extends QueryBundle
{
private final QualifiedName tableName;

public DataQueryBundle(
QualifiedName tableName,
List<Statement> setupQueries,
Statement query,
List<Statement> teardownQueries,
ClusterType cluster)
{
super(setupQueries, query, teardownQueries, cluster);
this.tableName = requireNonNull(tableName, "tableName is null");
}

public QualifiedName getTableName()
{
return tableName;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
import static java.util.Objects.requireNonNull;

public class DataVerification
extends AbstractVerification<DataMatchResult>
extends AbstractVerification<DataQueryBundle, DataMatchResult>
{
private final QueryRewriter queryRewriter;
private final DeterminismAnalyzer determinismAnalyzer;
Expand Down Expand Up @@ -66,13 +66,13 @@ public DataVerification(
}

@Override
protected QueryBundle getQueryRewrite(ClusterType clusterType)
protected DataQueryBundle getQueryRewrite(ClusterType clusterType)
{
return queryRewriter.rewriteQuery(getSourceQuery().getQuery(clusterType), clusterType);
}

@Override
public DataMatchResult verify(QueryBundle control, QueryBundle test, ChecksumQueryContext controlContext, ChecksumQueryContext testContext)
public DataMatchResult verify(DataQueryBundle control, DataQueryBundle test, ChecksumQueryContext controlContext, ChecksumQueryContext testContext)
{
List<Column> controlColumns = getColumns(getHelperAction(), typeManager, control.getTableName());
List<Column> testColumns = getColumns(getHelperAction(), typeManager, test.getTableName());
Expand All @@ -94,15 +94,15 @@ public DataMatchResult verify(QueryBundle control, QueryBundle test, ChecksumQue
}

@Override
protected DeterminismAnalysisDetails analyzeDeterminism(QueryBundle control, DataMatchResult matchResult)
protected DeterminismAnalysisDetails analyzeDeterminism(DataQueryBundle control, DataMatchResult matchResult)
{
return determinismAnalyzer.analyze(control, matchResult.getControlChecksum());
}

@Override
protected Optional<String> resolveFailure(
Optional<QueryBundle> control,
Optional<QueryBundle> test,
Optional<DataQueryBundle> control,
Optional<DataQueryBundle> test,
QueryContext controlQueryContext,
Optional<DataMatchResult> matchResult,
Optional<Throwable> throwable)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public class DataVerificationUtil

private DataVerificationUtil() {}

public static void teardownSafely(QueryAction queryAction, Optional<QueryBundle> bundle, Consumer<QueryActionStats> queryStatsConsumer)
public static void teardownSafely(QueryAction queryAction, Optional<? extends QueryBundle> bundle, Consumer<QueryActionStats> queryStatsConsumer)
{
if (!bundle.isPresent()) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,14 @@ public DeterminismAnalyzer(
this.handleLimitQuery = config.isHandleLimitQuery();
}

protected DeterminismAnalysisDetails analyze(QueryBundle control, ChecksumResult controlChecksum)
protected DeterminismAnalysisDetails analyze(DataQueryBundle control, ChecksumResult controlChecksum)
{
DeterminismAnalysisDetails.Builder determinismAnalysisDetails = DeterminismAnalysisDetails.builder();
DeterminismAnalysis analysis = analyze(control, controlChecksum, determinismAnalysisDetails);
return determinismAnalysisDetails.build(analysis);
}

private DeterminismAnalysis analyze(QueryBundle control, ChecksumResult controlChecksum, DeterminismAnalysisDetails.Builder determinismAnalysisDetails)
private DeterminismAnalysis analyze(DataQueryBundle control, ChecksumResult controlChecksum, DeterminismAnalysisDetails.Builder determinismAnalysisDetails)
{
// Handle mutable catalogs
if (isNonDeterministicCatalogReferenced(control.getQuery())) {
Expand Down Expand Up @@ -132,7 +132,7 @@ private DeterminismAnalysis analyze(QueryBundle control, ChecksumResult controlC
Map<QueryBundle, DeterminismAnalysisRun.Builder> queryRuns = new HashMap<>();
try {
for (int i = 0; i < maxAnalysisRuns; i++) {
QueryBundle queryBundle = queryRewriter.rewriteQuery(sourceQuery.getQuery(CONTROL), CONTROL);
DataQueryBundle queryBundle = queryRewriter.rewriteQuery(sourceQuery.getQuery(CONTROL), CONTROL);
DeterminismAnalysisRun.Builder run = determinismAnalysisDetails.addRun().setTableName(queryBundle.getTableName().toString());
queryRuns.put(queryBundle, run);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
*/
package com.facebook.presto.verifier.framework;

import com.facebook.presto.sql.tree.QualifiedName;
import com.facebook.presto.sql.tree.Statement;
import com.google.common.collect.ImmutableList;

Expand All @@ -23,31 +22,23 @@

public class QueryBundle
{
private final QualifiedName tableName;
private final List<Statement> setupQueries;
private final Statement query;
private final List<Statement> teardownQueries;
private final ClusterType cluster;

public QueryBundle(
QualifiedName tableName,
List<Statement> setupQueries,
Statement query,
List<Statement> teardownQueries,
ClusterType cluster)
{
this.tableName = requireNonNull(tableName, "tableName is null");
this.setupQueries = ImmutableList.copyOf(setupQueries);
this.query = requireNonNull(query, "query is null");
this.teardownQueries = ImmutableList.copyOf(teardownQueries);
this.cluster = requireNonNull(cluster, "cluster is null");
}

public QualifiedName getTableName()
{
return tableName;
}

public List<Statement> getSetupQueries()
{
return setupQueries;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
package com.facebook.presto.verifier.resolver;

import com.facebook.presto.jdbc.QueryStats;
import com.facebook.presto.verifier.framework.QueryBundle;
import com.facebook.presto.verifier.framework.DataQueryBundle;
import com.facebook.presto.verifier.framework.QueryException;
import com.google.common.collect.ImmutableSet;

Expand All @@ -30,7 +30,7 @@ public class ChecksumExceededTimeLimitFailureResolver
public static final String NAME = "checksum-exceeded-time-limit";

@Override
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<QueryBundle> test)
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<DataQueryBundle> test)
{
return mapMatchingPrestoException(queryException, CONTROL_CHECKSUM, ImmutableSet.of(EXCEEDED_TIME_LIMIT),
e -> Optional.of("Time limit exceeded when running control checksum query"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
package com.facebook.presto.verifier.resolver;

import com.facebook.presto.jdbc.QueryStats;
import com.facebook.presto.verifier.framework.QueryBundle;
import com.facebook.presto.verifier.framework.DataQueryBundle;
import com.facebook.presto.verifier.framework.QueryException;
import com.google.common.collect.ImmutableSet;

Expand All @@ -30,7 +30,7 @@ public class ExceededGlobalMemoryLimitFailureResolver
public static final String NAME = "exceeded-global-memory-limit";

@Override
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<QueryBundle> test)
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<DataQueryBundle> test)
{
return mapMatchingPrestoException(queryException, TEST_MAIN, ImmutableSet.of(EXCEEDED_GLOBAL_MEMORY_LIMIT),
e -> e.getQueryActionStats().getQueryStats().isPresent()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
package com.facebook.presto.verifier.resolver;

import com.facebook.presto.jdbc.QueryStats;
import com.facebook.presto.verifier.framework.QueryBundle;
import com.facebook.presto.verifier.framework.DataQueryBundle;
import com.facebook.presto.verifier.framework.QueryException;
import com.google.common.collect.ImmutableSet;

Expand All @@ -30,7 +30,7 @@ public class ExceededTimeLimitFailureResolver
public static final String NAME = "exceeded-time-limit";

@Override
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<QueryBundle> test)
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<DataQueryBundle> test)
{
return mapMatchingPrestoException(queryException, TEST_MAIN, ImmutableSet.of(EXCEEDED_TIME_LIMIT),
e -> Optional.of("Time limit exceeded on test cluster"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,15 @@

import com.facebook.presto.jdbc.QueryStats;
import com.facebook.presto.verifier.framework.DataMatchResult;
import com.facebook.presto.verifier.framework.DataQueryBundle;
import com.facebook.presto.verifier.framework.QueryBundle;
import com.facebook.presto.verifier.framework.QueryException;

import java.util.Optional;

public interface FailureResolver
{
default Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<QueryBundle> test)
default Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<DataQueryBundle> test)
{
return Optional.empty();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.facebook.presto.verifier.resolver;

import com.facebook.presto.verifier.framework.DataMatchResult;
import com.facebook.presto.verifier.framework.DataQueryBundle;
import com.facebook.presto.verifier.framework.QueryBundle;
import com.facebook.presto.verifier.framework.QueryException;
import com.facebook.presto.verifier.prestoaction.QueryActionStats;
Expand All @@ -32,7 +33,7 @@ public FailureResolverManager(Set<FailureResolver> failureResolvers)
this.failureResolvers = requireNonNull(failureResolvers, "failureResolvers is null");
}

public Optional<String> resolveException(QueryActionStats controlStats, Throwable throwable, Optional<QueryBundle> test)
public Optional<String> resolveException(QueryActionStats controlStats, Throwable throwable, Optional<DataQueryBundle> test)
{
if (!(throwable instanceof QueryException)) {
return Optional.of("Verifier Error");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import com.facebook.presto.sql.tree.Property;
import com.facebook.presto.sql.tree.ShowCreate;
import com.facebook.presto.verifier.annotation.ForTest;
import com.facebook.presto.verifier.framework.QueryBundle;
import com.facebook.presto.verifier.framework.DataQueryBundle;
import com.facebook.presto.verifier.framework.QueryException;
import com.facebook.presto.verifier.prestoaction.PrestoAction;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -73,7 +73,7 @@ public TooManyOpenPartitionsFailureResolver(
}

@Override
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<QueryBundle> test)
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<DataQueryBundle> test)
{
if (!test.isPresent()) {
return Optional.empty();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
package com.facebook.presto.verifier.resolver;

import com.facebook.presto.jdbc.QueryStats;
import com.facebook.presto.verifier.framework.QueryBundle;
import com.facebook.presto.verifier.framework.DataQueryBundle;
import com.facebook.presto.verifier.framework.QueryException;
import com.google.common.collect.ImmutableSet;

Expand All @@ -31,7 +31,7 @@ public class VerifierLimitationFailureResolver
public static final String NAME = "verifier-limitation";

@Override
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<QueryBundle> test)
public Optional<String> resolveQueryFailure(QueryStats controlQueryStats, QueryException queryException, Optional<DataQueryBundle> test)
{
return mapMatchingPrestoException(queryException, CONTROL_CHECKSUM, ImmutableSet.of(COMPILER_ERROR, GENERATED_BYTECODE_TOO_LARGE),
e -> Optional.of("Checksum query too large"));
Expand Down
Loading

0 comments on commit b388a1b

Please sign in to comment.