drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 30701: DRILL-2173 partition queries for dynamic partition pruning
Date Sat, 28 Feb 2015 00:26:04 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30701/#review74629
-----------------------------------------------------------



exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DirectoryExplorers.java
<https://reviews.apache.org/r/30701/#comment121249>

    Is there a reason to use java.io. here?



exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DirectoryExplorers.java
<https://reviews.apache.org/r/30701/#comment121250>

    Why use this fully qualified name instead of just MAXDIR_NAME? Same below.



exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DirectoryExplorers.java
<https://reviews.apache.org/r/30701/#comment121251>

    No blank line here.



exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java
<https://reviews.apache.org/r/30701/#comment121252>

    Capture f.getType() once in a final local before the first conditional. Then, it should
be ok to directly check for equality using == against the classes -- they'll be the same class
reference in the class loader. No need to use equals() in this case.



exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java
<https://reviews.apache.org/r/30701/#comment121253>

    To avoid future confusion with these, can we add units to the ends of the names, e.g.,
INITIAL_OFF_HEAP_ALLOCATION_MB, or whatever it is?



exec/java-exec/src/main/java/org/apache/drill/exec/ops/UdfUtilities.java
<https://reviews.apache.org/r/30701/#comment121256>

    Formatting: after the opening brace, use a newline, and line these up one after the other
with regular function-style indenting:
    
    public static final Class[] INJECTABLE_TYPES = {
      DrillBuf.class,
      QueryDateTimeInfo.class,
      PartitionExplorer.class,
      };



exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionExplorer.java
<https://reviews.apache.org/r/30701/#comment121258>

    Can we have an interface level comment that describes what the interface is for?



exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionExplorer.java
<https://reviews.apache.org/r/30701/#comment121259>

    No blank line here.



exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionExplorer.java
<https://reviews.apache.org/r/30701/#comment121260>

    There should also be an @throws line for the exception you declared, explaining the conditions
which will cause the exception.



exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionExplorer.java
<https://reviews.apache.org/r/30701/#comment121276>

    If this could be the set of all the files in a directory, it could be really large. Would
it be better to define this to return an Iterator<String>? That doesn't necessarily
mean the implementations have to change (the interator might just iterate over an array that's
generated internally), but it would give implementations that would have a large result set
more flexibility in how they work, and not necessarily require materializing a large result
set all at once.



exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionNotFoundException.java
<https://reviews.apache.org/r/30701/#comment121261>

    There's no need to explicitly show super().



exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginPartitionExplorer.java
<https://reviews.apache.org/r/30701/#comment121262>

    @throws PartitionNotFoundException when ....



exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginPartitionExplorer.java
<https://reviews.apache.org/r/30701/#comment121277>

    Same comment as before re the array of Strings.



exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
<https://reviews.apache.org/r/30701/#comment121264>

    Make these two values final.



exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
<https://reviews.apache.org/r/30701/#comment121263>

    If this try block is really just to protect yourself against a failure of 'new String(temp,
"UTF-8")', then you should just protect that in order to avoid causing confusion if plugins.get()
might also be able to throw UnsupportedEncodingException:
    
    String pluginName;
    try {
      pluginName = new String(temp, "UTF-8");
    } catch (UnsupportedEncodingException e) {
      throw new RuntimeException("UTF-8 encoding unavailable", e);
    }
    
    return plugins.get(pluginName).getSubpartitions(...);



exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
<https://reviews.apache.org/r/30701/#comment121267>

    make currentInput and temp final.



exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
<https://reviews.apache.org/r/30701/#comment121269>

    Same comment as for the handling of this in the StoragePluginRegistry. Since the pattern
repeats here two more times, it would be best to capture that and reuse it. I suggest a public
static function, possibly on StoragePluginRegistry (or some similar appropriate place) that
does this:
    
    public static String getStringFromHolder(final VarCharHolder holder) {
      final byte[] temp = new byte[currentInput.end - currentInput.start];
      currentInput.buffer.getBytes(0, temp, 0, currentInput.end - currentInput.start);
      String s;
      try {
          s = ...;
      } catch (...) {
         ...
      }
      return s;
    }
    
    Then, in this, and the function above, you can use
    final String pluginName = getStringFromHolder(workspace); // or whatever



exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
<https://reviews.apache.org/r/30701/#comment121270>

    Can this be final?



exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
<https://reviews.apache.org/r/30701/#comment121271>

    Move the declaration to here, and make it final.


- Chris Westin


On Feb. 27, 2015, 4:18 p.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30701/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2015, 4:18 p.m.)
> 
> 
> Review request for drill, Jacques Nadeau, Mehant Baid, and Parth Chandra.
> 
> 
> Bugs: DRILL-2173
>     https://issues.apache.org/jira/browse/DRILL-2173
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> Adds a new interface for UDFs to access partition information. Together with 2060 which
allows constant expression folding this will allow UDFs that
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java 279c428

>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionConverter.java 0127e6e

>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DirectoryExplorers.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java
0fe36cb 
>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java e413921

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java c881432 
>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/UdfUtilities.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/store/AbstractStoragePlugin.java
b032fce 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionExplorer.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionNotFoundException.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePlugin.java ef5978c

>   exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginPartitionExplorer.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
5d0eed6 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java
7a1f61e 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/ExpressionInterpreterTest.java
PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/exec/fn/interp/TestConstantFolding.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/30701/diff/
> 
> 
> Testing
> -------
> 
> Some unit tests on the functionality have been run, still a work in progress so no full
mvn build run yet
> 
> 
> Thanks,
> 
> Jason Altekruse
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message