GH-765: Do not close/free imported BaseStruct objects - #766
Conversation
Sorry, something went wrong.
This comment has been minimized.
|
#763 (comment) suggests adding new or overloaded methods that do not call close instead of introducing a breaking change. Is there any preference wrt naming these? |
Sorry, something went wrong.
|
Perhaps by analogy with the base library, load and unload, instead of import and export? That won't tell you which method is newer, but then we can add Deprecated annotations |
Sorry, something went wrong.
|
Just FYI, I'm experimenting a bit, and neither the load variant nor the boolean flag variant are particularly appealing. load feels a bit asymmetric. There's no need for any changes in the export methods as far as I can tell, so then you have three sets of methods: export, import, and load. The overload of import with a boolean flag is an option, but a bit ugly for importVectorSchemaRoot(BufferAllocator, ArrowArray, ArrowSchema, CDataDictionaryProvider) where the behavior is controlled for both the array and the schema. Having two boolean flags seems even worser though. |
Sorry, something went wrong.
|
Hmm. A single boolean flag seems fine for importVectorSchemaRoot, IMO. Alternatively: add a constructor boolean parameter for ArrayImporter, then create a new NonOwningData or something? |
Sorry, something went wrong.
|
I've pushed a second attempt that introduces overloaded import methods with a boolean argument. I've moved all the close calls to the Data class for clarity. ArrayImporter and ArrowArrayStreamReader are both package visible classes, so the change there is not a breaking change. |
Sorry, something went wrong.
| ArrowArrayStreamReader reader = new ArrowArrayStreamReader(allocator, stream); | ||
| if (closeImportedStructs) { | ||
| stream.close(); | ||
| } | ||
| return reader; |
There was a problem hiding this comment.
(1) the indentation seems off?
(2) should we put this in a try-with-resources to at least fix the edge case noted (failure to free in case of exception)?
Sorry, something went wrong.
There was a problem hiding this comment.
Indentation fixed has been corrected.
Regarding the stream#close call, I went for maintaining the exact current behavior. The ArrowArrayStreamReader constructor did not call close in a finally block. Let me know if you would prefer to change this.
Sorry, something went wrong.
|
Looks reasonable to me, thanks. |
Sorry, something went wrong.
|
checkstyle is still unhappy: Warning: src/main/java/org/apache/arrow/c/Data.java:[361] (javadoc) SummaryJavadoc: First sentence of Javadoc is missing an ending period. Warning: src/main/java/org/apache/arrow/c/Data.java:[543] (javadoc) AtclauseOrder: Javadoc comment at column 36 has parse error. Details: mismatched input '#' expecting MEMBER while parsing REFERENCE Warning: src/main/java/org/apache/arrow/c/Data.java:[543] (javadoc) JavadocTagContinuationIndentation: Javadoc comment at column 36 has parse error. Details: mismatched input '#' expecting MEMBER while parsing REFERENCE Warning: src/main/java/org/apache/arrow/c/Data.java:[543] (javadoc) NonEmptyAtclauseDescription: Javadoc comment at column 36 has parse error. Details: mismatched input '#' expecting MEMBER while parsing REFERENCE Warning: src/main/java/org/apache/arrow/c/Data.java:[543] (javadoc) SummaryJavadoc: Javadoc comment at column 36 has parse error. Details: mismatched input '#' expecting MEMBER while parsing REFERENCE |
Sorry, something went wrong.
|
Additionally, could we add a small unit test if possible? Otherwise LGTM |
Sorry, something went wrong.
|
Checkstyle issues should be fixed. I'll add some unit tests next. |
Sorry, something went wrong.
…orted BaseStruct objects
|
I've added a couple of tests. mvn verify says 👍 locally. |
Sorry, something went wrong.
What's Changed
This PR removes the direct and indirect calls to BaseStruct#close from org.apache.arrow.c.Data. By not eagerly closing/freeing these objects callers can reuse instances multiple times.
This contains breaking changes.: a quick check in, for instance, org.apache.arrow.c.StreamTest shows that a lot of existing code is already written using try-with-resources. This change will not have any impact there. Code that does not use a try-with-resources or try-finally-close pattern and instead counts on Data to call close will report memory leaks after this change.
The second version of this PR removes the breaking change.
Closes #765.