[GitHub] [drill] vdiravka commented on a change in pull request #1687: DRILL-2326: Fix scalar replacement for the case when static method which does not return values is called

[GitBox <gi...@apache.org>]

vdiravka commented on a change in pull request #1687: DRILL-2326: Fix scalar replacement for the case when static method which does not return values is called
URL: https://github.com/apache/drill/pull/1687#discussion_r264330989
 
 

 ##########
 File path: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java
 ##########
 @@ -536,68 +527,27 @@ public void testUTF8() throws Throwable {
     verifyPhysicalPlan("convert_to('apache_drill', 'UTF8')", new byte[] {'a', 'p', 'a', 'c', 'h', 'e', '_', 'd', 'r', 'i', 'l', 'l'});
   }
 
-  @Ignore // TODO(DRILL-2326) remove this when we get rid of the scalar replacement option test cases below
-  @Test
+  @Test // DRILL-2326
   public void testBigIntVarCharReturnTripConvertLogical() throws Exception {
-    final String logicalPlan = Resources.toString(
+    String logicalPlan = Resources.toString(
         Resources.getResource(CONVERSION_TEST_LOGICAL_PLAN), Charsets.UTF_8);
-    final List<QueryDataBatch> results =  testLogicalWithResults(logicalPlan);
-    int count = 0;
-    final RecordBatchLoader loader = new RecordBatchLoader(getAllocator());
-    for (QueryDataBatch result : results) {
-      count += result.getHeader().getRowCount();
-      loader.load(result.getHeader().getDef(), result.getData());
-      if (loader.getRecordCount() > 0) {
-        VectorUtil.logVectorAccessibleContent(loader);
-      }
-      loader.clear();
-      result.release();
-    }
-    assertTrue(count == 10);
-  }
 
+    List<String> compilers = Arrays.asList(ClassCompilerSelector.CompilerPolicy.JANINO.name(),
+      ClassCompilerSelector.CompilerPolicy.JDK.name());
 
-  @Test // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
-  public void testBigIntVarCharReturnTripConvertLogical_ScalarReplaceTRY() throws Exception {
-    final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ScalarReplacementOption.TRY);
-    try {
-      // this should work fine
-      testBigIntVarCharReturnTripConvertLogical();
-    } finally {
-      // restore the system option
-      QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
-    }
-  }
-
-  @Test // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
-  @Ignore // Because this test sometimes fails, sometimes succeeds
-  public void testBigIntVarCharReturnTripConvertLogical_ScalarReplaceON() throws Exception {
-    final OptionValue srOption = QueryTestUtil.setupScalarReplacementOption(bits[0], ScalarReplacementOption.ON);
-    boolean caughtException = false;
     try {
-      // this used to fail (with a JUnit assertion) until we fix the SR bug
-      // Something in DRILL-5116 seemed to fix this problem, so the test now
-      // succeeds - sometimes.
-      testBigIntVarCharReturnTripConvertLogical();
-    } catch(RpcException e) {
-      caughtException = true;
-    } finally {
-      QueryTestUtil.restoreScalarReplacementOption(bits[0], srOption.string_val);
-    }
-
-    // Yes: sometimes this works, sometimes it does not...
-    assertTrue(!caughtException || caughtException);
-  }
+      setSessionOption(ExecConstants.SCALAR_REPLACEMENT_OPTION, ClassTransformer.ScalarReplacementOption.ON.name());
+      setSessionOption(ClassCompilerSelector.JAVA_COMPILER_DEBUG_OPTION, false);
+      for (String compilerName : compilers) {
+        setSessionOption(ClassCompilerSelector.JAVA_COMPILER_OPTION, compilerName);
 
-  @Test // TODO(DRILL-2326) temporary until we fix the scalar replacement bug for this case
-  public void testBigIntVarCharReturnTripConvertLogical_ScalarReplaceOFF() throws Exception {
 
 Review comment:
   Why these tests are no longer needed? 
   `testBigIntVarCharReturnTripConvertLogical_ScalarReplaceOFF`
   `testBigIntVarCharReturnTripConvertLogical_ScalarReplaceON`
   `testBigIntVarCharReturnTripConvertLogical_ScalarReplaceTRY`

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services