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