Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
lidavidm merged PR #766: URL: https://github.com/apache/arrow-java/pull/766 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
pepijnve commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2912226154 I've added a couple of test. `mvn verify` says :+1: locally. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
lidavidm commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2911601263 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 ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
pepijnve commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2911776174 Checkstyle issues should be fixed. I'll add some unit tests next. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
lidavidm commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2911602224 Additionally, could we add a small unit test if possible? Otherwise LGTM -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
pepijnve commented on code in PR #766:
URL: https://github.com/apache/arrow-java/pull/766#discussion_r2108516403
##
c/src/main/java/org/apache/arrow/c/Data.java:
##
@@ -383,29 +521,52 @@ public static VectorSchemaRoot importVectorSchemaRoot(
* @param array C data interface struct holding the record batch data
(optional)
* @param schema C data interface struct holding the record batch schema
* @param provider Dictionary provider to load dictionary vectors to
(optional)
+ * @param closeImportedStructs if true, the ArrowArray struct will be closed
when this method completes successfully and
+ * the ArrowSchema struct will be always be
closed.
* @return Imported vector schema root
*/
public static VectorSchemaRoot importVectorSchemaRoot(
- BufferAllocator allocator,
- ArrowArray array,
- ArrowSchema schema,
- CDataDictionaryProvider provider) {
+BufferAllocator allocator,
+ArrowArray array,
+ArrowSchema schema,
+CDataDictionaryProvider provider,
+boolean closeImportedStructs) {
VectorSchemaRoot vsr =
-VectorSchemaRoot.create(importSchema(allocator, schema, provider),
allocator);
+VectorSchemaRoot.create(importSchema(allocator, schema, provider,
closeImportedStructs), allocator);
if (array != null) {
- importIntoVectorSchemaRoot(allocator, array, vsr, provider);
+ importIntoVectorSchemaRoot(allocator, array, vsr, provider,
closeImportedStructs);
}
return vsr;
}
/**
- * Import an ArrowArrayStream as an {@link ArrowReader}.
+ * Equivalent to calling {@link ##importArrayStream(BufferAllocator,
ArrowArrayStream, boolean) importArrayStream(allocator, stream, true)}.
*
* @param allocator Buffer allocator for allocating the output data.
* @param stream C stream interface struct to import.
* @return Imported reader
+ * @see #importArrayStream(BufferAllocator, ArrowArrayStream, boolean)
*/
public static ArrowReader importArrayStream(BufferAllocator allocator,
ArrowArrayStream stream) {
-return new ArrowArrayStreamReader(allocator, stream);
+return importArrayStream(allocator, stream, true);
+ }
+
+ /**
+ * Import an ArrowArrayStream as an {@link ArrowReader}.
+ *
+ * On successful completion, the ArrowArrayStream struct will have been
moved (as per the C data interface specification)
+ * to a private object held alive by the resulting ArrowReader.
+ *
+ * @param allocator Buffer allocator for allocating the output data.
+ * @param stream C stream interface struct to import.
+ * @param closeImportedStructs if true, the ArrowArrayStream struct will be
closed when this method completes successfully
+ * @return Imported reader
+ */
+ public static ArrowReader importArrayStream(BufferAllocator allocator,
ArrowArrayStream stream, boolean closeImportedStructs) {
+ArrowArrayStreamReader reader = new ArrowArrayStreamReader(allocator,
stream);
+ if (closeImportedStructs) {
+ stream.close();
+ }
+ return reader;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
lidavidm commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2910822927 Looks reasonable to me, thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
lidavidm commented on code in PR #766:
URL: https://github.com/apache/arrow-java/pull/766#discussion_r2107950825
##
c/src/main/java/org/apache/arrow/c/Data.java:
##
@@ -383,29 +521,52 @@ public static VectorSchemaRoot importVectorSchemaRoot(
* @param array C data interface struct holding the record batch data
(optional)
* @param schema C data interface struct holding the record batch schema
* @param provider Dictionary provider to load dictionary vectors to
(optional)
+ * @param closeImportedStructs if true, the ArrowArray struct will be closed
when this method completes successfully and
+ * the ArrowSchema struct will be always be
closed.
* @return Imported vector schema root
*/
public static VectorSchemaRoot importVectorSchemaRoot(
- BufferAllocator allocator,
- ArrowArray array,
- ArrowSchema schema,
- CDataDictionaryProvider provider) {
+BufferAllocator allocator,
+ArrowArray array,
+ArrowSchema schema,
+CDataDictionaryProvider provider,
+boolean closeImportedStructs) {
VectorSchemaRoot vsr =
-VectorSchemaRoot.create(importSchema(allocator, schema, provider),
allocator);
+VectorSchemaRoot.create(importSchema(allocator, schema, provider,
closeImportedStructs), allocator);
if (array != null) {
- importIntoVectorSchemaRoot(allocator, array, vsr, provider);
+ importIntoVectorSchemaRoot(allocator, array, vsr, provider,
closeImportedStructs);
}
return vsr;
}
/**
- * Import an ArrowArrayStream as an {@link ArrowReader}.
+ * Equivalent to calling {@link ##importArrayStream(BufferAllocator,
ArrowArrayStream, boolean) importArrayStream(allocator, stream, true)}.
*
* @param allocator Buffer allocator for allocating the output data.
* @param stream C stream interface struct to import.
* @return Imported reader
+ * @see #importArrayStream(BufferAllocator, ArrowArrayStream, boolean)
*/
public static ArrowReader importArrayStream(BufferAllocator allocator,
ArrowArrayStream stream) {
-return new ArrowArrayStreamReader(allocator, stream);
+return importArrayStream(allocator, stream, true);
+ }
+
+ /**
+ * Import an ArrowArrayStream as an {@link ArrowReader}.
+ *
+ * On successful completion, the ArrowArrayStream struct will have been
moved (as per the C data interface specification)
+ * to a private object held alive by the resulting ArrowReader.
+ *
+ * @param allocator Buffer allocator for allocating the output data.
+ * @param stream C stream interface struct to import.
+ * @param closeImportedStructs if true, the ArrowArrayStream struct will be
closed when this method completes successfully
+ * @return Imported reader
+ */
+ public static ArrowReader importArrayStream(BufferAllocator allocator,
ArrowArrayStream stream, boolean closeImportedStructs) {
+ArrowArrayStreamReader reader = new ArrowArrayStreamReader(allocator,
stream);
+ if (closeImportedStructs) {
+ stream.close();
+ }
+ return reader;
Review 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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
pepijnve commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2904649965 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
lidavidm commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2904232162 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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
pepijnve commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2904118948 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
lidavidm commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2903486262 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
pepijnve commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2903458783 https://github.com/apache/arrow-java/issues/763#issuecomment-2903439789 suggests adding new or overloaded methods that do not call close instead of introducing a breaking change. Is there any preference wrt naming these? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-765: Do not close/free imported BaseStruct objects [arrow-java]
github-actions[bot] commented on PR #766: URL: https://github.com/apache/arrow-java/pull/766#issuecomment-2903448895 Thank you for opening a pull request! Please label the PR with one or more of: - bug-fix - chore - dependencies - documentation - enhancement Also, add the 'breaking-change' label if appropriate. See [CONTRIBUTING.md](https://github.com/apache/arrow-java/blob/main/CONTRIBUTING.md) for details. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
