----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5591/#review8628 -----------------------------------------------------------
/trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java <https://reviews.apache.org/r/5591/#comment18274> can you get rid of trailing whitespace? In vim: %s/\s\+$// will do it /trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java <https://reviews.apache.org/r/5591/#comment18265> you can have this class extend AccumulatorEvalFunc -- it was made just for this case :) /trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java <https://reviews.apache.org/r/5591/#comment18266> I don't like this. What is the source of errors? /trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java <https://reviews.apache.org/r/5591/#comment18273> 2 points here. 1) It seems odd to me that you lump outputSchema with the getValue method given your annotation driven approach. Why not annotate the Groovy class instead, or, better yet, allow users to set their own method? Leading to... 2) you could also support dynamic outputSchemas based on input schemas (jython and jruby support both do this) /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java <https://reviews.apache.org/r/5591/#comment18275> I'm so happy that someone who isn't me found this useful :) /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java <https://reviews.apache.org/r/5591/#comment18276> IMHO, if they have a UDF that returns null, you should detect this earlier on and throw an error. Same with any methods which don't accept Pig types, if you want to get fancy (JRuby did not get this fancy, but I think at least the former is important rather than returning null) /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18277> throw an UnsupportedOp exception, it shouldn't be called /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18278> In general, I'd prefer /** */ javadoc style comments when commenting in line, but this is a style nitpick /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18279> It seems weird to allow Groovy static methods as UDFs. I suppose there is no harm in it, but given that in Pig all UDF's imply that they are instantiated, it proposes a potential strong departure from how people typically should think about UDF's. /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18280> See above, this is a weird special case to me... /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java <https://reviews.apache.org/r/5591/#comment18281> You can also make sure sure that Initial and Intermed return Tuple /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18284> I'm a big fan of having a private static final TupleFactory and BagFactory in the class. YMMV /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18282> Is it not possible for users to create a pig Tuple that they then put Groovy objects into? /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18283> Pig maps have to have Strings as keys. I suppose we don't HAVE to check that here, but it could have potentially weird results /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18287> In the case of an int, we shouldn't have to go to/from int. Same with Long, Double, and Float. /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18285> you should go express support of the BigInt/BigDec patch :) /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18286> Why do you copy the byte array here? It's not like you're copying in all other cases. Is the goal buffer reuse or something? /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18288> why not just return the boolean? /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18289> you can just iterate directly on it without calling getall. also, you could use groovy.lang.Tuple#addAll? /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18292> Same comment as above: Pig maps always have String keys /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18293> Is there a reason why we return groovy Tuples and not just pig Tuples themselves? Is it because we have to go into the Tuple to do the conversion anyway? I'm not averse, just curious on your thoughts. /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18290> instead of doing groovy.lang.Tuple you could do a static import as GroovyTuple or something. /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java <https://reviews.apache.org/r/5591/#comment18291> I don't know if it should throw away the exception like this. - Jonathan Coveney On June 26, 2012, 5:52 p.m., Mathias Herberts wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5591/ > ----------------------------------------------------------- > > (Updated June 26, 2012, 5:52 p.m.) > > > Review request for pig, Julien Le Dem and Jonathan Coveney. > > > Description > ------- > > Adds support for Groovy UDFs in Pig. > > > Diffs > ----- > > /trunk/ivy.xml 1353307 > /trunk/ivy/libraries.properties 1353307 > /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1353307 > /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/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 > >
