----------------------------------------------------------- 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 > >
