drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Altekruse" <altekruseja...@gmail.com>
Subject Re: Review Request 30754: DRILL-2143 - part 2 - remove record batch interface from UDF interface
Date Sat, 14 Mar 2015 01:46:29 GMT


> 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.
> 
> Jason Altekruse wrote:
>     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.

fixed


> 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?
> 
> Jason Altekruse wrote:
>     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.

fixed


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message