[ 
https://issues.apache.org/jira/browse/DRILL-5423?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15994210#comment-15994210
 ] 

ASF GitHub Bot commented on DRILL-5423:
---------------------------------------

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

    https://github.com/apache/drill/pull/811#discussion_r114465900
  
    --- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/ops/AbstractOperatorExecContext.java
 ---
    @@ -0,0 +1,94 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + * http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.drill.exec.ops;
    +
    +import org.apache.drill.exec.memory.BufferAllocator;
    +import org.apache.drill.exec.physical.base.PhysicalOperator;
    +import org.apache.drill.exec.testing.ExecutionControls;
    +
    +import io.netty.buffer.DrillBuf;
    +
    +/**
    + * Implementation of {@link OperatorExecContext} that provides services
    + * needed by most run-time operators. Excludes services that need the
    + * entire Drillbit. Allows easy testing of operator code that uses this
    + * interface.
    + */
    +
    +public class AbstractOperatorExecContext implements OperatorExecContext {
    +
    +  protected final BufferAllocator allocator;
    +  protected final ExecutionControls executionControls;
    +  protected final PhysicalOperator popConfig;
    +  protected final BufferManager manager;
    +  protected OperatorStatReceiver statsWriter;
    +
    +  public AbstractOperatorExecContext(BufferAllocator allocator, 
PhysicalOperator popConfig,
    +                                     ExecutionControls executionControls,
    +                                     OperatorStatReceiver stats) {
    +    this.allocator = allocator;
    +    this.popConfig = popConfig;
    +    this.manager = new BufferManagerImpl(allocator);
    +    statsWriter = stats;
    +
    +    this.executionControls = executionControls;
    +  }
    +
    +  @Override
    +  public DrillBuf replace(DrillBuf old, int newSize) {
    +    return manager.replace(old, newSize);
    +  }
    +
    +  @Override
    +  public DrillBuf getManagedBuffer() {
    +    return manager.getManagedBuffer();
    +  }
    +
    +  @Override
    +  public DrillBuf getManagedBuffer(int size) {
    +    return manager.getManagedBuffer(size);
    +  }
    +
    +  @Override
    +  public ExecutionControls getExecutionControls() {
    +    return executionControls;
    +  }
    +
    +  @Override
    +  public BufferAllocator getAllocator() {
    +    if (allocator == null) {
    +      throw new UnsupportedOperationException("Operator context does not 
have an allocator");
    +    }
    +    return allocator;
    +  }
    +
    +  public void close() {
    +    try {
    +      manager.close();
    +    } finally {
    +      if (allocator != null) {
    +        allocator.close();
    +      }
    +    }
    +  }
    +
    +  @Override
    +  public OperatorStatReceiver getStatsWriter() {
    +    return statsWriter;
    --- End diff --
    
    The stats writer is a required part of the operator exec context. Said 
another way, this is the context that operators use for low-level (non-network) 
services. The stats writer must exist -- either in production code (normal use) 
or in a test fixture (for unit tests.)
    
    I can make one change, make the member variable final to show that it must 
be set to one and only one value at construction time.


> Refactor ScanBatch to allow unit testing record readers
> -------------------------------------------------------
>
>                 Key: DRILL-5423
>                 URL: https://issues.apache.org/jira/browse/DRILL-5423
>             Project: Apache Drill
>          Issue Type: Improvement
>    Affects Versions: 1.9.0
>            Reporter: Paul Rogers
>            Assignee: Paul Rogers
>            Priority: Minor
>             Fix For: 1.11.0
>
>
> A recent set of PRs refactored some of the "context" code to allow easier 
> unit testing of operator internals.
> A recent attempt to help a community user revealed that it would be helpful 
> to refactor parts of {{ScanBatch}} and {{OperatorContext}} to allow unit 
> testing of reader code. In particular:
> * Make {{ScanBatch.Mutator}} into a static class that can be created in a 
> unit test separate from a {{ScanBatch}} (and the rest of Drill.)
> * Split {{OperatorContext}} into a execution-only {{OperatorExecContext}} 
> interface that can be used in testing. Leave in {{OperatorContext}} the 
> methods that require all of Drill to be present.
> Doing the above requires a bit of implementation work:
> * Change {{OperatorContext}} from an abstract class to an interface so that 
> it can extend {OperatorExceContext}}.
> * {{OperatorContext}} appears to be a class so that it can hold a static 
> method. (Java 8 allows static methods on interfaces, but Drill uses Java 7 
> which does not have such support.) Move this method to a new 
> {{OperatorUtilities}} class and fix up references.
> * Split the {{OperatorContextImpl}} class into two parts. Move into a new 
> {{AbstractOperatorContext}} class the code which implements the low-level 
> runtime methods. Leave in the original class the functionality that depends 
> on the rest of Drill (such as references to the {{DrillbitContext}}.)
> * Add a method to the new {{OperatorFixture}} class to create a test-time 
> version of the {{OperatorExecContext}} interface.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Reply via email to