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