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

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Gunther Hagleitner 2013-02-18, 01:21

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16698
-----------------------------------------------------------
Looks good to me.

- Gunther Hagleitner
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-2641 PIG-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
>
>

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Prashant Kommireddi 2013-02-18, 05:51

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

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

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

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Russell Jurney 2013-02-18, 22:43

> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 38
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line38>
> >
> > This could probably be constructed lazily only once. Something like
> >
> > ByteArrayOutputStream baos;
> >
> > if(baos == null) {
> > baos = new ByteArrayOutputStream(BUF_SIZE);
> > }

   ByteArrayOutputStream baos;
   if(baos == null) {
   baos = new ByteArrayOutputStream(BUF_SIZE);
   }

does not parse
> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 40
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line40>
> >
> > This can also be lazily created (once only)?

It can't be created once because it must be parametized by baos.
> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 30
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line30>
> >
> > Is this object used anywhere? You can may be get rid of this if not.

Thanks
- Russell
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16700
-----------------------------------------------------------
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-2641 PIG-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
>
>

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Russell Jurney 2013-02-18, 23:02

> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 38
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line38>
> >
> > This could probably be constructed lazily only once. Something like
> >
> > ByteArrayOutputStream baos;
> >
> > if(baos == null) {
> > baos = new ByteArrayOutputStream(BUF_SIZE);
> > }
>
> Russell Jurney wrote:
> ByteArrayOutputStream baos;
> if(baos == null) {
> baos = new ByteArrayOutputStream(BUF_SIZE);
> }
>
> does not parse

Thanks, I did this.
> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 40
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line40>
> >
> > This can also be lazily created (once only)?
>
> Russell Jurney wrote:
> It can't be created once because it must be parametized by baos.

Thanks, I did this too.
- Russell
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16700
-----------------------------------------------------------
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-2641 PIG-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
>
>

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Russell Jurney 2013-02-18, 23:03

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

(Updated Feb. 18, 2013, 11:03 p.m.)
Review request for pig, Ashutosh Chauhan, Dmitriy Ryaboy, Alan Gates, Jonathan Coveney, and Bill Graham.
Changes
-------

Implemented all fixes recommended from Gunther, except reusing json - that gives bad output.
Description
-------

This patch is for JIRA, https://issues.apache.org/jira/browse/PIG-2641 PIG-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 (updated)
-----

  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

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Russell Jurney 2013-02-18, 23:04

> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 40
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line40>
> >
> > This can also be lazily created (once only)?
>
> Russell Jurney wrote:
> It can't be created once because it must be parametized by baos.
>
> Russell Jurney wrote:
> Thanks, I did this too.

Actually, this one does not work. The json output is bad, even when I flush.
> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 38
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line38>
> >
> > This could probably be constructed lazily only once. Something like
> >
> > ByteArrayOutputStream baos;
> >
> > if(baos == null) {
> > baos = new ByteArrayOutputStream(BUF_SIZE);
> > }
>
> Russell Jurney wrote:
> ByteArrayOutputStream baos;
> if(baos == null) {
> baos = new ByteArrayOutputStream(BUF_SIZE);
> }
>
> does not parse
>
> Russell Jurney wrote:
> Thanks, I did this.

I had to do baos.reset() each execution to reuse this.
- Russell
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16700
-----------------------------------------------------------
On Feb. 18, 2013, 11:03 p.m., Russell Jurney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9481/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2013, 11:03 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-2641 PIG-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
>
>

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Prashant Kommireddi 2013-02-18, 23:06

> On Feb. 18, 2013, 5:51 a.m., Prashant Kommireddi wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 38
> > <https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line38>
> >
> > This could probably be constructed lazily only once. Something like
> >
> > ByteArrayOutputStream baos;
> >
> > if(baos == null) {
> > baos = new ByteArrayOutputStream(BUF_SIZE);
> > }
>
> Russell Jurney wrote:
> ByteArrayOutputStream baos;
> if(baos == null) {
> baos = new ByteArrayOutputStream(BUF_SIZE);
> }
>
> does not parse
>
> Russell Jurney wrote:
> Thanks, I did this.
>
> Russell Jurney wrote:
> I had to do baos.reset() each execution to reuse this.

"does not parse"? Did not get that. Wouldn't declaring it as a class var and initializing only once work?

public class ToJson extends EvalFunc<String> {
   private static ByteArrayOutputStream baos = null;

