> 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)
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. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyEvalFunc.java, line 129 > > <https://reviews.apache.org/r/5591/diff/1/?file=116557#file116557line129> > > > > 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) This is done because the GroovyEvalFunc wrapper is used for Accumulator UDFs when calling accumulate/cleanup which are 'void' methods. Not supporting 'void' methods in GroovyEvalFunc would force to add a GroovyVoidEvalFunc class just for the Accumulator case. > 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 I always use // for in line comments, this way I can comment out a block of code spanning multiple lines by using /* ... */ > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java, line 195 > > <https://reviews.apache.org/r/5591/diff/1/?file=116559#file116559line195> > > > > 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. As stated earlier, a Groovy class should really be seen as a container for multiple UDFs, not as containing a single one. Non static methods are needed for Accumulator UDFs, all other UDFs maintain no state, thus the use of static methods. I guess non static methods could be supported too. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java, line 200 > > <https://reviews.apache.org/r/5591/diff/1/?file=116559#file116559line200> > > > > See above, this is a weird special case to me... methods annotated with @AccumulatorGetValue need to have an OuputSchema defined, but since they are part of a trio of methods used to implement the Accumulator, they should not be exposed directly. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 96 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line96> > > > > Is it not possible for users to create a pig Tuple that they then put > > Groovy objects into? They could, but this is strongly discouraged, the use case is to create Pig's Tuple or DataBag and populate them with Groovy objects converted by GroovyUtils.groovyToPig. The ability to create Pig's DataBag from Groovy is to benefit from the spill to disk nature of those. The support of Pig's Tuple is simply to be coherent. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 95 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line95> > > > > I'm a big fan of having a private static final TupleFactory and > > BagFactory in the class. YMMV Ok, added. > 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 :) I already did! > 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? 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. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 147 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line147> > > > > In the case of an int, we shouldn't have to go to/from int. Same with > > Long, Double, and Float. Ok, fixed it. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 170 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line170> > > > > why not just return the boolean? Fixed. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 210 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line210> > > > > you can just iterate directly on it without calling getall. also, you > > could use groovy.lang.Tuple#addAll? Since I have to convert Pig objects to their Groovy counterpart, I can't simply call addAll on the Groovy Tuple. > 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. 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. > On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote: > > /trunk/src/org/apache/pig/scripting/groovy/GroovyUtils.java, line 271 > > <https://reviews.apache.org/r/5591/diff/1/?file=116560#file116560line271> > > > > I don't know if it should throw away the exception like this. What would you recommend? Throwing RuntimeException? - Mathias ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5591/#review8628 ----------------------------------------------------------- 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 > >
