ARROW-1717: Refactor JsonReader #1290
Conversation
|
cc @BryanCutler @siddharthteotia This patch cleans up JsonReader. |
|
+1, I prefer it this way to keep the vector classes cleaner |
icexelloss
changed the title
icexelloss
changed the title
icexelloss
changed the title
|
This also fixes ARROW-1431 |
| ArrowBuf buf = allocator.buffer(bufferSize); | ||
|
|
||
| // C++ integration test fails without this. | ||
| buf.setZero(0, bufferSize); |
There was a problem hiding this comment.
|
Ping @siddharthteotia Thoughts? |
|
LGTM. My two cents: We should be little careful of adding patches to refactor branch since that essentially requires to grab and retest the changes downstream with Dremio. Currently there are two patches in refactor branch and there is a 3rd one coming in (changes that were discovered as part of testing in Dremio). So there is a good chance of conflict as well. My suggestion is that if there is some cleanup necessary, it can easily be done after refactor branch is merged into master. |
@siddharthteotia When are you planning to merge refactor branch to master? |
|
Let's merge this patch? We can discuss what should we do to concurrently work on refactor. I think @BryanCutler also has a patch that he wants to merge in (arrow ipc refactor). |
|
Ping @siddharthteotia When do you plan to merge this branch into master? In the meantime, should we wait until then before working on new refactor-related patches? Just want to make sure we have a way to make forward progress independently. |
|
There are about 4 regression test failures remaining to be fixed. Rest of the functional testing looks good with Dremio. Perf testing in progress. This week, we should be able to start the process of formal approval of merging this branch into master. As I had mentioned earlier about the 3rd patch w.r.t changes made to Arrow as part of testing in Dremio, the PR should land in by EOD. |
|
Thanks for the update. I think this patch shouldn't affect Dremio tests, it only touches json reader and integration test. How about we merge this? Also, should we work on items on https://issues.apache.org/jira/browse/ARROW-1463 after the merge? |
|
Sure, let's merge this since the changes are fairly isolated. I will be putting up third patch today EOD. Our performance testing is complete and we have seen overall improvement in TPCH. Will send out an email on the mailing list updating everyone and starting the formal process of merging java-vector-refactor into master. Will also talk about the follow-up items. Are the integration test issues fixed? I suppose they are not JAVA vector code problem. cc @jacques-n |
|
The integration tests passed. It failed previously due to issue with JsonReader during refactoring. |
|
+1 |
|
I'm going to test this locally and then merge into the refactor branch |
|
Merged in a2fa461, thanks all! |
4 participants
This patch: