DRILL-4824: JSON with complex nested data produces incorrect output w… #580
Conversation
…ith missing fields - Added changes to skip empty Lists or Maps. - Changed TestJsonReader.testSplitAndTransferFailure() and TestJsonReader.testFieldSelectionBug() according to new output logic.
| || (v.getAccessor().getObject(index) instanceof List | ||
| && ((List) v.getAccessor().getObject(index)).size() == 0)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Does this handle the difference beteen a missing field, and a field that exists, but contains an empty map or list? Examples:
{ "a" : { "b" : 10 }, "c" : [ 1, 2, 3 ] } // Baseline case
{ "a" : { }, "c" : [ 4 ] } // Keep "a"?
{ "b" : [ 5 ] } // Remove a: OK
{ "a" : { "b": 20 }, "c" : [ ] } // Keep "c"?
{ "a" : { "b": 20 } } // Remove c: OK
That is, should we diffentiate between a empty map/list and a non-existent one?
The code seems to discard all empty maps & lists. Should the check actually be done in the parser to not pass along empty items? Is this possible (I'm on thin ice here in my detailed knowledge of value vectors...)
There was a problem hiding this comment.
@paul-rogers, map fields have data mode required and they are the part of the schema, that's why there are no difference between missing field in some record, and the field that exists but empty.
This fix for your example will return result
{"a":{"b":10},"c":[1,2,3]}
{"c":[4]}
{"b":[5]}
{"a":{"b":20}}
{"a":{"b":20}}
There was a problem hiding this comment.
See the JIRA entry for more comments. The key problem is that Drill does not support standard JSON rules. So, we are simply moving the problem around.
Consider this case:
{ }
{ "a": { } }
{ "a": { "b": {} } }
It appears that the code here would emit:
{ }
{ }
{ "a": { } }
If we are going to apply the empty-is-not-present rule we should do so recursively.
| mapOf( | ||
| "inner_1", listOf(), | ||
| "inner_3", mapOf())) | ||
| mapOf()) |
There was a problem hiding this comment.
The test file used here is not sufficient to fully test the code change. Perhaps create an additional test, using a new test file, that tests all the variations suggested in the comment below and in the JIRA entry.
There was a problem hiding this comment.
Overall, there is no good solution for this fix. So, we have to figure out what is good enough. I think that would be:
- Handle recursively empty structures (see comments.)
- Include tests for those cases.
The more correct JSON solution must wait for a larger project.
|
@KulykRoman @paul-rogers Is there a path to take the PR to completion? Or was it decided that things would be handled differently in another PR? |
|
@ilooner, there are quite a few open issues around JSON. There are a number of JIRA tickets that explain the issues, as in a whole section in the "Batch Handling" document on my Wiki. This is not a simple bug fix. |
|
Thanks @paul-rogers then it sounds like this PR should be closed. @KulykRoman if you'd like to look into the larger issues around JSON handling please look into the Jiras that Paul has mentioned. Or if you feel this PR has been closed in error, please feel free to reopen and address Paul's comments. |
…ith missing fields