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
Jeremy Karn 2013-02-18, 14:15
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
> >
> >
>