andygrove commented on PR #51: URL: https://github.com/apache/datafusion-java/pull/51#issuecomment-4451618306
### Code review Found 2 issues: 1. Class-level Javadoc on `DataFrame` still names only `collect` as the execution/consume method, but after this PR `executeStream` is a peer that also consumes the DataFrame and makes `close()` a no-op. Worth updating the top-of-class summary so the lifecycle paragraph covers both methods. https://github.com/apache/datafusion-java/blob/9743fde3839804fc36121b4ecad44fbc770696b6/core/src/main/java/org/apache/datafusion/DataFrame.java#L27-L35 2. `executeStreamSurvivesEarlyClose` doesn't put the `ArrowReader` in a try-with-resources, so if `loadNextBatch()` or `assertTrue(...)` throws, the reader leaks and the surrounding `BufferAllocator.close()` will fail and mask the original failure. The other tests in this file use the right idiom — recommend matching it here. https://github.com/apache/datafusion-java/blob/9743fde3839804fc36121b4ecad44fbc770696b6/core/src/test/java/org/apache/datafusion/DataFrameExecuteStreamTest.java#L131-L145 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
