fix: erroneous QueryCompiler retry when nothing unaccounted for (deep…

…haven#6451) Fixes deephaven#6216
malhotrashivam · Dec 2, 2024 · 3827ba0 · 3827ba0
1 parent 0f761b8
commit 3827ba0
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 15 deletions.
diff --git a/engine/context/src/test/java/io/deephaven/engine/context/TestQueryCompiler.java b/engine/context/src/test/java/io/deephaven/engine/context/TestQueryCompiler.java
@@ -264,18 +264,17 @@ public void testCollidingCompile() throws Exception {
 
     @Test
     public void testMultiCompileWithFailure() throws ExecutionException, InterruptedException {
-        final String goodProgram = String.join(
-                "\n",
+        final String goodProgram = String.join("\n",
                 "public class GoodTest {",
                 "   public static void main (String [] args) {",
                 "   }",
                 "}");
-        final String badProgram = String.join(
-                "\n",
-                "public class BadTest {",
-                "   public static void main (String [] args) {",
-                "   }",
-                "}}");
+        final String badProgram = String.join("\n",
+                "public class Formula {",
+                "    public Formula() {",
+                "        S.badCall(0);",
+                "    }",
+                "}");
 
         QueryCompilerRequest[] requests = new QueryCompilerRequest[] {
                 QueryCompilerRequest.builder()
@@ -299,15 +298,53 @@ public void testMultiCompileWithFailure() throws ExecutionException, Interrupted
                         CompletionStageFuture.make(),
                 };
 
-        try {
-            ExecutionContext.getContext().getQueryCompiler().compile(requests, resolvers);
-            // noinspection DataFlowIssue
-            throw Assert.statementNeverExecuted();
-        } catch (Exception ignored) {
-        }
+        ExecutionContext.getContext().getQueryCompiler().compile(requests, resolvers);
 
         Assert.eqTrue(resolvers[0].getFuture().isDone(), "resolvers[0].getFuture().isDone()");
         Assert.eqTrue(resolvers[1].getFuture().isDone(), "resolvers[0].getFuture().isDone()");
         Assert.neqNull(resolvers[1].getFuture().get(), "resolvers[1].getFuture().get()");
     }
+
+    @Test
+    public void testMultiCompileWithFailureSecond() throws ExecutionException, InterruptedException {
+        final String badProgram = String.join("\n",
+                "public class Formula {",
+                "    public Formula() {",
+                "        S.badCall(0);",
+                "    }",
+                "}");
+        final String goodProgram = String.join("\n",
+                "public class Formula {",
+                "   public static void main (String [] args) {",
+                "   }",
+                "}");
+
+        QueryCompilerRequest[] requests = new QueryCompilerRequest[] {
+                QueryCompilerRequest.builder()
+                        .description("Test Good Compile")
+                        .className("Formula")
+                        .classBody(goodProgram)
+                        .packageNameRoot("com.deephaven.test")
+                        .build(),
+                QueryCompilerRequest.builder()
+                        .description("Test Bad Compile")
+                        .className("Formula")
+                        .classBody(badProgram)
+                        .packageNameRoot("com.deephaven.test")
+                        .build(),
+        };
+
+        // noinspection unchecked
+        CompletionStageFuture.Resolver<Class<?>>[] resolvers =
+                (CompletionStageFuture.Resolver<Class<?>>[]) new CompletionStageFuture.Resolver[] {
+                        CompletionStageFuture.make(),
+                        CompletionStageFuture.make(),
+                };
+
+        ExecutionContext.getContext().getQueryCompiler().compile(requests, resolvers);
+
+        Assert.eqTrue(resolvers[1].getFuture().isDone(), "resolvers[0].getFuture().isDone()");
+        Assert.eqTrue(resolvers[0].getFuture().isDone(), "resolvers[0].getFuture().isDone()");
+        Assert.neqNull(resolvers[0].getFuture().get(), "resolvers[1].getFuture().get()");
+    }
 }
diff --git a/engine/table/src/main/java/io/deephaven/engine/context/QueryCompilerImpl.java b/engine/table/src/main/java/io/deephaven/engine/context/QueryCompilerImpl.java
@@ -1004,7 +1004,7 @@ private boolean doCreateClassesSingleRound(
             }
         });
 
-        return wantRetry;
+        return wantRetry && !toRetry.isEmpty();
     }
 
     /**
-Original file line number
+Diff line change
@@ Expand Up / @@ -1004,7 +1004,7 @@ private boolean doCreateClassesSingleRound( @@
                 }
             });
-            return wantRetry;
+            return wantRetry && !toRetry.isEmpty();
         }
         /**
@@ Expand Down @@