Re: Review Request 30701: DRILL-2173 partition queries for dynamic partition pruning

[Jason Altekruse <al...@gmail.com>]

> 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
> 
>