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