----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5591/#review8764 -----------------------------------------------------------
/trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java <https://reviews.apache.org/r/5591/#comment18501> Now that it's an accumulator eval func, you can throw away the exec /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java <https://reviews.apache.org/r/5591/#comment18503> do you need to do the cast? The type parameter should ensure that GroovyEvalFunc returns type T /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java <https://reviews.apache.org/r/5591/#comment18504> same as above. It should already be returning Tuple /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java <https://reviews.apache.org/r/5591/#comment18505> the extends is incorrect, should be Tuple. copy paste error :) /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java <https://reviews.apache.org/r/5591/#comment18506> it has to be Map<String,?> /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java <https://reviews.apache.org/r/5591/#comment18507> What do you do if a method is overloaded, but only one of them applies to the arguments that will be given? /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java <https://reviews.apache.org/r/5591/#comment18508> Modifier.isStatic(this.method.getModifiers()) /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java <https://reviews.apache.org/r/5591/#comment18509> Same comment as earlier. I feel like we should detect this earlier, and throw an error. A method that returns null shouldn't happen. /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java <https://reviews.apache.org/r/5591/#comment18510> Can you post an example of how your OutputSchemaFunctions work? /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java <https://reviews.apache.org/r/5591/#comment18511> ParserException and IOException both are the result of a bad function and probably should abort. I could be persuaded otherwise. Use the LOG object instead of printing to stdout. /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFuncObject.java <https://reviews.apache.org/r/5591/#comment18512> Is this just do avoid having to write GroovyEvalFunc<Object> a bunch above? Given that you're saving 2 characters, is it worth it? /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18514> You have three code paths here. One is a normal UDF, one is Algebraic, one is Accumulator. I think this code would be more understandable if you had something like isEvalFunc(annotation), isAlgebraic(annotation), isAccumulator(annotation), and then processEvalFunc(annotation), processAlgebraic(annotation), and so on. The nested ifs and fors etc make it a bit uglier, and I think it'd be nice to separate it out. /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18517> Given you maintain that an evalfunc must specify it's output, perhaps GroovyEvalFunc should check to make sure that schema isn't null? /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18518> given the previous logic, it's impossible to get here without one of schema or schemafunction not being null /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18522> There are some previous comments that still stand w.r.t. whether you want to introduce Map<String,Object> here, since that is what Pig maps have to be. - Jonathan Coveney On June 28, 2012, 6:25 a.m., Mathias Herberts wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5591/ > ----------------------------------------------------------- > > (Updated June 28, 2012, 6:25 a.m.) > > > Review request for pig, Julien Le Dem and Jonathan Coveney. > > > Description > ------- > > Adds support for Groovy UDFs in Pig. > > > This addresses bug PIG-2763. > https://issues.apache.org/jira/browse/PIG-2763 > > > Diffs > ----- > > /trunk/ivy.xml 1353307 > /trunk/ivy/libraries.properties 1353307 > /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1354285 > /trunk/src/org/apache/pig/scripting/groovy/AccumulatorAccumulate.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/AccumulatorCleanup.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/AccumulatorGetValue.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/AlgebraicFinal.java PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/AlgebraicInitial.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/AlgebraicIntermed.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFuncObject.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java > PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java PRE-CREATION > /trunk/src/org/apache/pig/scripting/groovy/OutputSchemaFunction.java > PRE-CREATION > /trunk/test/org/apache/pig/test/TestUDFGroovy.java PRE-CREATION > /trunk/test/unit-tests 1353307 > > Diff: https://reviews.apache.org/r/5591/diff/ > > > Testing > ------- > > > Thanks, > > Mathias Herberts > >