   public String exec(Tuple input) throws IOException {
   if(input == null ......)
   //

   if(baos == null) {
   baos = new ByteArrayOutputStream(BUF_SIZE);
   }

   //
   //

}
- Prashant
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16700
-----------------------------------------------------------
On Feb. 18, 2013, 11:03 p.m., Russell Jurney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9481/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2013, 11:03 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-2641 PIG-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
>
>

Review Request: Review for PIG-3190, add LuceneTokenize and SnowballTokenize

Russell Jurney 2013-02-18, 23:16

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

Review request for pig, Alan Gates, Prashant Sharma, Jonathan Coveney, and Gunther Hagleitner.
Description
-------

PIG-3190 adds two 'sane' tokenizers to Pig
This addresses bug PIG-3190.
   https://issues.apache.org/jira/browse/PIG-3190
Diffs
-----

  ivy.xml 70e8d50
  src/org/apache/pig/builtin/LuceneTokenize.java PRE-CREATION
  src/org/apache/pig/builtin/SnowballTokenize.java PRE-CREATION
  test/org/apache/pig/test/TestLuceneTokenize.java PRE-CREATION
  test/org/apache/pig/test/TestSnowballTokenize.java PRE-CREATION
  test/org/apache/pig/test/data/ExpectedLuceneTokens.txt PRE-CREATION
  test/org/apache/pig/test/data/ExpectedSnowballTokens.txt PRE-CREATION
  test/org/apache/pig/test/data/InputFiles/ten_enron_emails.txt PRE-CREATION

Diff: https://reviews.apache.org/r/9511/diff/
Testing
-------

Runs locally for me, two unit tests pass
Thanks,

Russell Jurney

Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig

Russell Jurney 2013-02-18, 23:29

Thanks, I was able to declare baos as a class variable and then
re-initialize it via reset. json would not work this way though, the output
was corrupted, even when I flushed.
On Mon, Feb 18, 2013 at 3:06 PM, Prashant Kommireddi <[EMAIL PROTECTED]>wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9481/
>
> On February 18th, 2013, 5:51 a.m., *Prashant Kommireddi* wrote:
>
> src/org/apache/pig/builtin/ToJson.java<https://reviews.apache.org/r/9481/diff/2/?file=259422#file259422line38> (Diff
> revision 2)
>
> None
>
> {'text': ' public String exec(Tuple input) throws IOException {', 'line': 26}
>
> 38
>
> ByteArrayOutputStream baos = new ByteArrayOutputStream(BUF_SIZE);
>
> This could probably be constructed lazily only once. Something like
>
> ByteArrayOutputStream baos;
>
> if(baos == null) {
> baos = new ByteArrayOutputStream(BUF_SIZE);
> }
>
> On February 18th, 2013, 10:43 p.m., *Russell Jurney* wrote:
>
> ByteArrayOutputStream baos;
> if(baos == null) {
> baos = new ByteArrayOutputStream(BUF_
> SIZE);
> }
>
> does not parse
>
> On February 18th, 2013, 11:02 p.m., *Russell Jurney* wrote:
>
> Thanks, I did this.
>
> On February 18th, 2013, 11:04 p.m., *Russell Jurney* wrote:
>
> I had to do baos.reset() each execution to reuse this.
>
> "does not parse"? Did not get that. Wouldn't declaring it as a class var and initializing only once work?
>
> public class ToJson extends EvalFunc<String> {
> private static ByteArrayOutputStream baos = null;
>
> public String exec(Tuple input) throws IOException {
> if(input == null ......)
> //
>
> if(baos == null) {
> baos = new ByteArrayOutputStream(BUF_SIZE);
> }
>
> //
> //
>
> }
>
>
>
> - Prashant
>
> On February 18th, 2013, 11:03 p.m., Russell Jurney wrote:
> Review request for pig, Ashutosh Chauhan, Dmitriy Ryaboy, Alan Gates,
> Jonathan Coveney, and Bill Graham.
> By Russell Jurney.
>
> *Updated Feb. 18, 2013, 11:03 p.m.*
> Description
>
> This patch is for JIRA, https://issues.apache.org/jira/browse/PIG-2641 PIG-2641 - which adds a ToJson builtin based on the serialization in JsonStorage.
>
> Testing
>
> This works for me locally, and there is a working unit test.
>
> *Bugs: * 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)
>
> View Diff <https://reviews.apache.org/r/9481/diff/>
>

--
Russell Jurney twitter.com/rjurney [EMAIL PROTECTED] datasyndrome.com