> On March 7, 2015, 2:32 a.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java, > > line 140 > > <https://reviews.apache.org/r/30754/diff/3/?file=885028#file885028line140> > > > > 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.
I do agree that we should standardize where this is defined, but I don't believe adding it as an argument in the annotation is the best place. This would require any consumers of this "interface" to give the right value despite the fact that it is constant. I think we could simply create a static map that holds a relationship between Injectable classes and their corresponding getter methods on the UDfUtilities interface. > On March 7, 2015, 2:32 a.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java, > > line 55 > > <https://reviews.apache.org/r/30754/diff/3/?file=885033#file885033line55> > > > > for constantExpr evaluation, why do we need to passin a outVV? Is it > > better that this method return a ValueHolder directly? I agree, I'll make this change as I go to update the second half of 2060, this is the patch that is actually using this new interface. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30754/#review75608 ----------------------------------------------------------- On March 4, 2015, 11: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, 11: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 > >
