You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pig.apache.org by Russell Jurney <ru...@gmail.com> on 2013/02/17 00:56:10 UTC
Review Request: Review for PIG-2641 - add ToJson builtin to Pig
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/
-----------------------------------------------------------
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/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
Posted by Jonathan Coveney <jc...@gmail.com>.
> On Feb. 17, 2013, 1:03 a.m., Jonathan Coveney wrote:
> >
This looks useful. Have some stuff to address, but nothing too huge.
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16691
-----------------------------------------------------------
On Feb. 16, 2013, 11:56 p.m., Russell Jurney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9481/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2013, 11:56 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/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
Posted by Russell Jurney <ru...@gmail.com>.
> On Feb. 17, 2013, 1:03 a.m., Jonathan Coveney wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 34
> > <https://reviews.apache.org/r/9481/diff/1/?file=259402#file259402line34>
> >
> > What if the schema is null?
Then I'll throw an exception.
> On Feb. 17, 2013, 1:03 a.m., Jonathan Coveney wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 103
> > <https://reviews.apache.org/r/9481/diff/1/?file=259402#file259402line103>
> >
> > Maps can actually have complex values (i.e. a tuple of stuff, which is itself nested). You will need to check for that based on the Schema
Ok, but this pertains to JsonStorage as well, so should be addressed as a separate JIRA under the new static class in JsonStorage, JsonSerializer.
> On Feb. 17, 2013, 1:03 a.m., Jonathan Coveney wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 108
> > <https://reviews.apache.org/r/9481/diff/1/?file=259402#file259402line108>
> >
> > Issue with this, from json spec:
> > "An object is an unordered set of name/value pairs."
> > This means that there is no guarantee that we will deserialize tuples in the same order we serialized them, since order is not necessarily maintained.
> >
> > You could instead serialize bags and tuples as lists, and just have a convention for the name i.e. alias_tuple, and alias_bag.
Same as above. I will make a JIRA from these comments to fix this in JSonStorage.
> On Feb. 17, 2013, 1:03 a.m., Jonathan Coveney wrote:
> > src/org/apache/pig/builtin/ToJson.java, line 130
> > <https://reviews.apache.org/r/9481/diff/1/?file=259402#file259402line130>
> >
> > you should just test this on initialization, you can go through and make sure the Schema is valid
Again, will address in overhaul of JsonStorage. Mortar Data is doing this, will work with them.
> On Feb. 17, 2013, 1:03 a.m., Jonathan Coveney wrote:
> > test/org/apache/pig/test/TestToJson.java, line 79
> > <https://reviews.apache.org/r/9481/diff/1/?file=259403#file259403line79>
> >
> > add a test for map with a complex key, and add some more nested stuff.
Am focused on sharing test data with JsonStorage, and now static code for parsing. Should be addressed in another JIRA.
- Russell
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16691
-----------------------------------------------------------
On Feb. 16, 2013, 11:56 p.m., Russell Jurney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9481/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2013, 11:56 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/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
Posted by Jonathan Coveney <jc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9481/#review16691
-----------------------------------------------------------
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35397>
Is this stageful? Is there any reason it shouldn't be static?
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35398>
What if the schema is null?
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35399>
This should be lazily initialized once
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35400>
you can just annotate this class with @OutputSchema("chararray") and get rid of this method
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35402>
I'm still unsure why you need a ResourceSchema here...FieldSchema has alias info etc
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35401>
give variables meaningful names :)
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35403>
Maps can actually have complex values (i.e. a tuple of stuff, which is itself nested). You will need to check for that based on the Schema
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35405>
Issue with this, from json spec:
"An object is an unordered set of name/value pairs."
This means that there is no guarantee that we will deserialize tuples in the same order we serialized them, since order is not necessarily maintained.
You could instead serialize bags and tuples as lists, and just have a convention for the name i.e. alias_tuple, and alias_bag.
src/org/apache/pig/builtin/ToJson.java
<https://reviews.apache.org/r/9481/#comment35404>
you should just test this on initialization, you can go through and make sure the Schema is valid
test/org/apache/pig/test/TestToJson.java
<https://reviews.apache.org/r/9481/#comment35406>
add a test for map with a complex key, and add some more nested stuff.
test/org/apache/pig/test/TestToJson.java
<https://reviews.apache.org/r/9481/#comment35407>
you can do a try {} finally {} and add a removeOutput, and then get rid of it in setUp and tearDown
- Jonathan Coveney
On Feb. 16, 2013, 11:56 p.m., Russell Jurney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9481/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2013, 11:56 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/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
Posted by Russell Jurney <ru...@gmail.com>.
> 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
Posted by Russell Jurney <ru...@gmail.com>.
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 <pr...@gmail.com>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 russell.jurney@gmail.com datasyndrome.com
Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig
Posted by Prashant Kommireddi <pr...@gmail.com>.
> 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
>
>
Re: Review Request: Review for PIG-2641 - add ToJson builtin to Pig
Posted by Russell Jurney <ru...@gmail.com>.
> 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
Posted by Russell Jurney <ru...@gmail.com>.
> 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
Posted by Jonathan Coveney <jc...@gmail.com>.
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 <jk...@mortardata.com>
> 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 <prash1784@gmail.com
> > 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
Posted by Jeremy Karn <jk...@mortardata.com>.
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
<pr...@gmail.com>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
Posted by Prashant Kommireddi <pr...@gmail.com>.
-----------------------------------------------------------
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
Posted by Gunther Hagleitner <gh...@hortonworks.com>.
-----------------------------------------------------------
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
Posted by Russell Jurney <ru...@gmail.com>.
-----------------------------------------------------------
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
Posted by Russell Jurney <ru...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Please check out ny changes. I will work with Doug Daniels on fixing up the static class JsonSerializer in JsonStorage, which will effect both this and JsonStorage.
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