----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30754/#review75608 -----------------------------------------------------------
Ship it! Have two minor comments. Overall looks good to me. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java <https://reviews.apache.org/r/30754/#comment122807> I guess for now it's fine to add code just to handle Current_Date() function etc. However, in the future, if we have more functions which depends on QueryContext, like Current_version(), Current_Node_Number, etc, are we going to modify again? I feel it would be better to add new annotation in the function template for the inject workspace, @Inject(get="getQueryDateTimeInfor") QueryDateTimeInfo datetime; QueryDateTimeInfo or any new class will extend QueryContextInfo. Then, here, we do not need to add one additional "else if" block each time when we add one individual new function which depends on QueryContext. exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java <https://reviews.apache.org/r/30754/#comment122810> for constantExpr evaluation, why do we need to passin a outVV? Is it better that this method return a ValueHolder directly? - Jinfeng Ni On March 4, 2015, 3:52 p.m., Jason Altekruse wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30754/ > ----------------------------------------------------------- > > (Updated March 4, 2015, 3:52 p.m.) > > > Review request for drill, Aman Sinha, Jacques Nadeau, Jinfeng Ni, Mehant > Baid, and Parth Chandra. > > > Bugs: DRILL-2143 > https://issues.apache.org/jira/browse/DRILL-2143 > > > Repository: drill-git > > > Description > ------- > > This patch fixes the breakage of removing the record batch from the setup > method in the DrillFunc interface. It adds an injectable type to bring back > the date functions and make the interpreted expression evaluation work with > the new interface. > > > Diffs > ----- > > > exec/interpreter/src/test/java/org/apache/drill/exec/expr/ExpressionInterpreterTest.java > a94ef94 > > 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/DateTypeFunctions.java > cc4be89 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java > a3bc1de > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/DrillSimpleFuncInterpreter.java > e3696f0 > > 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/expr/fn/interpreter/InterpreterGenerator.java > 6cede33 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/holders/ValueHolder.java > 5c2adc6 > exec/java-exec/src/main/java/org/apache/drill/exec/expr/package-info.java > PRE-CREATION > 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/QueryDateTimeInfo.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/ops/UdfUtilities.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/30754/diff/ > > > Testing > ------- > > Almost all cluster tests are passing, recieved some failures that seem > unrelated and unlikely cased by the changes, but are not reported as expected > failures currently. Still need to run full unit tests again with these most > recent changes. > > > Thanks, > > Jason Altekruse > >
