> On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyAccumulatorEvalFunc.java, > > line 80 > > <https://reviews.apache.org/r/5591/diff/1/?file=116555#file116555line80> > > > > 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) > > Mathias Herberts wrote: > Annotating the Groovy Class would mean that we have a single UDF per > class as is the case in Java. It seems to me it is more practical to see > several UDFs in a single Groovy class, thus making the class more of a UDF > library container than a single UDF container. > > Dynamic outputschemas have been added via an OutputSchemaFunction > annotation, this will be reflected in the next iteration of the patch.
I'm cool with that. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java, line 88 > > <https://reviews.apache.org/r/5591/diff/1/?file=116559#file116559line88> > > > > In general, I'd prefer /** */ javadoc style comments when commenting > > in line, but this is a style nitpick > > Mathias Herberts wrote: > I always use // for in line comments, this way I can comment out a block > of code spanning multiple lines by using /* ... */ I may steal that from you. /* is so much prettier though :) > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 149 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line149> > > > > you should go express support of the BigInt/BigDec patch :) > > Mathias Herberts wrote: > I already did! Haha I meant +1ing this: https://issues.apache.org/jira/browse/PIG-2764 > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 160 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line160> > > > > 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? > > Mathias Herberts wrote: > I don't know where the byte[] is coming from, the Groovy method might > have retrieved it from a class which reuses a static instance, so as a safety > measure I allocate a new one and copy the content. I find this acceptable, though there are some cases where if people are doing funky stuff with object reuse, you won't protect them. I suppose this is reasonable, though, since a bytearray is a particularly dangerous case. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 253 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line253> > > > > 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. > > Mathias Herberts wrote: > Since the iterator is used on the Groovy side, it felt more natural to > have Groovy Tuples, using Pig types on the Groovy side should be the > exception, not the norm. I am down with that - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5591/#review8628 ----------------------------------------------------------- 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 > >
