drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Altekruse" <altekruseja...@gmail.com>
Subject Re: Review Request 30701: DRILL-2173 partition queries for dynamic partition pruning
Date Tue, 03 Mar 2015 20:13:59 GMT


> On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DirectoryExplorers.java,
line 71
> > <https://reviews.apache.org/r/30701/diff/3/?file=853387#file853387line71>
> >
> >     Is there a reason to use java.io. here?

In the compiled classes we do not include the merged imports list (I do not know the reason
for this). You can see references to static helper methods, for example several of the UDFs
defined in StringFunctions refernce regex classes/utilties or a Drill specific toStringFromUTF8
method from Drill using the fully qualified name.


> On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DirectoryExplorers.java,
line 79
> > <https://reviews.apache.org/r/30701/diff/3/?file=853387#file853387line79>
> >
> >     Why use this fully qualified name instead of just MAXDIR_NAME? Same below.

see above.


> On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionExplorer.java,
line 50
> > <https://reviews.apache.org/r/30701/diff/3/?file=853393#file853393line50>
> >
> >     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.

That sounds good to me. To allow for use of the extended for loop by consumers of the interface
I will have it return Iterable instead.


> On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DirectoryExplorers.java,
line 106
> > <https://reviews.apache.org/r/30701/diff/3/?file=853387#file853387line106>
> >
> >     No blank line here.

fixed


> On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginPartitionExplorer.java,
line 50
> > <https://reviews.apache.org/r/30701/diff/3/?file=853396#file853396line50>
> >
> >     Same comment as before re the array of Strings.

Fixed


- Jason


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


On Feb. 28, 2015, 12:18 a.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30701/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2015, 12:18 a.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