> On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java, > > line 133 > > <https://reviews.apache.org/r/5591/diff/3/?file=117072#file117072line133> > > > > the extends is incorrect, should be Tuple. copy paste error :)
Corrected. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java, > > line 269 > > <https://reviews.apache.org/r/5591/diff/3/?file=117072#file117072line269> > > > > it has to be Map<String,?> Corrected. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java, > > line 63 > > <https://reviews.apache.org/r/5591/diff/3/?file=117072#file117072line63> > > > > do you need to do the cast? The type parameter should ensure that > > GroovyEvalFunc returns type T Nope, gone. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java, > > line 46 > > <https://reviews.apache.org/r/5591/diff/3/?file=117071#file117071line46> > > > > Now that it's an accumulator eval func, you can throw away the exec Done. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java, line 160 > > <https://reviews.apache.org/r/5591/diff/3/?file=117073#file117073line160> > > > > Can you post an example of how your OutputSchemaFunctions work? There is a unit test that contains the required Groovy code, here it is: <code> import org.apache.pig.scripting.groovy.OutputSchemaFunction;", class GroovyUDFs { @OutputSchemaFunction('squareSchema') public static square(x) { return x * x; } public static squareSchema(input) { return input; } } </code> > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java, line 80 > > <https://reviews.apache.org/r/5591/diff/3/?file=117073#file117073line80> > > > > What do you do if a method is overloaded, but only one of them applies > > to the arguments that will be given? PigContext keeps track of registered functions in a Map, therefore only a single function can be defined with a given name. For this reason, we now check for duplicate names in GroovyEvalFunc and throw an exception if we find more than one method matching a given name. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java, line 143 > > <https://reviews.apache.org/r/5591/diff/3/?file=117073#file117073line143> > > > > Same comment as earlier. I feel like we should detect this earlier, and > > throw an error. A method that returns null shouldn't happen. As stated previously, this is done so the 'cleanup' and 'accumulate' methods can be wrapped into a GroovyEvalFunc. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java, line 163 > > <https://reviews.apache.org/r/5591/diff/3/?file=117073#file117073line163> > > > > 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. I now wrap them in a RuntimeException and throw the RE. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFuncObject.java, line > > 23 > > <https://reviews.apache.org/r/5591/diff/3/?file=117074#file117074line23> > > > > Is this just do avoid having to write GroovyEvalFunc<Object> a bunch > > above? Given that you're saving 2 characters, is it worth it? Nope, it's because I don't think there is another way to create a FuncSpec which will instantiate a GroovyEvalFunc<Object>: FuncSpec spec = new FuncSpec(GroovyEvalFuncObject.class.getCanonicalName() + "('" + path + "','" + namespace + "','" + method.getName() + "')"); > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java, line 126 > > <https://reviews.apache.org/r/5591/diff/3/?file=117075#file117075line126> > > > > 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. I added private static methods isAlgebraic(Annotation annotation) and isAccumulator(Annotation annotation), I'm reluctant to introduce processEvalFunc/processAlgebraic/processAccumulator since the two later can't be called until we've filled the method arrays with sufficient methods. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java, line 205 > > <https://reviews.apache.org/r/5591/diff/3/?file=117075#file117075line205> > > > > Given you maintain that an evalfunc must specify it's output, perhaps > > GroovyEvalFunc should check to make sure that schema isn't null? Once again, GroovyEvalFunc is also used for auxiliary methods of Accumulator/Algebraic UDFs which do not have a return schema, that's why we allow GroovyEvalFunc to wrap methods which either don't have a schema/schemaFunction and/or do not return values. > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java, line 226 > > <https://reviews.apache.org/r/5591/diff/3/?file=117075#file117075line226> > > > > given the previous logic, it's impossible to get here without one of > > schema or schemafunction not being null Modified the code to only check 'isAccumulator' > On June 29, 2012, 10:48 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 117 > > <https://reviews.apache.org/r/5591/diff/3/?file=117076#file117076line117> > > > > 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. Corrected, we now consider Pig maps to only have String keys and we convert Groovy Maps' keys to String prior to populating the PigMap. - Mathias ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5591/#review8764 ----------------------------------------------------------- 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 > >
