> On March 10, 2015, 9:06 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java, > > line 26 > > <https://reviews.apache.org/r/31871/diff/1/?file=889807#file889807line26> > > > > This should be AutoCloseable, not Closeable. Note that close() throws > > Exception then.
Fixed > On March 10, 2015, 9:06 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java, > > line 103 > > <https://reviews.apache.org/r/31871/diff/1/?file=889809#file889809line103> > > > > Does getAllocator() really need to be public? Unfortunately it will for now. There is a pattern throughout the code to require direct access to an allocator to create a Vector or a ValueHolder of a type that is not stored on heap (such as varchar). The primary interfaces they interact with are defined in TypeHelper, such as getNewVector(). As almost all of these calls are invoked from operators with an allocator available through OperatorContext, there might be a case that we should provide the interface to request these kind of resources directly from their repsective contexts, rather than requesting an allocator and passing it into a static utility method. This would allow us to create a more explicit contract within operators to specifically allow creation of vectors, or managed buffers (mangement might have to change scope from the fragment to Operators themselves to allow more flexibility). Giving direct access to allocators provides everyone with access to the public buffer() methods, while this is currently used sparringl y, we should probably discuss the possibility of standardizing how Operators manage and properly close general purpose buffers that may not be well suited for the current managed buffer pattern used in UDFs. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31871/#review75973 ----------------------------------------------------------- On March 9, 2015, 10:07 p.m., Jason Altekruse wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31871/ > ----------------------------------------------------------- > > (Updated March 9, 2015, 10:07 p.m.) > > > Review request for drill and Jacques Nadeau. > > > Bugs: DRILL-2406 > https://issues.apache.org/jira/browse/DRILL-2406 > > > Repository: drill-git > > > Description > ------- > > To enable planning rules to take advantage of constant expressions in > queries, or to allow them to run queries against small datasets, such as > partition information we need to e able to evaluate expressions at planning > time. Expression evaluation in the regular expecuation path is relatively > expensive, as it invovles java code generation, compilation and JITing > expression trees. An interpreted expression system was added recently to > allow evaluating an expression without generating java code at runtime. This > patch exposes that work (after some refactorings in 2143 and the first part > of 2406) to the planning context for use in optimizer rules. > > > Diffs > ----- > > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java > 00aaec6 > > exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java > 2a28bcb > exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java > aa1dffd > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java > c881432 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceAggregatesRule.java > 93fff35 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java > 907fcb1 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java > 6b54c43 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > 409450f > > Diff: https://reviews.apache.org/r/31871/diff/ > > > Testing > ------- > > Cluster tests completed, waiting on unit tests. > > > Thanks, > > Jason Altekruse > >
