GH-765: Do not close/free imported BaseStruct objects

[pepijnve]

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.

[pepijnve]

#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?

[lidavidm]

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

[pepijnve]

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.

[lidavidm]

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?

[pepijnve]

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.

[lidavidm]

(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)?

[pepijnve]

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.

[lidavidm]

Looks reasonable to me, thanks.

[lidavidm]

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

[lidavidm]

Additionally, could we add a small unit test if possible? Otherwise LGTM

[pepijnve]

Checkstyle issues should be fixed. I'll add some unit tests next.

[pepijnve]

I've added a couple of tests. mvn verify says 👍 locally.