ARROW-17962: [Java] Remove unused schema creation from try with resources #14346
Conversation
|
|
|
|
| @@ -673,8 +673,7 @@ public void testTable() { | |||
| Table table = new Table(vsr); | |||
| ) { | |||
| // Producer creates structures from existing memory pointers | |||
| try (ArrowSchema arrowSchema = ArrowSchema.wrap(consumerArrowSchema.memoryAddress()); | |||
| ArrowArray arrowArray = ArrowArray.wrap(consumerArrowArray.memoryAddress())) { | |||
| try (ArrowArray arrowArray = ArrowArray.wrap(consumerArrowArray.memoryAddress())) { | |||
There was a problem hiding this comment.
Hmm, how does this fix the issue? Closing a schema created via ArrowArray.wrap shouldn't actually do anything to the original data, from what I can see (unless ArrowBuf.close() zeroes the memory or something?)
There was a problem hiding this comment.
I'm not sure, but arrowSchema was unused. My assumption was that it was being closed at the end of the try, and also by 'something else'. It's really hard to tell how to fix it since it works in my environment, but given that the variable was unused, I thought it worth a try.
There was a problem hiding this comment.
So on the contrary I think exportTable should be passed the arrowSchema here, since below we import from consumerArrowSchema. If we never export to it, it'll be uninitialized, and I don't know off the top of my head if the buffer will be zeroed or just contain random bytes, but that could cause an exception/crash.
There was a problem hiding this comment.
The unused variable had to go anyway, but it looks like there is sill a problem. I see that the build is multi-threaded here, which makes me wonder if that could be it. Also, not sure why it only fails in this one job:
Java JNI / AMD64 manylinux2014 Java JNI (pull_request)
There was a problem hiding this comment.
So on the contrary I think
exportTableshould be passed thearrowSchemahere, since below we import fromconsumerArrowSchema. If we never export to it, it'll be uninitialized, and I don't know off the top of my head if the buffer will be zeroed or just contain random bytes, but that could cause an exception/crash.
Ok. I will look into it over the weekend.
There was a problem hiding this comment.
Ok, no pressure. Let me know if you'd like me to dig into it too.
|
CI still seems to fail here. |
|
@lidavidm Thanks for the offer. IDK if this is related or not, but there was a bug in the production code where a VSR was created for exporting, but was not closed. That is fixed here. If you could unstick the workflows when you have time that would be great. |
Also, I passed the ArrowSchema into the import method. Strictly speaking, the ArrowSchema is optional, but it might be the case in this round-trip test that if you pass one in one direction you should pass one in the other |
|
Looks like it still fails, but it's just due to Checkstyle |
|
@github-actions submit java |
|
@github-actions crossbow submit java |
|
Revision: 87bbc85 Submitted crossbow builds: ursacomputing/crossbow @ actions-4544d4bf0c
|
|
Benchmark runs are scheduled for baseline = 21dbf4a and contender = 5120e1a. 5120e1a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…rces (apache#14346) Authored-by: Larry White <lwhite1@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>
No description provided.