Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/1059#discussion_r158593845
  
    --- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestNestedLoopJoin.java
 ---
    @@ -333,4 +341,74 @@ public void testNlJoinWithLargeRightInputSuccess() 
throws Exception {
           test(RESET_JOIN_OPTIMIZATION);
         }
       }
    +
    +  private void buildFile(String fileName, String[] data, File testDir) 
throws IOException {
    +    try(PrintWriter out = new PrintWriter(new FileWriter(new File(testDir, 
fileName)))) {
    +      for (String line : data) {
    +        out.println(line);
    +      }
    +    }
    +  }
    +
    +  @Test
    +  public void testNestedLeftJoinWithEmptyTable() throws Exception {
    +    setSessionOption("planner.enable_join_optimization", false);
    +    setSessionOption("planner.enable_nljoin_for_scalar_only", false);
    +    setSessionOption("planner.enable_hashjoin", false);
    +    setSessionOption("planner.enable_mergejoin", false);
    +    try (ClusterFixture cluster = 
ClusterFixture.builder(dirTestWatcher).build();
    --- End diff --
    
    The above certainly does not do what one would expect. I suspect there is a 
bit of confusion. Not surprising, we don't document our test structure so we 
learn by trial and error.
    
    This test class derives from `PlanTestBase` which derives from 
`BaseTestQuery`. `BaseTestQuery` starts a server before running the tests.
    
    The `setSessionOption()` methods here sets options on the server created by 
`BaseTestQuery`. So far so good.
    
    But, the code in this tests then uses `ClusterFixture` to start a new 
server. This means:
    
    * Two servers are running.
    * The session options were set in the first.
    * The test was run against the second.
    
    In general, there are two equally valid choices.
    
    1. Run the tests against the server started by the test class.
    2. Change the test superclass to ClusterTest (to use a shared server) or 
DrillTest (to start the server in each test.)
    
    Here, we are adding to existing tests, so we should use the existing 
structure and use the server started by the test class.
    
    In another scenario, you might want to start a server for each test. If so, 
then you can pass the session options to the cluster builder so they are set 
when the server starts. (Not needed here, but handy to remember for other 
tests.)


---

Reply via email to