pig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lin Guo" <guolin2...@yahoo.com>
Subject Re: Review Request: Update pig parser to allow function arguments to contain multiple lines
Date Fri, 10 Dec 2010 21:29:34 GMT


> On 2010-12-10 10:40:46, Daniel Dai wrote:
> > trunk/test/org/apache/pig/test/TestParamSubPreproc.java, line 1387
> > <https://reviews.apache.org/r/154/diff/1/?file=1105#file1105line1387>
> >
> >     Can you add a test case to test QueryParser itself?

I will add more test cases to TestQueryParser. 


> On 2010-12-10 10:40:46, Daniel Dai wrote:
> > trunk/src/org/apache/pig/impl/logicalLayer/parser/QueryParser.jjt, line 1417
> > <https://reviews.apache.org/r/154/diff/1/?file=1104#file1104line1417>
> >
> >     I prefer to allow multiple line support to all strings. Only allow function
arguments taking multiple lines seems confusing to the user.

Is it okay to have all other stuffs (e.g. paths, filenames) contain multiple lines? It doesn't
seem very intuitive to me. 

In the parser definition, only "define clause" and "function argument" use StringList and
I have changed "function arguments" to use MultiLinedStringList. 
I am not sure whether there is any side effect of updating "define clause" to take MultiLinedStringList.
Any insights here? 


> On 2010-12-10 10:40:46, Daniel Dai wrote:
> > trunk/src/org/apache/pig/impl/logicalLayer/parser/QueryParser.jjt, line 969
> > <https://reviews.apache.org/r/154/diff/1/?file=1104#file1104line969>
> >
> >     The QueryParser will go away in 0.9 release. We will switch the parser (https://issues.apache.org/jira/browse/PIG-1618)
in couple of months. So shall we wait for the new parser?

Thanks a lot for your comments. 

Will the new QueryParser allow multi-lines in function arguments? If not, can we add this
as a requirement? 


- Lin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/154/#review66
-----------------------------------------------------------


On 2010-12-09 01:26:33, Lin Guo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/154/
> -----------------------------------------------------------
> 
> (Updated 2010-12-09 01:26:33)
> 
> 
> Review request for pig.
> 
> 
> Summary
> -------
> 
> Summary:
> 
> We want to extend pig parser to allow function arguments to contain multiple lines as
well as string list, like
> 
> STORE data INTO 'testOut' 
> USING storage.avro.AvroStorage (
> '{"debug": 5,
>   "data": "/user/lguo/testOut/ComponentActTracking4/part-m-00000.avro",
>   "field0": "int",
>   "field1": "def:browser_id",
>   "field3": "def:act_content" } '
> );
> 
> 
> This addresses bug PIG-1749.
>     https://issues.apache.org/jira/browse/PIG-1749
> 
> 
> Diffs
> -----
> 
>   trunk/src/org/apache/pig/impl/logicalLayer/parser/QueryParser.jjt 1040872 
>   trunk/test/org/apache/pig/test/TestParamSubPreproc.java 1040872 
> 
> Diff: https://reviews.apache.org/r/154/diff
> 
> 
> Testing
> -------
> 
>      [exec] 
>      [exec] +1 overall.  
>      [exec] 
>      [exec]     +1 @author.  The patch does not contain any @author tags.
>      [exec] 
>      [exec]     +1 tests included.  The patch appears to include 3 new or modified tests.
>      [exec] 
>      [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
>      [exec] 
>      [exec]     +1 javac.  The applied patch does not increase the total number of javac
compiler warnings.
>      [exec] 
>      [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
>      [exec] 
>      [exec]     +1 release audit.  The applied patch does not increase the total number
of release audit warnings.
>      [exec] 
>      [exec] 
> 
> 
> Thanks,
> 
> Lin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message