Bug fix in LimitQueryDeterminismAnalyzer

Fix LimitQueryDeterminismAnalyzer to return DETERMINISTIC when row count from the control query is less then the limit of the limit clause. Before the fix, LimitQueryDeterminismAnalyzer concluded ANALYSIS_FAILED_DATA_CHANGED instead.
prestodb · Sep 17, 2020 · ee1627e · ee1627e
1 parent 600c157
commit ee1627e
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 15 deletions.
diff --git a/...to-verifier/src/main/java/com/facebook/presto/verifier/framework/DeterminismAnalyzer.java b/...to-verifier/src/main/java/com/facebook/presto/verifier/framework/DeterminismAnalyzer.java
@@ -115,12 +115,11 @@ private DeterminismAnalysis analyze(DataQueryBundle control, ChecksumResult cont
         switch (limitQueryAnalysis) {
             case NOT_RUN:
             case FAILED_QUERY_FAILURE:
+            case DETERMINISTIC:
                 // try the next analysis
                 break;
             case NON_DETERMINISTIC:
                 return NON_DETERMINISTIC_LIMIT_CLAUSE;
-            case DETERMINISTIC:
-                return DETERMINISTIC;
             case FAILED_DATA_CHANGED:
                 return ANALYSIS_FAILED_DATA_CHANGED;
             default:

diff --git a/...r/src/main/java/com/facebook/presto/verifier/framework/LimitQueryDeterminismAnalyzer.java b/...r/src/main/java/com/facebook/presto/verifier/framework/LimitQueryDeterminismAnalyzer.java
@@ -153,6 +153,9 @@ private LimitQueryDeterminismAnalysis analyzeQuery(Query query)
             return NOT_RUN;
         }
         long limit = parseLong(query.getLimit().get());
+        if (rowCount < limit) {
+            return DETERMINISTIC;
+        }
         Optional<String> newLimit = Optional.of(Long.toString(limit + 1));
         Query newLimitQuery = new Query(query.getWith(), query.getQueryBody(), Optional.empty(), newLimit);
         return analyzeLimitNoOrderBy(newLimitQuery, limit);
@@ -211,6 +214,9 @@ private LimitQueryDeterminismAnalysis analyzeQuerySpecification(Optional<With> w
             return NOT_RUN;
         }
         long limit = parseLong(querySpecification.getLimit().get());
+        if (rowCount < limit) {
+            return DETERMINISTIC;
+        }
         Optional<String> newLimit = Optional.of(Long.toString(limit + 1));
         Optional<OrderBy> orderBy = querySpecification.getOrderBy();
 
@@ -262,7 +268,7 @@ private LimitQueryDeterminismAnalysis analyzeLimitNoOrderBy(Query newLimitQuery,
         if (rowCountHigherLimit == rowCount) {
             return DETERMINISTIC;
         }
-        if (rowCount >= limit && rowCountHigherLimit > rowCount) {
+        if (rowCountHigherLimit > rowCount) {
             return NON_DETERMINISTIC;
         }
         return FAILED_DATA_CHANGED;

diff --git a/...c/test/java/com/facebook/presto/verifier/framework/TestLimitQueryDeterminismAnalyzer.java b/...c/test/java/com/facebook/presto/verifier/framework/TestLimitQueryDeterminismAnalyzer.java
@@ -41,8 +41,6 @@
 import static com.facebook.presto.verifier.framework.LimitQueryDeterminismAnalysis.NON_DETERMINISTIC;
 import static com.facebook.presto.verifier.framework.LimitQueryDeterminismAnalysis.NOT_RUN;
 import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertNull;
 
 @Test(singleThreaded = true)
 public class TestLimitQueryDeterminismAnalyzer
@@ -150,14 +148,16 @@ public void testNonDeterministicLimitNoOrderBy()
     @Test
     public void testDeterministicLimitNoOrderBy()
     {
-        assertAnalysis(createPrestoAction(1000), "INSERT INTO test SELECT * FROM source LIMIT 1000", DETERMINISTIC);
+        MockPrestoAction prestoAction = createPrestoAction(1000);
+        assertAnalysis(prestoAction, "INSERT INTO test SELECT * FROM source LIMIT 1000", 500, DETERMINISTIC, false);
+        assertAnalysis(prestoAction, "INSERT INTO test SELECT * FROM source UNION ALL SELECT * FROM source LIMIT 1000", 500, DETERMINISTIC, false);
+        assertAnalysis(prestoAction, "INSERT INTO test SELECT * FROM source LIMIT 1000", DETERMINISTIC);
     }
 
     @Test
     public void testFailedDataChangedLimitNoOrderBy()
     {
         assertAnalysis(createPrestoAction(999), "INSERT INTO test SELECT * FROM source LIMIT 1000", FAILED_DATA_CHANGED);
-        assertAnalysis(createPrestoAction(1001), "INSERT INTO test SELECT * FROM source LIMIT 2000", FAILED_DATA_CHANGED);
     }
 
     @Test
@@ -187,7 +187,10 @@ public void testLimitOrderByNonDeterministic()
     @Test
     public void testLimitOrderByDeterministic()
     {
-        MockPrestoAction prestoAction = createPrestoAction(ImmutableList.of(ImmutableList.of(1, 2, 3, 4, 5, 1, 6)), TIE_INSPECTOR_COLUMNS);
+        MockPrestoAction prestoAction = createPrestoAction(1000);
+        assertAnalysis(prestoAction, "INSERT INTO test SELECT * FROM source ORDER BY 1 LIMIT 1000", 500, DETERMINISTIC, false);
+
+        prestoAction = createPrestoAction(ImmutableList.of(ImmutableList.of(1, 2, 3, 4, 5, 1, 6)), TIE_INSPECTOR_COLUMNS);
         assertAnalysis(prestoAction, ORDER_BY_LIMIT_QUERY, DETERMINISTIC);
 
         prestoAction = createPrestoAction(
@@ -216,23 +219,23 @@ private static MockPrestoAction createPrestoAction(List<List<Object>> rows, List
     }
 
     private static void assertAnalysis(PrestoAction prestoAction, String query, LimitQueryDeterminismAnalysis expectedAnalysis)
+    {
+        assertAnalysis(prestoAction, query, ROW_COUNT_WITH_LIMIT, expectedAnalysis, expectedAnalysis != NOT_RUN);
+    }
+
+    private static void assertAnalysis(PrestoAction prestoAction, String query, long controlRowCount, LimitQueryDeterminismAnalysis expectedAnalysis, boolean queryRan)
     {
         DeterminismAnalysisDetails.Builder determinismAnalysisDetailsBuilder = DeterminismAnalysisDetails.builder();
         LimitQueryDeterminismAnalysis analysis = new LimitQueryDeterminismAnalyzer(
                 prestoAction,
                 true,
                 sqlParser.createStatement(query, PARSING_OPTIONS),
-                ROW_COUNT_WITH_LIMIT,
+                controlRowCount,
                 determinismAnalysisDetailsBuilder).analyze();
         DeterminismAnalysisDetails determinismAnalysisDetails = determinismAnalysisDetailsBuilder.build(NON_DETERMINISTIC_LIMIT_CLAUSE);
 
         assertEquals(analysis, expectedAnalysis);
-        if (expectedAnalysis == NOT_RUN) {
-            assertNull(determinismAnalysisDetails.getLimitQueryAnalysisQueryId());
-        }
-        else {
-            assertNotNull(determinismAnalysisDetails.getLimitQueryAnalysisQueryId());
-        }
+        assertEquals(determinismAnalysisDetails.getLimitQueryAnalysisQueryId() != null, queryRan);
     }
 
     private static void assertAnalyzerQuery(MockPrestoAction prestoAction, String expectedQuery)