miguno/elephant-bird — comment on commit 86bfa834d06af954066d716cb060027a163ec827
File: src/java/com/twitter/elephantbird/pig/load/JsonLoader.java line 104
By miguno:

Yes, parser.parse() returns a null reference. It will do so when you pass in the string "null". And after re-reading the JSONParser source I'd even say that this is intended behavior:

From parse(Reader, ContainerFactory) javadocs
```
Parse JSON text into java object from the input source.
```

And since there are null values in JSON it seems to make sense to transform it into a Java null value during parsing.

Hence on second thought I'd suggest to use an IF-statement like you described above but instead of throwing a ParseException() -- because semantically it is actually not an exception for JSONParser -- we rather silently skip the line in case of LzoJsonReader and (essentially) return null tupleFactory_.newTuple(Maps.newHashMap()) in case of JsonLoader. Another option for JsonLoader would be to return null as in the case of e.g. ParseExceptions. I am a bit torn between the two options for JsonLoader, and have inspected the other code in this context.

In JsonLoader#getNext(Tuple) I see:

```
/**
* Return every non-null line as a single-element tuple to Pig.
*/

[...]

Tuple t = parseStringToTuple(value_);
if (t != null) {
incrCounter(LzoJsonLoaderCounters.LinesJsonDecoded, 1L);
return t;
}
```

Looking at counters I think it would be best to indeed return an empty (but non-null) Map in parseStringToTuple() as the line was parsed properly and hence the counter should be increased in getNext(). It might also make sense to return an empty Map "upstream" to Pig because the JSON null value is indeed what's actually stored in the input data. But on the other hand the javadocs for getNext() read "Return every non-null line as a single-element tuple to Pig". For me this means that it the intended behavior is to skip "null" lines in the input data, and to not returning an empty Map upstream. Now this is the exact opposite behavior.

It's your call to make. To me this appears to be similar to the question of return null or an empty array in case e.g. a search method has found no matching elements. Here, the best practice (Bloch & Co.) is to return an empty array instead of null. So I'm leaning towards the "empty Map" approach for JsonLoader#parseStringToTuple() but I do not want to break other parts of the code...