> On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java, > > line 67 > > <https://reviews.apache.org/r/30701/diff/3/?file=853390#file853390line67> > > > > 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?
I had to move this change into an earlier patch. I have made the correction for the name with appropriate units there > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/UdfUtilities.java, > > line 31 > > <https://reviews.apache.org/r/30701/diff/3/?file=853391#file853391line31> > > > > 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, > > }; fixed > 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 24 > > <https://reviews.apache.org/r/30701/diff/3/?file=853393#file853393line24> > > > > Can we have an interface level comment that describes what the > > interface is for? Added an explanation with the complete use case (might be a bit much, but the lack of related docs to refer to makes this a bit hard to describe succinctly). > 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 25 > > <https://reviews.apache.org/r/30701/diff/3/?file=853393#file853393line25> > > > > 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/PartitionExplorer.java, > > line 48 > > <https://reviews.apache.org/r/30701/diff/3/?file=853393#file853393line48> > > > > There should also be an @throws line for the exception you declared, > > explaining the conditions which will cause the exception. fixed > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/PartitionNotFoundException.java, > > line 23 > > <https://reviews.apache.org/r/30701/diff/3/?file=853394#file853394line23> > > > > There's no need to explicitly show super(). 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 49 > > <https://reviews.apache.org/r/30701/diff/3/?file=853396#file853396line49> > > > > @throws PartitionNotFoundException when .... fixed > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java, > > line 306 > > <https://reviews.apache.org/r/30701/diff/3/?file=853397#file853397line306> > > > > Make these two values final. Fixed > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java, > > line 309 > > <https://reviews.apache.org/r/30701/diff/3/?file=853397#file853397line309> > > > > 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(...); Figred out if I actually pass a charset I don't get the exception. simplifies this in a number of places. > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java, > > line 121 > > <https://reviews.apache.org/r/30701/diff/3/?file=853398#file853398line121> > > > > make currentInput and temp final. refactored, added finals where appropriate in new code. > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java, > > line 130 > > <https://reviews.apache.org/r/30701/diff/3/?file=853398#file853398line130> > > > > 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 Found out there was a function similar to what I wanted in StringFucntionHelpers, I defined a utilty there to convert a VarCharHolder to a string and added it in the various places where this pattern appeared before. > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java, > > line 134 > > <https://reviews.apache.org/r/30701/diff/3/?file=853398#file853398line134> > > > > Can this be final? yes, fixed > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSystemPlugin.java, > > line 144 > > <https://reviews.apache.org/r/30701/diff/3/?file=853398#file853398line144> > > > > Move the declaration to here, and make it final. refactored, put finals in new iterator class that replaces this interface. > On Feb. 28, 2015, 12:26 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java, > > line 107 > > <https://reviews.apache.org/r/30701/diff/3/?file=853388#file853388line107> > > > > 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. 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 > >
