From 402108bf3d8bbe4d21a5147bac4eaba2c36c4444 Mon Sep 17 00:00:00 2001 From: Sergey Pershin Date: Mon, 3 Feb 2025 17:42:30 -0800 Subject: [PATCH] Add 'use-test-checksum-in-determinism-analyzer' config property to Verifier. --- .../event/DeterminismAnalysisRun.java | 19 +++++- .../framework/AbstractVerification.java | 16 ++++- .../verifier/framework/DataMatchResult.java | 9 +++ .../verifier/framework/DataVerification.java | 31 ++++++++- .../framework/DataVerificationUtil.java | 2 + .../verifier/framework/DdlVerification.java | 2 +- .../framework/DeterminismAnalyzer.java | 39 ++++++----- .../framework/ExplainVerification.java | 2 +- .../framework/ExtendedVerification.java | 4 ++ .../verifier/framework/VerifierConfig.java | 16 ++++- .../framework/AbstractVerificationTest.java | 6 +- .../framework/TestDataVerification.java | 65 +++++++++++++++++++ .../framework/TestVerifierConfig.java | 3 + .../resolver/FailureResolverTestUtil.java | 2 +- 14 files changed, 187 insertions(+), 29 deletions(-) diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/event/DeterminismAnalysisRun.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/event/DeterminismAnalysisRun.java index d5334ec241cfb..42f8598a94438 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/event/DeterminismAnalysisRun.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/event/DeterminismAnalysisRun.java @@ -29,6 +29,7 @@ public class DeterminismAnalysisRun { private final String tableName; + private final String clusterType; private final String queryId; private final List setupQueryIds; private final List teardownQueryIds; @@ -36,12 +37,14 @@ public class DeterminismAnalysisRun private DeterminismAnalysisRun( Optional tableName, + Optional clusterType, Optional queryId, List setupQueryIds, List teardownQueryIds, Optional checksumQueryId) { this.tableName = tableName.orElse(null); + this.clusterType = clusterType.orElse(null); this.queryId = queryId.orElse(null); this.setupQueryIds = ImmutableList.copyOf(setupQueryIds); this.teardownQueryIds = ImmutableList.copyOf(teardownQueryIds); @@ -54,6 +57,12 @@ public String getTableName() return tableName; } + @EventField + public String getClusterType() + { + return clusterType; + } + @EventField public String getQueryId() { @@ -86,6 +95,7 @@ public static Builder builder() public static class Builder { private Optional tableName = Optional.empty(); + private Optional clusterType = Optional.empty(); private Optional queryId = Optional.empty(); private ImmutableList.Builder setupQueryIds = ImmutableList.builder(); private ImmutableList.Builder teardownQueryIds = ImmutableList.builder(); @@ -100,6 +110,13 @@ public Builder setTableName(String tableName) return this; } + public Builder setClusterType(String clusterType) + { + checkState(!this.clusterType.isPresent(), "clusterType is already set"); + this.clusterType = Optional.of(clusterType); + return this; + } + public Builder setQueryId(String queryId) { checkState(!this.queryId.isPresent(), "queryId is already set"); @@ -128,7 +145,7 @@ public Builder setChecksumQueryId(String checksumQueryId) public DeterminismAnalysisRun build() { - return new DeterminismAnalysisRun(tableName, queryId, setupQueryIds.build(), teardownQueryIds.build(), checksumQueryId); + return new DeterminismAnalysisRun(tableName, clusterType, queryId, setupQueryIds.build(), teardownQueryIds.build(), checksumQueryId); } } } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java index cd94ba3be540b..31f9bcaf0f66b 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/AbstractVerification.java @@ -96,6 +96,7 @@ public abstract class AbstractVerification snapshotQueries; @@ -131,6 +132,7 @@ public AbstractVerification( this.runningMode = verifierConfig.getRunningMode(); this.saveSnapshot = verifierConfig.isSaveSnapshot(); this.isExplain = verifierConfig.isExplain(); + this.isRunDeterminismAnalysisOnTest = verifierConfig.isRunDeterminismAnalysisOnTest(); } protected abstract B getQueryRewrite(ClusterType clusterType); @@ -143,7 +145,7 @@ protected abstract R verify( ChecksumQueryContext controlChecksumQueryContext, ChecksumQueryContext testChecksumQueryContext); - protected abstract DeterminismAnalysisDetails analyzeDeterminism(B control, R matchResult); + protected abstract DeterminismAnalysisDetails analyzeDeterminism(B controlObject, B testObject, R matchResult); protected abstract Optional resolveFailure( Optional control, @@ -166,6 +168,16 @@ protected PrestoAction getHelperAction() return queryActions.getHelperAction(); } + protected QueryAction getTestAction() + { + return queryActions.getTestAction(); + } + + protected QueryAction getControlAction() + { + return queryActions.getControlAction(); + } + protected boolean isControlEnabled() { return !skipControl || saveSnapshot; @@ -291,7 +303,7 @@ else if ((isControlEnabled()) && !skipChecksum) { return new VerificationResult(this, true, Optional.empty()); } else if (matchResult.get().isMismatchPossiblyCausedByNonDeterminism()) { - determinismAnalysisDetails = Optional.of(analyzeDeterminism(control.get(), matchResult.get())); + determinismAnalysisDetails = Optional.of(analyzeDeterminism(control.get(), test.get(), matchResult.get())); } } } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataMatchResult.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataMatchResult.java index 6de343581713a..7733b6658d7e4 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataMatchResult.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataMatchResult.java @@ -53,6 +53,7 @@ public enum MatchType private final DataType dataType; private final MatchType matchType; private final Optional controlChecksum; + private final Optional testChecksum; private final OptionalLong controlRowCount; private final OptionalLong testRowCount; private final List> mismatchedColumns; @@ -61,6 +62,7 @@ public DataMatchResult( DataType dataType, MatchType matchType, Optional controlChecksum, + Optional testChecksum, OptionalLong controlRowCount, OptionalLong testRowCount, List> mismatchedColumns) @@ -68,6 +70,7 @@ public DataMatchResult( this.dataType = requireNonNull(dataType, "data type is null"); this.matchType = requireNonNull(matchType, "match type is null"); this.controlChecksum = requireNonNull(controlChecksum, "controlChecksum is null"); + this.testChecksum = requireNonNull(testChecksum, "testChecksum is null"); this.controlRowCount = requireNonNull(controlRowCount, "controlRowCount is null"); this.testRowCount = requireNonNull(testRowCount, "testRowCount is null"); this.mismatchedColumns = ImmutableList.copyOf(mismatchedColumns); @@ -114,6 +117,12 @@ public ChecksumResult getControlChecksum() return controlChecksum.get(); } + public ChecksumResult getTestChecksum() + { + checkState(testChecksum.isPresent(), "testChecksum is missing"); + return testChecksum.get(); + } + public List> getMismatchedColumns() { return mismatchedColumns; diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java index 73aba566d8181..26cc2c8aeba25 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerification.java @@ -20,6 +20,7 @@ import com.facebook.presto.verifier.checksum.ChecksumResult; import com.facebook.presto.verifier.checksum.ChecksumValidator; import com.facebook.presto.verifier.event.DeterminismAnalysisDetails; +import com.facebook.presto.verifier.event.DeterminismAnalysisRun; import com.facebook.presto.verifier.event.QueryInfo; import com.facebook.presto.verifier.prestoaction.QueryActions; import com.facebook.presto.verifier.prestoaction.SqlExceptionClassifier; @@ -34,6 +35,8 @@ import java.util.Map; import java.util.Optional; import java.util.OptionalLong; +import java.util.stream.Collectors; +import java.util.stream.Stream; import static com.facebook.presto.verifier.framework.DataMatchResult.DataType.DATA; import static com.facebook.presto.verifier.framework.DataMatchResult.MatchType.MATCH; @@ -132,6 +135,7 @@ public DataMatchResult verify( DATA, MATCH, Optional.empty(), + Optional.empty(), OptionalLong.empty(), OptionalLong.empty(), ImmutableList.of()); @@ -146,7 +150,14 @@ else if (QUERY_BANK_MODE.equals(runningMode)) { controlChecksumResult = ChecksumResult.fromJson(snapshotJson); } else { - return new DataMatchResult(DATA, SNAPSHOT_DOES_NOT_EXIST, Optional.empty(), OptionalLong.empty(), OptionalLong.empty(), Collections.emptyList()); + return new DataMatchResult( + DATA, + SNAPSHOT_DOES_NOT_EXIST, + Optional.empty(), + Optional.empty(), + OptionalLong.empty(), + OptionalLong.empty(), + Collections.emptyList()); } } @@ -159,9 +170,23 @@ else if (QUERY_BANK_MODE.equals(runningMode)) { } @Override - protected DeterminismAnalysisDetails analyzeDeterminism(QueryObjectBundle control, DataMatchResult matchResult) + protected DeterminismAnalysisDetails analyzeDeterminism(QueryObjectBundle controlObject, QueryObjectBundle testObject, DataMatchResult matchResult) { - return determinismAnalyzer.analyze(control, matchResult.getControlChecksum()); + if (isRunDeterminismAnalysisOnTest) { + DeterminismAnalysisDetails analysis = determinismAnalyzer.analyze(getTestAction(), testObject, matchResult.getTestChecksum()); + if (!analysis.getDeterminismAnalysis().isNonDeterministic()) { + return analysis; + } + // In case we rerun determinism analysis on control, we keep the test runs for stats. + List runs = analysis.getRuns(); + analysis = determinismAnalyzer.analyze(getControlAction(), controlObject, matchResult.getControlChecksum()); + return new DeterminismAnalysisDetails( + analysis.getDeterminismAnalysis(), + Stream.concat(runs.stream(), analysis.getRuns().stream()).collect(Collectors.toList()), + LimitQueryDeterminismAnalysis.valueOf(analysis.getLimitQueryAnalysis()), + Optional.ofNullable(analysis.getLimitQueryAnalysisQueryId())); + } + return determinismAnalyzer.analyze(getControlAction(), controlObject, matchResult.getControlChecksum()); } @Override diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerificationUtil.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerificationUtil.java index 7847e9ac4a19a..6baa9a514f62a 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerificationUtil.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DataVerificationUtil.java @@ -85,6 +85,7 @@ public static DataMatchResult match( dataType, SCHEMA_MISMATCH, Optional.empty(), + Optional.empty(), OptionalLong.empty(), OptionalLong.empty(), ImmutableList.of()); @@ -107,6 +108,7 @@ public static DataMatchResult match( dataType, matchType, Optional.of(controlChecksum), + Optional.of(testChecksum), controlRowCount, testRowCount, mismatchedColumns); diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java index c8948a574aaa4..81c7b4bcd228e 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DdlVerification.java @@ -152,7 +152,7 @@ else if (QUERY_BANK_MODE.equals(runningMode)) { } @Override - protected DeterminismAnalysisDetails analyzeDeterminism(QueryObjectBundle control, DdlMatchResult matchResult) + protected DeterminismAnalysisDetails analyzeDeterminism(QueryObjectBundle controlObject, QueryObjectBundle testObject, DdlMatchResult matchResult) { throw new UnsupportedOperationException("analyzeDeterminism is not supported for DdlVerification"); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DeterminismAnalyzer.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DeterminismAnalyzer.java index dabf0121b3f1d..2d69c1a2eeb65 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DeterminismAnalyzer.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/DeterminismAnalyzer.java @@ -25,6 +25,7 @@ import com.facebook.presto.verifier.event.DeterminismAnalysisDetails; import com.facebook.presto.verifier.event.DeterminismAnalysisRun; import com.facebook.presto.verifier.prestoaction.PrestoAction; +import com.facebook.presto.verifier.prestoaction.QueryAction; import com.facebook.presto.verifier.rewrite.QueryRewriter; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; @@ -36,7 +37,6 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; -import static com.facebook.presto.verifier.framework.ClusterType.CONTROL; import static com.facebook.presto.verifier.framework.DataVerificationUtil.getColumns; import static com.facebook.presto.verifier.framework.DataVerificationUtil.match; import static com.facebook.presto.verifier.framework.DataVerificationUtil.teardownSafely; @@ -60,7 +60,7 @@ public class DeterminismAnalyzer { private final SourceQuery sourceQuery; - private final PrestoAction prestoAction; + private final PrestoAction helperAction; private final QueryRewriter queryRewriter; private final ChecksumValidator checksumValidator; private final TypeManager typeManager; @@ -72,14 +72,14 @@ public class DeterminismAnalyzer public DeterminismAnalyzer( SourceQuery sourceQuery, - PrestoAction prestoAction, + PrestoAction helperAction, QueryRewriter queryRewriter, ChecksumValidator checksumValidator, TypeManager typeManager, DeterminismAnalyzerConfig config) { this.sourceQuery = requireNonNull(sourceQuery, "sourceQuery is null"); - this.prestoAction = requireNonNull(prestoAction, "prestoAction is null"); + this.helperAction = requireNonNull(helperAction, "helperAction is null"); this.queryRewriter = requireNonNull(queryRewriter, "queryRewriter is null"); this.checksumValidator = requireNonNull(checksumValidator, "checksumValidator is null"); this.typeManager = requireNonNull(typeManager, "typeManager is null"); @@ -90,26 +90,27 @@ public DeterminismAnalyzer( this.handleLimitQuery = config.isHandleLimitQuery(); } - protected DeterminismAnalysisDetails analyze(QueryObjectBundle control, ChecksumResult controlChecksum) + protected DeterminismAnalysisDetails analyze(QueryAction queryAction, QueryObjectBundle objectBundle, ChecksumResult referenceChecksum) { + requireNonNull(queryAction, "queryAction is null"); DeterminismAnalysisDetails.Builder determinismAnalysisDetails = DeterminismAnalysisDetails.builder(); - DeterminismAnalysis analysis = analyze(control, controlChecksum, determinismAnalysisDetails); + DeterminismAnalysis analysis = analyze(queryAction, objectBundle, referenceChecksum, determinismAnalysisDetails); return determinismAnalysisDetails.build(analysis); } - private DeterminismAnalysis analyze(QueryObjectBundle control, ChecksumResult controlChecksum, DeterminismAnalysisDetails.Builder determinismAnalysisDetails) + private DeterminismAnalysis analyze(QueryAction actionForQuery, QueryObjectBundle objectBundle, ChecksumResult referenceChecksum, DeterminismAnalysisDetails.Builder determinismAnalysisDetails) { // Handle mutable catalogs - if (isNonDeterministicCatalogReferenced(control.getQuery())) { + if (isNonDeterministicCatalogReferenced(objectBundle.getQuery())) { return NON_DETERMINISTIC_CATALOG; } // Handle limit query LimitQueryDeterminismAnalysis limitQueryAnalysis = new LimitQueryDeterminismAnalyzer( - prestoAction, + helperAction, handleLimitQuery, - control.getQuery(), - controlChecksum.getRowCount(), + objectBundle.getQuery(), + referenceChecksum.getRowCount(), determinismAnalysisDetails).analyze(); switch (limitQueryAnalysis) { @@ -127,29 +128,31 @@ private DeterminismAnalysis analyze(QueryObjectBundle control, ChecksumResult co } // Rerun control query multiple times - List columns = getColumns(prestoAction, typeManager, control.getObjectName()); + List columns = getColumns(helperAction, typeManager, objectBundle.getObjectName()); + ClusterType clusterType = objectBundle.getCluster(); Map queryRuns = new HashMap<>(); try { for (int i = 0; i < maxAnalysisRuns; i++) { - QueryObjectBundle queryBundle = queryRewriter.rewriteQuery(sourceQuery.getQuery(CONTROL), sourceQuery.getQueryConfiguration(CONTROL), CONTROL, false); + QueryObjectBundle queryBundle = queryRewriter.rewriteQuery(sourceQuery.getQuery(clusterType), sourceQuery.getQueryConfiguration(clusterType), clusterType, false); DeterminismAnalysisRun.Builder run = determinismAnalysisDetails.addRun().setTableName(queryBundle.getObjectName().toString()); + run.setClusterType(clusterType.name()); queryRuns.put(queryBundle, run); // Rerun setup and main query queryBundle.getSetupQueries().forEach(query -> runAndConsume( - () -> prestoAction.execute(query, DETERMINISM_ANALYSIS_SETUP), + () -> helperAction.execute(query, DETERMINISM_ANALYSIS_SETUP), stats -> stats.getQueryStats().map(QueryStats::getQueryId).ifPresent(run::addSetupQueryId))); runAndConsume( - () -> prestoAction.execute(queryBundle.getQuery(), DETERMINISM_ANALYSIS_MAIN), + () -> actionForQuery.execute(queryBundle.getQuery(), DETERMINISM_ANALYSIS_MAIN), stats -> stats.getQueryStats().map(QueryStats::getQueryId).ifPresent(run::setQueryId)); // Run checksum query Query checksumQuery = checksumValidator.generateChecksumQuery(queryBundle.getObjectName(), columns, Optional.empty()); ChecksumResult testChecksum = getOnlyElement(callAndConsume( - () -> prestoAction.execute(checksumQuery, DETERMINISM_ANALYSIS_CHECKSUM, ChecksumResult::fromResultSet), + () -> helperAction.execute(checksumQuery, DETERMINISM_ANALYSIS_CHECKSUM, ChecksumResult::fromResultSet), stats -> stats.getQueryStats().map(QueryStats::getQueryId).ifPresent(run::setChecksumQueryId)).getResults()); - DeterminismAnalysis analysis = matchResultToDeterminism(match(DataMatchResult.DataType.DATA, checksumValidator, columns, columns, controlChecksum, testChecksum)); + DeterminismAnalysis analysis = matchResultToDeterminism(match(DataMatchResult.DataType.DATA, checksumValidator, columns, columns, referenceChecksum, testChecksum)); if (analysis != DETERMINISTIC) { return analysis; } @@ -163,7 +166,7 @@ private DeterminismAnalysis analyze(QueryObjectBundle control, ChecksumResult co finally { if (runTeardown) { queryRuns.forEach((queryBundle, run) -> teardownSafely( - prestoAction, + helperAction, Optional.of(queryBundle), queryStats -> queryStats.getQueryStats().map(QueryStats::getQueryId).ifPresent(run::addTeardownQueryId))); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java index d1a35fdd88b07..2d681e7e581e0 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExplainVerification.java @@ -122,7 +122,7 @@ protected ExplainMatchResult verify( } @Override - protected DeterminismAnalysisDetails analyzeDeterminism(QueryBundle control, ExplainMatchResult matchResult) + protected DeterminismAnalysisDetails analyzeDeterminism(QueryBundle controlObject, QueryBundle testObject, ExplainMatchResult matchResult) { throw new UnsupportedOperationException("analyzeDeterminism is not supported for ExplainVerification"); } diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExtendedVerification.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExtendedVerification.java index fee119d50360f..4e74374c26564 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExtendedVerification.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/ExtendedVerification.java @@ -163,6 +163,7 @@ private Optional verifyPartition( PARTITION_DATA, PARTITION_COUNT_MISMATCH, Optional.empty(), + Optional.empty(), OptionalLong.of(controlPartitionChecksum.size()), OptionalLong.of(testPartitionChecksum.size()), ImmutableList.of())); @@ -183,6 +184,7 @@ private Optional verifyPartition( PARTITION_DATA, MATCH, Optional.empty(), + Optional.empty(), OptionalLong.of(controlPartitionChecksum.size()), OptionalLong.of(testPartitionChecksum.size()), ImmutableList.of())); @@ -212,6 +214,7 @@ private Optional verifyBucket( BUCKET_DATA, BUCKET_COUNT_MISMATCH, Optional.empty(), + Optional.empty(), OptionalLong.of(controlBucketChecksum.size()), OptionalLong.of(testBucketChecksum.size()), ImmutableList.of())); @@ -232,6 +235,7 @@ private Optional verifyBucket( BUCKET_DATA, MATCH, Optional.empty(), + Optional.empty(), OptionalLong.of(controlBucketChecksum.size()), OptionalLong.of(testBucketChecksum.size()), ImmutableList.of())); diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java index 91acb60b5c1ac..5f297707ce8b0 100644 --- a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java +++ b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/VerifierConfig.java @@ -61,6 +61,7 @@ public class VerifierConfig private boolean teardownOnMainClusters = true; private boolean skipControl; private boolean skipChecksum; + private boolean runDeterminismAnalysisOnTest; private boolean concurrentControlAndTest; private boolean explain; @@ -314,7 +315,7 @@ public boolean isSetupOnMainClusters() return setupOnMainClusters; } - @ConfigDescription("If true, run control/test setup queries on control/test clusters. Otherwise, run setup queries on the help cluster.") + @ConfigDescription("If true, run control/test setup queries on control/test clusters. Otherwise, run setup queries on the help cluster.X") @Config("setup-on-main-clusters") public VerifierConfig setSetupOnMainClusters(boolean setupOnMainClusters) { @@ -361,6 +362,19 @@ public boolean isSkipChecksum() return skipChecksum; } + @ConfigDescription("Run Determinism Analysis on test rather than control. If the Determinism Analysis returns 'non-deterministic', we then re-run it in control") + @Config("run-determinism-analysis-on-test") + public VerifierConfig setRunDeterminismAnalysisOnTest(boolean runDeterminismAnalysisOnTest) + { + this.runDeterminismAnalysisOnTest = runDeterminismAnalysisOnTest; + return this; + } + + public boolean isRunDeterminismAnalysisOnTest() + { + return runDeterminismAnalysisOnTest; + } + public boolean isExplain() { return explain; diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java index 017f176ca8cc8..9026af24918b5 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/AbstractVerificationTest.java @@ -69,6 +69,7 @@ public abstract class AbstractVerificationTest protected static final ParsingOptions PARSING_OPTIONS = ParsingOptions.builder().setDecimalLiteralTreatment(AS_DOUBLE).build(); protected static final String CONTROL_TABLE_PREFIX = "tmp_verifier_c"; protected static final String TEST_TABLE_PREFIX = "tmp_verifier_t"; + protected static final int DETERMINISM_ANALYSIS_RUNS = 3; protected static VerificationSettings concurrentControlAndTestSettings; protected static VerificationSettings skipControlSettings; @@ -83,7 +84,7 @@ public abstract class AbstractVerificationTest private final SqlParser sqlParser = new SqlParser(new SqlParserOptions().allowIdentifierSymbol(COLON, AT_SIGN)); private final BlockEncodingSerde blockEncodingSerde = new BlockEncodingManager(); private final PrestoExceptionClassifier exceptionClassifier = PrestoExceptionClassifier.defaultBuilder().build(); - private final DeterminismAnalyzerConfig determinismAnalyzerConfig = new DeterminismAnalyzerConfig().setMaxAnalysisRuns(3).setRunTeardown(true); + private final DeterminismAnalyzerConfig determinismAnalyzerConfig = new DeterminismAnalyzerConfig().setMaxAnalysisRuns(DETERMINISM_ANALYSIS_RUNS).setRunTeardown(true); private final FailureResolverManagerFactory failureResolverManagerFactory; public AbstractVerificationTest() @@ -218,6 +219,7 @@ private Optional verify( settings.runningMode.ifPresent(verifierConfig::setRunningMode); settings.saveSnapshot.ifPresent(verifierConfig::setSaveSnapshot); settings.functionSubstitutes.ifPresent(verifierConfig::setFunctionSubstitutes); + settings.runDeterminismAnalysisOnTest.ifPresent(verifierConfig::setRunDeterminismAnalysisOnTest); }); QueryRewriteConfig controlRewriteConfig = new QueryRewriteConfig().setTablePrefix(CONTROL_TABLE_PREFIX); QueryRewriteConfig testRewriteConfig = new QueryRewriteConfig().setTablePrefix(TEST_TABLE_PREFIX); @@ -261,6 +263,7 @@ public VerificationSettings() saveSnapshot = Optional.empty(); functionSubstitutes = Optional.empty(); reuseTable = Optional.empty(); + runDeterminismAnalysisOnTest = Optional.empty(); } Optional concurrentControlAndTest; @@ -269,6 +272,7 @@ public VerificationSettings() Optional saveSnapshot; Optional functionSubstitutes; Optional reuseTable; + Optional runDeterminismAnalysisOnTest; } public static class MockSnapshotSupplierAndConsumer diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java index 29467e3de3fb9..71fa5f5dad021 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestDataVerification.java @@ -46,6 +46,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; @Test(singleThreaded = true) @@ -272,6 +273,70 @@ public void testNonDeterministic() assertDeterminismAnalysisRun(runs.get(0), true); } + @Test + public void testDeterminismAnalysisOnControlAndTest() + { + Optional event; + List runs; + + // Control and test are stable, same results. + // No need for determinism analysis. Status is SUCCEEDED. + event = runVerification("SELECT 2.0", "SELECT 2.0"); + assertTrue(event.isPresent()); + assertEquals(event.get().getStatus(), SUCCEEDED.name()); + assertNull(event.get().getDeterminismAnalysisDetails()); + + // Control is non-deterministic, test is stable, different results. + // Determinism analysis found query to be non-deterministic in 1 run. Status is SKIPPED. + event = runVerification("SELECT rand()", "SELECT 2.0"); + assertTrue(event.isPresent()); + assertEquals(event.get().getStatus(), SKIPPED.name()); + assertEquals(event.get().getSkippedReason(), NON_DETERMINISTIC.name()); + runs = event.get().getDeterminismAnalysisDetails().getRuns(); + assertEquals(runs.size(), 1); + assertEquals(runs.get(0).getClusterType(), ClusterType.CONTROL.name()); + + // Control is stable, test is non-deterministic, different results. + // Determinism analysis found query to be deterministic in 3 runs. Status is FAILED. + event = runVerification("SELECT 2.0", "SELECT rand()"); + assertTrue(event.isPresent()); + assertEquals(event.get().getStatus(), FAILED.name()); + runs = event.get().getDeterminismAnalysisDetails().getRuns(); + assertEquals(runs.size(), DETERMINISM_ANALYSIS_RUNS); + for (DeterminismAnalysisRun run : runs) { + assertEquals(runs.get(0).getClusterType(), ClusterType.CONTROL.name()); + } + + // From this moment determinism analysis will run on TEST, not CONTROL and will rerun on CONTROL, + // if found non-deterministic on TEST. + VerificationSettings settings = new VerificationSettings(); + settings.runDeterminismAnalysisOnTest = Optional.of(true); + + // Control is non-deterministic, test is stable, different results. + // Determinism analysis found query to be deterministic in 3 runs on TEST. Status is FAILED. + event = runVerification("SELECT rand()", "SELECT 2.0", settings); + assertTrue(event.isPresent()); + assertEquals(event.get().getStatus(), FAILED.name()); + runs = event.get().getDeterminismAnalysisDetails().getRuns(); + assertEquals(runs.size(), DETERMINISM_ANALYSIS_RUNS); + for (DeterminismAnalysisRun run : runs) { + assertEquals(runs.get(0).getClusterType(), ClusterType.TEST.name()); + } + + // Control is stable, test is non-deterministic, different results. + // First determinism analysis found query to be non-deterministic in 1 run on TEST. + // Second determinism analysis found query to be deterministic in 3 runs on CONTROL. Status is FAILED. + event = runVerification("SELECT 2.0", "SELECT rand()", settings); + assertTrue(event.isPresent()); + assertEquals(event.get().getStatus(), FAILED.name()); + runs = event.get().getDeterminismAnalysisDetails().getRuns(); + assertEquals(runs.size(), DETERMINISM_ANALYSIS_RUNS + 1); + assertEquals(runs.get(0).getClusterType(), ClusterType.TEST.name()); + for (int i = 1; i < runs.size(); i++) { + assertEquals(runs.get(i).getClusterType(), ClusterType.CONTROL.name()); + } + } + @Test public void testArrayOfRow() { diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java index 077ce3052b38d..ccf5f21cdc8bf 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/framework/TestVerifierConfig.java @@ -48,6 +48,7 @@ public void testDefault() .setTeardownOnMainClusters(true) .setSkipControl(false) .setSkipChecksum(false) + .setRunDeterminismAnalysisOnTest(false) .setExplain(false) .setConcurrentControlAndTest(false) .setRunningMode("control-test") @@ -81,6 +82,7 @@ public void testExplicitPropertyMappings() .put("teardown-on-main-clusters", "false") .put("skip-control", "true") .put("skip-checksum", "true") + .put("run-determinism-analysis-on-test", "true") .put("explain", "true") .put("concurrent-control-and-test", "true") .put("running-mode", "query-bank") @@ -110,6 +112,7 @@ public void testExplicitPropertyMappings() .setTeardownOnMainClusters(false) .setSkipControl(true) .setSkipChecksum(true) + .setRunDeterminismAnalysisOnTest(true) .setExplain(true) .setConcurrentControlAndTest(true) .setRunningMode("query-bank") diff --git a/presto-verifier/src/test/java/com/facebook/presto/verifier/resolver/FailureResolverTestUtil.java b/presto-verifier/src/test/java/com/facebook/presto/verifier/resolver/FailureResolverTestUtil.java index a8c48c42b6f65..f6a77da93a382 100644 --- a/presto-verifier/src/test/java/com/facebook/presto/verifier/resolver/FailureResolverTestUtil.java +++ b/presto-verifier/src/test/java/com/facebook/presto/verifier/resolver/FailureResolverTestUtil.java @@ -40,7 +40,7 @@ public static ColumnMatchResult createMismatchedCo public static DataMatchResult createMatchResult(ColumnMatchResult... mismatchedColumns) { - return new DataMatchResult(DATA, COLUMN_MISMATCH, Optional.empty(), OptionalLong.of(1L), OptionalLong.of(1L), asList(mismatchedColumns)); + return new DataMatchResult(DATA, COLUMN_MISMATCH, Optional.empty(), Optional.empty(), OptionalLong.of(1L), OptionalLong.of(1L), asList(mismatchedColumns)); } public static SqlVarbinary binary(int data)