You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Julien Le Dem <ju...@ledem.net> on 2011/01/14 04:01:24 UTC
Review Request: Javascript support for Pig UDFs and embedding in scripting
languages
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/
-----------------------------------------------------------
Review request for pig and Richard Ding.
Summary
-------
This is not a final patch.
I've limited modifications to the existing framework as much as possible.
However some changes where necessary. They are backward compatible.
This addresses bug PIG-1794.
https://issues.apache.org/jira/browse/PIG-1794
Diffs
-----
/trunk/src/org/apache/pig/Main.java 1056815
/trunk/src/org/apache/pig/scripting/Pig.java 1056815
/trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
/trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
/trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
/trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
/trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
Diff: https://reviews.apache.org/r/321/diff
Testing
-------
Thanks,
Julien
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Richard Ding <rd...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/#review185
-----------------------------------------------------------
Ship it!
- Richard
On 2011-01-16 22:40:06, Julien Le Dem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/321/
> -----------------------------------------------------------
>
> (Updated 2011-01-16 22:40:06)
>
>
> Review request for pig and Richard Ding.
>
>
> Summary
> -------
>
> This is not a final patch.
> I've limited modifications to the existing framework as much as possible.
> However some changes where necessary. They are backward compatible.
>
>
> This addresses bug PIG-1794.
> https://issues.apache.org/jira/browse/PIG-1794
>
>
> Diffs
> -----
>
> /trunk/ivy.xml 1056815
> /trunk/ivy/libraries.properties 1056815
> /trunk/ivy/pig.pom 1056815
> /trunk/src/org/apache/pig/Main.java 1056815
> /trunk/src/org/apache/pig/scripting/Pig.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
> /trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
> /trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
> /trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/321/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Julien
>
>
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Julien Le Dem <ju...@ledem.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/
-----------------------------------------------------------
(Updated 2011-02-23 15:11:36.720059)
Review request for pig and Richard Ding.
Changes
-------
A few bug fixes. Now committed to TRUNK
Summary
-------
This is not a final patch.
I've limited modifications to the existing framework as much as possible.
However some changes where necessary. They are backward compatible.
This addresses bug PIG-1794.
https://issues.apache.org/jira/browse/PIG-1794
Diffs (updated)
-----
/trunk/CHANGES.txt 1073618
/trunk/ivy.xml 1073618
/trunk/ivy/libraries.properties 1073618
/trunk/ivy/pig.pom 1073618
/trunk/src/org/apache/pig/Main.java 1073618
/trunk/src/org/apache/pig/scripting/Pig.java 1073618
/trunk/src/org/apache/pig/scripting/ScriptEngine.java 1073618
/trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1073618
/trunk/src/org/apache/pig/scripting/js/JSPig.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1073618
/trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
/trunk/test/org/apache/pig/test/Util.java 1073618
/trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
Diff: https://reviews.apache.org/r/321/diff
Testing
-------
Thanks,
Julien
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Julien Le Dem <ju...@ledem.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/
-----------------------------------------------------------
(Updated 2011-01-16 22:40:06.285242)
Review request for pig and Richard Ding.
Changes
-------
I have updated the code based on the comments (some I have not done yet but planning to):
- switched to official Rhino (not sure how to make it optional)
- used StringBuffer more
- moved the list of languages out of Main
- factored the script loading
- added comments
- added namespace for functions
Summary
-------
This is not a final patch.
I've limited modifications to the existing framework as much as possible.
However some changes where necessary. They are backward compatible.
This addresses bug PIG-1794.
https://issues.apache.org/jira/browse/PIG-1794
Diffs (updated)
-----
/trunk/ivy.xml 1056815
/trunk/ivy/libraries.properties 1056815
/trunk/ivy/pig.pom 1056815
/trunk/src/org/apache/pig/Main.java 1056815
/trunk/src/org/apache/pig/scripting/Pig.java 1056815
/trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
/trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
/trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
/trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
/trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
/trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
Diff: https://reviews.apache.org/r/321/diff
Testing
-------
Thanks,
Julien
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Julien Le Dem <ju...@ledem.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/#review123
-----------------------------------------------------------
- Julien
On 2011-01-13 19:01:24, Julien Le Dem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/321/
> -----------------------------------------------------------
>
> (Updated 2011-01-13 19:01:24)
>
>
> Review request for pig and Richard Ding.
>
>
> Summary
> -------
>
> This is not a final patch.
> I've limited modifications to the existing framework as much as possible.
> However some changes where necessary. They are backward compatible.
>
>
> This addresses bug PIG-1794.
> https://issues.apache.org/jira/browse/PIG-1794
>
>
> Diffs
> -----
>
> /trunk/src/org/apache/pig/Main.java 1056815
> /trunk/src/org/apache/pig/scripting/Pig.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
> /trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
> /trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
> /trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/321/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Julien
>
>
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Julien Le Dem <ju...@ledem.net>.
> On 2011-01-14 15:46:50, Richard Ding wrote:
> > I downloaded the patch. It gives some compilation errors such as "package sun.org.mozilla.javascript.internal does not exist". Is there some jars missing from compile classpath?
It uses the Rhino bundled in the JDK. Hence the sun.org.mozilla...
It would probably be better to use the standard Rhino. Maybe you don't use the same version of the JDK ?
- Julien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/#review124
-----------------------------------------------------------
On 2011-01-13 19:01:24, Julien Le Dem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/321/
> -----------------------------------------------------------
>
> (Updated 2011-01-13 19:01:24)
>
>
> Review request for pig and Richard Ding.
>
>
> Summary
> -------
>
> This is not a final patch.
> I've limited modifications to the existing framework as much as possible.
> However some changes where necessary. They are backward compatible.
>
>
> This addresses bug PIG-1794.
> https://issues.apache.org/jira/browse/PIG-1794
>
>
> Diffs
> -----
>
> /trunk/src/org/apache/pig/Main.java 1056815
> /trunk/src/org/apache/pig/scripting/Pig.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
> /trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
> /trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
> /trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/321/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Julien
>
>
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Richard Ding <rd...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/#review124
-----------------------------------------------------------
I downloaded the patch. It gives some compilation errors such as "package sun.org.mozilla.javascript.internal does not exist". Is there some jars missing from compile classpath?
- Richard
On 2011-01-13 19:01:24, Julien Le Dem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/321/
> -----------------------------------------------------------
>
> (Updated 2011-01-13 19:01:24)
>
>
> Review request for pig and Richard Ding.
>
>
> Summary
> -------
>
> This is not a final patch.
> I've limited modifications to the existing framework as much as possible.
> However some changes where necessary. They are backward compatible.
>
>
> This addresses bug PIG-1794.
> https://issues.apache.org/jira/browse/PIG-1794
>
>
> Diffs
> -----
>
> /trunk/src/org/apache/pig/Main.java 1056815
> /trunk/src/org/apache/pig/scripting/Pig.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
> /trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
> /trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
> /trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/321/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Julien
>
>
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Julien Le Dem <ju...@ledem.net>.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/Main.java, line 885
> > <https://reviews.apache.org/r/321/diff/1/?file=9776#file9776line885>
> >
> > I am worried about having to keep changing this for every language even though we are supposed to have a general jvm-based-language framework. Is there a way to delegate this to the scripting implementation, or to avoid going into this code block?
Agreed, I was thinking the same thing. I will look into this.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsFunction.java, line 65
> > <https://reviews.apache.org/r/321/diff/1/?file=9780#file9780line65>
> >
> > Please use StringBuilder
> >
> > Also, it's a personal style thing, but do you mind putting spaces between operators between operators and operands?
> >
> > meaning, foo = bar instead of foo=bar
Agreed. This is a quickly hacked together debug function, please forgive me for this :P
I'll change to StringBuffer
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsFunction.java, line 201
> > <https://reviews.apache.org/r/321/diff/1/?file=9780#file9780line201>
> >
> > Any reason for this not to be Scriptable's toString()?
I would need to compare but I think toString() returns things like [Object sthg] which was not useful for debugging
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java, line 77
> > <https://reviews.apache.org/r/321/diff/1/?file=9781#file9781line77>
> >
> > you probably also want to catch SecurityException.
> >
> > What about files in HDFS?
This was based on the PythonScriptEngine. We should probably extract the common logic here.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java, line 106
> > <https://reviews.apache.org/r/321/diff/1/?file=9781#file9781line106>
> >
> > Is that always safe? I think there is some edge case where either getUDFContext() or getUDFProperties() returns null..
> >
> > *scratches head trying to remember*
I think at the time I call it it must be safe. Otherwise the whole thing is broken.
More unit tests would help.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java, line 234
> > <https://reviews.apache.org/r/321/diff/1/?file=9781#file9781line234>
> >
> > Is the print/println specialcasing documented somewhere?
I'm adding these functions for debugging purpose like the Java scripting integration does. I'll add documentation about it.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsFunction.java, line 158
> > <https://reviews.apache.org/r/321/diff/1/?file=9780#file9780line158>
> >
> > you seem to always do " " + indent. Shouldn't indent just have the " " in it?
and increment first thing at the top ? Probably.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsFunction.java, line 62
> > <https://reviews.apache.org/r/321/diff/1/?file=9780#file9780line62>
> >
> > Should probably be a static class.
> >
> > Should this go into SchemaUtil or similar? Seems generally useful.
Makes sense. However, this is a different representation than the official Schema format. I feel this format is less confusing for debuging.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > /trunk/src/org/apache/pig/scripting/js/JsFunction.java, line 48
> > <https://reviews.apache.org/r/321/diff/1/?file=9780#file9780line48>
> >
> > String functionSchemaStr = functionName + ".outputSchema";
> >
> > .. 3 times is too many :)
> >
> > why does jsEval need the same string passed in twice as arguments? That might need a comment.
one is the string to eval, the other one is the name of the script (used in error message). I could not come up with a better idea (it was late :P). I'll add doc to make it more obvious.
- Julien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/#review118
-----------------------------------------------------------
On 2011-01-13 19:01:24, Julien Le Dem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/321/
> -----------------------------------------------------------
>
> (Updated 2011-01-13 19:01:24)
>
>
> Review request for pig and Richard Ding.
>
>
> Summary
> -------
>
> This is not a final patch.
> I've limited modifications to the existing framework as much as possible.
> However some changes where necessary. They are backward compatible.
>
>
> This addresses bug PIG-1794.
> https://issues.apache.org/jira/browse/PIG-1794
>
>
> Diffs
> -----
>
> /trunk/src/org/apache/pig/Main.java 1056815
> /trunk/src/org/apache/pig/scripting/Pig.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
> /trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
> /trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
> /trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/321/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Julien
>
>
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Julien Le Dem <ju...@ledem.net>.
> On 2011-01-14 15:49:04, Dmitriy Ryaboy wrote:
> > Minor tweaks below.
> > Needs the ant/ivy work.
> > Should probably be integrated in such a way that only a specific ant target builds js support and bundles Rhino; we shouldn't pack everything in by default.
It uses the Rhino bundled in the JDK. Hence the sun.org.mozilla... which is not ideal. I'll convert to an official Rhino distrib.
my comments inline. (when none, it means I do not feel strongly about it)
- Julien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/#review118
-----------------------------------------------------------
On 2011-01-13 19:01:24, Julien Le Dem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/321/
> -----------------------------------------------------------
>
> (Updated 2011-01-13 19:01:24)
>
>
> Review request for pig and Richard Ding.
>
>
> Summary
> -------
>
> This is not a final patch.
> I've limited modifications to the existing framework as much as possible.
> However some changes where necessary. They are backward compatible.
>
>
> This addresses bug PIG-1794.
> https://issues.apache.org/jira/browse/PIG-1794
>
>
> Diffs
> -----
>
> /trunk/src/org/apache/pig/Main.java 1056815
> /trunk/src/org/apache/pig/scripting/Pig.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
> /trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
> /trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
> /trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/321/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Julien
>
>
Re: Review Request: Javascript support for Pig UDFs and embedding in
scripting languages
Posted by Dmitriy Ryaboy <dv...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/321/#review118
-----------------------------------------------------------
Minor tweaks below.
Needs the ant/ivy work.
Should probably be integrated in such a way that only a specific ant target builds js support and bundles Rhino; we shouldn't pack everything in by default.
/trunk/src/org/apache/pig/Main.java
<https://reviews.apache.org/r/321/#comment230>
lots of red whitespace makes dmitriy sad.
/trunk/src/org/apache/pig/Main.java
<https://reviews.apache.org/r/321/#comment232>
Throw in a comment that tells people to use run() instead of main() if they don't want the jvm to disappear?
/trunk/src/org/apache/pig/Main.java
<https://reviews.apache.org/r/321/#comment231>
I am worried about having to keep changing this for every language even though we are supposed to have a general jvm-based-language framework. Is there a way to delegate this to the scripting implementation, or to avoid going into this code block?
/trunk/src/org/apache/pig/scripting/ScriptPigContext.java
<https://reviews.apache.org/r/321/#comment233>
yes thank you!
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment248>
String functionSchemaStr = functionName + ".outputSchema";
.. 3 times is too many :)
why does jsEval need the same string passed in twice as arguments? That might need a comment.
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment249>
extra whitespace
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment250>
Should probably be a static class.
Should this go into SchemaUtil or similar? Seems generally useful.
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment252>
String str (for less confusion)?
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment251>
Please use StringBuilder
Also, it's a personal style thing, but do you mind putting spaces between operators between operators and operands?
meaning, foo = bar instead of foo=bar
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment253>
@SuppressWarnings("rawtypes")
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment254>
you are logging the debug info slightly differently in this case and the above case, which makes it hard to grep through the logs and such. Put this logging into jsScriptEngine.jsCall so that it is consistent, and DRYes up the code a bit?
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment255>
Object eval = (outputSchema.size() == 1) ? evalTuple.get(0) : evalTuple;
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment256>
@SuppressWarnings("rawtypes")
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment257>
you seem to always do " " + indent. Shouldn't indent just have the " " in it?
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment259>
Any reason for this not to be Scriptable's toString()?
/trunk/src/org/apache/pig/scripting/js/JsFunction.java
<https://reviews.apache.org/r/321/#comment260>
this goes away if the above becomes Scriptable's toString()
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java
<https://reviews.apache.org/r/321/#comment261>
you probably also want to catch SecurityException.
What about files in HDFS?
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java
<https://reviews.apache.org/r/321/#comment262>
Is that always safe? I think there is some edge case where either getUDFContext() or getUDFProperties() returns null..
*scratches head trying to remember*
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java
<https://reviews.apache.org/r/321/#comment263>
import EcmaError?
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java
<https://reviews.apache.org/r/321/#comment264>
Spacing is weird here
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java
<https://reviews.apache.org/r/321/#comment265>
Is the print/println specialcasing documented somewhere?
/trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java
<https://reviews.apache.org/r/321/#comment266>
LOG.debug
/trunk/src/org/apache/pig/scripting/js/Pig.java
<https://reviews.apache.org/r/321/#comment267>
that's a little odd.. maybe public class JsPig?
- Dmitriy
On 2011-01-13 19:01:24, Julien Le Dem wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/321/
> -----------------------------------------------------------
>
> (Updated 2011-01-13 19:01:24)
>
>
> Review request for pig and Richard Ding.
>
>
> Summary
> -------
>
> This is not a final patch.
> I've limited modifications to the existing framework as much as possible.
> However some changes where necessary. They are backward compatible.
>
>
> This addresses bug PIG-1794.
> https://issues.apache.org/jira/browse/PIG-1794
>
>
> Diffs
> -----
>
> /trunk/src/org/apache/pig/Main.java 1056815
> /trunk/src/org/apache/pig/scripting/Pig.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptEngine.java 1056815
> /trunk/src/org/apache/pig/scripting/ScriptPigContext.java 1056815
> /trunk/src/org/apache/pig/scripting/js/JsFunction.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/JsScriptEngine.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/js/Pig.java PRE-CREATION
> /trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java 1056815
> /trunk/test/org/apache/pig/test/TestScriptLanguageJavaScript.java PRE-CREATION
> /trunk/test/org/apache/pig/test/data/tc.js PRE-CREATION
>
> Diff: https://reviews.apache.org/r/321/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Julien
>
>