----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9496/#review17244 -----------------------------------------------------------
Hi Jonathan, Overall looks good. My only concern is lack of unit tests. Indeed, you converted some test cases in TestBuiltin, but that doesn't seem to cover dump, describe, explain, illustrate. I am wondering whether you can add basic test cases somewhere. PIG-2994 added TestShortcuts, and that might be a good place to add new test cases? Let me know what you think. src/org/apache/pig/tools/pigscript/parser/PigScriptParser.jj <https://reviews.apache.org/r/9496/#comment36655> Please update (" ")* with (" " | "\t")*. This is fine in interactive mode since Grunt shell doesn't allow tab chars. However, it will throw an error when executing a Pig script that contains tab chars in batch mode. For example, if I have a Pig script called test.pig: -- tab after fat arrow => load '1.txt'; dump @; ./bin/pig -x local test.pig This fails because of the tab char is not recognized. But the following script works: -- tab after equal a = load '1.txt'; dump a; - Cheolsoo Park On March 1, 2013, 10:32 a.m., Jonathan Coveney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9496/ > ----------------------------------------------------------- > > (Updated March 1, 2013, 10:32 a.m.) > > > Review request for pig, Daniel Dai and Alan Gates. > > > Description > ------- > > https://issues.apache.org/jira/browse/PIG-3136 > > > This addresses bug PIG-3136. > https://issues.apache.org/jira/browse/PIG-3136 > > > Diffs > ----- > > src/org/apache/pig/PigServer.java a46cc2a > src/org/apache/pig/parser/LogicalPlanBuilder.java b705686 > src/org/apache/pig/parser/LogicalPlanGenerator.g 01dc570 > src/org/apache/pig/parser/QueryLexer.g bdd0836 > src/org/apache/pig/parser/QueryParser.g ba2fec9 > src/org/apache/pig/parser/QueryParserDriver.java a250b73 > src/org/apache/pig/tools/grunt/GruntParser.java 2658466 > src/org/apache/pig/tools/pigscript/parser/PigScriptParser.jj 109a3b2 > test/org/apache/pig/test/TestBuiltin.java 9c5d408 > > Diff: https://reviews.apache.org/r/9496/diff/ > > > Testing > ------- > > ant -Dtestcase=TestBuiltin test, and ant test-commit > > I made them TestBuiltin use it > > > Thanks, > > Jonathan Coveney > >
