You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Mathias Herberts <Ma...@gmail.com> on 2012/06/26 19:52:30 UTC
Review Request: PIG-2763 - Groovy UDFs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/
-----------------------------------------------------------
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
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Julien Le Dem <ju...@ledem.net>.
> 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.
>
> Mathias Herberts wrote:
> 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.
For stateless methods that don't need initialization, static methods are easier. We should allow both
> 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.
>
> Mathias Herberts wrote:
> What would you recommend? Throwing RuntimeException?
yes. And chain the cause
- Julien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8628
-----------------------------------------------------------
On June 26, 2012, 11:26 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, 11:26 p.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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
> 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.
>
> Mathias Herberts wrote:
> 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.
>
> Julien Le Dem wrote:
> For stateless methods that don't need initialization, static methods are easier. We should allow both
I added support of both in PIG-2763-3.patch
> 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.
>
> Mathias Herberts wrote:
> What would you recommend? Throwing RuntimeException?
>
> Julien Le Dem wrote:
> yes. And chain the cause
Done in PIG-2763-3.patch
- Mathias
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8628
-----------------------------------------------------------
On June 26, 2012, 11:26 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, 11:26 p.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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Jonathan Coveney <jc...@gmail.com>.
> On June 26, 2012, 10:14 p.m., Jonathan Coveney wrote:
> >
Like this a lot! I also like that we're getting a clearer blueprint on what it takes it implement a scripting language... I think we could definitely make a better abstraction soon.
Oh and can you put a link to the JIRA on the reviewboard?
- Jonathan
-----------------------------------------------------------
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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Jonathan Coveney <jc...@gmail.com>.
> 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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
> 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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Jonathan Coveney <jc...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
> 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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Jonathan Coveney <jc...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Julien Le Dem <ju...@ledem.net>.
> On July 3, 2012, 5:54 p.m., Julien Le Dem wrote:
> > /trunk/src/org/apache/pig/scripting/groovy/AccumulatorAccumulate.java, line 40
> > <https://reviews.apache.org/r/5591/diff/4-6/?file=119006#file119006line40>
> >
> > you don't have to define value(). If you don't want it just delete this line.
> > (same bellow)
>
> Mathias Herberts wrote:
> We need the annotation's parameter as it defines the UDF name.
sorry, I misread the javadoc
- Julien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8844
-----------------------------------------------------------
On July 3, 2012, 7:34 p.m., Mathias Herberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5591/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 7:34 p.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 1356486
> /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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
> On July 3, 2012, 5:54 p.m., Julien Le Dem wrote:
> > /trunk/src/org/apache/pig/scripting/groovy/AccumulatorAccumulate.java, line 40
> > <https://reviews.apache.org/r/5591/diff/4-6/?file=119006#file119006line40>
> >
> > you don't have to define value(). If you don't want it just delete this line.
> > (same bellow)
We need the annotation's parameter as it defines the UDF name.
> On July 3, 2012, 5:54 p.m., Julien Le Dem wrote:
> > /trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java, lines 118-121
> > <https://reviews.apache.org/r/5591/diff/4-6/?file=119016#file119016line118>
> >
> > check out this to pass parameters:
> >
> > http://svn.apache.org/viewvc/pig/trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java?view=markup
> > line 178
> >
> > argv = (String[])ObjectSerializer.deserialize(pigContext.getProperties().getProperty(PigContext.PIG_CMD_ARGS_REMAINDERS));
> >
I was wondering where to get this, thanks.
- Mathias
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8844
-----------------------------------------------------------
On July 3, 2012, 1:07 a.m., Mathias Herberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5591/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 1:07 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 1356486
> /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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Julien Le Dem <ju...@ledem.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8844
-----------------------------------------------------------
Thanks for implementing the embedding as well.
/trunk/src/org/apache/pig/scripting/groovy/AccumulatorAccumulate.java
<https://reviews.apache.org/r/5591/#comment18780>
you don't have to define value(). If you don't want it just delete this line.
(same bellow)
/trunk/src/org/apache/pig/scripting/groovy/GroovyScriptEngine.java
<https://reviews.apache.org/r/5591/#comment18781>
check out this to pass parameters:
http://svn.apache.org/viewvc/pig/trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java?view=markup
line 178
argv = (String[])ObjectSerializer.deserialize(pigContext.getProperties().getProperty(PigContext.PIG_CMD_ARGS_REMAINDERS));
- Julien Le Dem
On July 3, 2012, 1:07 a.m., Mathias Herberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5591/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 1:07 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 1356486
> /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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Julien Le Dem <ju...@ledem.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8901
-----------------------------------------------------------
Ship it!
Ship It!
- Julien Le Dem
On July 3, 2012, 7:34 p.m., Mathias Herberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5591/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 7:34 p.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 1356486
> /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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8902
-----------------------------------------------------------
Ship it!
Ship It!
- Mathias Herberts
On July 3, 2012, 7:34 p.m., Mathias Herberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5591/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 7:34 p.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 1356486
> /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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/
-----------------------------------------------------------
(Updated July 3, 2012, 7:34 p.m.)
Review request for pig, Julien Le Dem and Jonathan Coveney.
Changes
-------
Added support for arguments to method main when running an embedded script.
Description
-------
Adds support for Groovy UDFs in Pig.
This addresses bug PIG-2763.
https://issues.apache.org/jira/browse/PIG-2763
Diffs (updated)
-----
/trunk/ivy.xml 1353307
/trunk/ivy/libraries.properties 1353307
/trunk/src/org/apache/pig/scripting/ScriptEngine.java 1356486
/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
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/
-----------------------------------------------------------
(Updated July 3, 2012, 1:07 a.m.)
Review request for pig, Julien Le Dem and Jonathan Coveney.
Changes
-------
Removed shebang for Groovy.
Added support for embedded Pig in Groovy.
Description
-------
Adds support for Groovy UDFs in Pig.
This addresses bug PIG-2763.
https://issues.apache.org/jira/browse/PIG-2763
Diffs (updated)
-----
/trunk/ivy.xml 1353307
/trunk/ivy/libraries.properties 1353307
/trunk/src/org/apache/pig/scripting/ScriptEngine.java 1356486
/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
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/
-----------------------------------------------------------
(Updated July 3, 2012, 12:05 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 (updated)
-----
/trunk/ivy.xml 1353307
/trunk/ivy/libraries.properties 1353307
/trunk/src/org/apache/pig/scripting/ScriptEngine.java 1356486
/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
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
> On July 2, 2012, 11:14 p.m., Julien Le Dem wrote:
> > /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java, lines 76-79
> > <https://reviews.apache.org/r/5591/diff/4/?file=119013#file119013line76>
> >
> > it does not do anything more than calling the parent then it does not realy need to override the method. (same for other classes here)
Good point. Corrected.
> On July 2, 2012, 11:14 p.m., Julien Le Dem wrote:
> > /trunk/test/org/apache/pig/test/TestUDFGroovy.java, lines 336-361
> > <https://reviews.apache.org/r/5591/diff/4/?file=119019#file119019line336>
> >
> > What about this syntax?
> >
> > class Sumalg extends AgebraicGroovyUDF {",
> > public Tuple initial(Tuple t) {
> > ...
> > }
> > public Tuple intermed(Tuple t) {
> > ...
> > }
> > public long final(Tuple t) {
> > ...
> > }
> > }
> >
> > This would be consistent with the JRuby impl (and java in general)
As I've already expressed, my experience with Groovy UDFs is that people tend to build libraries of UDFs in a single Groovy class. Sticking to the Java way of doing UDFs would multiply the REGISTER statements in the Pig scripts (one per function vs one per library). The Java way of doing things is fine because Java source code is compiled and packaged in a single jar which is then registered.
I'd like to retain the flexibility of being able to provide a single Groovy file containing multiple (usually related) UDFs, i.e. time.groovy, currency.groovy, etc.
> On July 2, 2012, 11:14 p.m., Julien Le Dem wrote:
> > /trunk/src/org/apache/pig/scripting/groovy/AccumulatorAccumulate.java, line 28
> > <https://reviews.apache.org/r/5591/diff/4/?file=119006#file119006line28>
> >
> > javadoc please for these
Added javadoc for AccumulatorXXX and AlgebraicXXX annotations.
> On July 2, 2012, 11:14 p.m., Julien Le Dem wrote:
> > /trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java, line 179
> > <https://reviews.apache.org/r/5591/diff/4/?file=119013#file119013line179>
> >
> > this is actually a funcspec and could return "org.apache....Final('Double')" if that helps.
I'll stick with Final.class.getName()
- Mathias
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8819
-----------------------------------------------------------
On July 2, 2012, 9:23 p.m., Mathias Herberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5591/
> -----------------------------------------------------------
>
> (Updated July 2, 2012, 9:23 p.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 1356486
> /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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Julien Le Dem <ju...@ledem.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/#review8819
-----------------------------------------------------------
This is a great patch!
Here are some comments.
/trunk/src/org/apache/pig/scripting/groovy/AccumulatorAccumulate.java
<https://reviews.apache.org/r/5591/#comment18680>
javadoc please for these
/trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java
<https://reviews.apache.org/r/5591/#comment18683>
it does not do anything more than calling the parent then it does not realy need to override the method. (same for other classes here)
/trunk/src/org/apache/pig/scripting/groovy/GroovyAlgebraicEvalFunc.java
<https://reviews.apache.org/r/5591/#comment18685>
this is actually a funcspec and could return "org.apache....Final('Double')" if that helps.
/trunk/test/org/apache/pig/test/TestUDFGroovy.java
<https://reviews.apache.org/r/5591/#comment18686>
What about this syntax?
class Sumalg extends AgebraicGroovyUDF {",
public Tuple initial(Tuple t) {
...
}
public Tuple intermed(Tuple t) {
...
}
public long final(Tuple t) {
...
}
}
This would be consistent with the JRuby impl (and java in general)
- Julien Le Dem
On July 2, 2012, 9:23 p.m., Mathias Herberts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5591/
> -----------------------------------------------------------
>
> (Updated July 2, 2012, 9:23 p.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 1356486
> /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
>
>
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/
-----------------------------------------------------------
(Updated July 2, 2012, 9:23 p.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 (updated)
-----
/trunk/ivy.xml 1353307
/trunk/ivy/libraries.properties 1353307
/trunk/src/org/apache/pig/scripting/ScriptEngine.java 1356486
/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
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Added support for non static methods (with associated unit test). Modified DataBagIterator to throw RuntimeException when it encounters an exception in pigToGroovy.
Description
-------
Adds support for Groovy UDFs in Pig.
This addresses bug PIG-2763.
https://issues.apache.org/jira/browse/PIG-2763
Diffs (updated)
-----
/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
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/
-----------------------------------------------------------
(Updated June 26, 2012, 11:26 p.m.)
Review request for pig, Julien Le Dem and Jonathan Coveney.
Changes
-------
Added ref to PIG-2763
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
Re: Review Request: PIG-2763 - Groovy UDFs
Posted by Mathias Herberts <Ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5591/
-----------------------------------------------------------
(Updated June 26, 2012, 11:22 p.m.)
Review request for pig, Julien Le Dem and Jonathan Coveney.
Changes
-------
Second iteration of patch, includes fixes for comments from jcoveney and addition of OutputSchemaFunction.
Description
-------
Adds support for Groovy UDFs in Pig.
Diffs (updated)
-----
/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