Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Pig, mail # dev - Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig


Copy link to this message
-
Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig
Jonathan Coveney 2013-02-18, 14:23
Jeremy, I'll post your comments on the JIRA, though that's generally where
we want to keep comments :) I'll also respond
2013/2/18 Jeremy Karn <[EMAIL PROTECTED]>

> I'm not sure if this is the right place to respond, but just a couple of
> comments:
>
> 1) Our latest patch for Jira 1914 (
> https://issues.apache.org/jira/browse/PIG-1914) supports storing Maps
> with complex schemas.  I think a lot of the tests will also be useful for
> this patch.
>
> 2) I don't think we should serialize tuples as json lists.  While it
> preserves order (which is nice) I suspect most users would rather have the
> direct mapping of pig alias to json key and to have their tuples as json
> objects with key/value pairs instead of as a list which requires
> referencing fields by position.
>
>
> --
>
> Jeremy Karn / Lead Developer
> MORTAR DATA / 519 277 4391 / www.mortardata.com
>
>
> On Mon, Feb 18, 2013 at 12:51 AM, Prashant Kommireddi <[EMAIL PROTECTED]
> > wrote:
>
>>
>> -----------------------------------------------------------
>>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/9481/#review16700
>> -----------------------------------------------------------
>>
>>
>>
>> src/org/apache/pig/builtin/ToJson.java
>> <https://reviews.apache.org/r/9481/#comment35417>
>>
>>
>>     Hey Russell, you must have missed the apache header?
>>
>>
>>
>> src/org/apache/pig/builtin/ToJson.java
>> <https://reviews.apache.org/r/9481/#comment35418>
>>
>>
>>     Is this object used anywhere? You can may be get rid of this if not.
>>
>>
>>
>> src/org/apache/pig/builtin/ToJson.java
>> <https://reviews.apache.org/r/9481/#comment35419>
>>
>>
>>     This could probably be constructed lazily only once. Something like
>>
>>     ByteArrayOutputStream baos;
>>
>>     if(baos == null) {
>>         baos = new ByteArrayOutputStream(BUF_SIZE);
>>     }
>>
>>
>>
>> src/org/apache/pig/builtin/ToJson.java
>> <https://reviews.apache.org/r/9481/#comment35420>
>>
>>
>>     This can also be lazily created (once only)?
>>
>>
>>
>> test/org/apache/pig/test/TestToJson.java
>> <https://reviews.apache.org/r/9481/#comment35421>
>>
>>
>>     Looks like the Apache header is in here twice? You have one at the
>> top, this one could be removed.
>>
>>
>> - Prashant Kommireddi
>>
>>
>> On Feb. 17, 2013, 9:47 p.m., Russell Jurney wrote:
>> >
>> > -----------------------------------------------------------
>>
>> > This is an automatically generated e-mail. To reply, visit:
>> > https://reviews.apache.org/r/9481/
>> > -----------------------------------------------------------
>> >
>> > (Updated Feb. 17, 2013, 9:47 p.m.)
>>
>> >
>> >
>> > Review request for pig, Ashutosh Chauhan, Dmitriy Ryaboy, Alan Gates,
>> Jonathan Coveney, and Bill Graham.
>> >
>> >
>> > Description
>> > -------
>>
>> >
>> > This patch is for JIRA, https://issues.apache.org/jira/browse/PIG-2641PIG-2641 - which adds a ToJson builtin based on the serialization in
>> JsonStorage.
>> >
>> >
>> > This addresses bug PIG-2641.
>> >     https://issues.apache.org/jira/browse/PIG-2641
>> >
>> >
>> > Diffs
>> > -----
>> >
>> >   src/org/apache/pig/builtin/JsonStorage.java 56086ff
>> >   src/org/apache/pig/builtin/ToJson.java PRE-CREATION
>> >   test/org/apache/pig/test/TestToJson.java PRE-CREATION
>> >   test/org/apache/pig/test/data/jsonStorage1.tojson PRE-CREATION
>> >
>> > Diff: https://reviews.apache.org/r/9481/diff/
>> >
>> >
>> > Testing
>> > -------
>>
>> >
>> > This works for me locally, and there is a working unit test.
>> >
>> >
>> > Thanks,
>> >
>> > Russell Jurney
>> >
>> >
>>
>