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