comphead commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282634866
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -185,48 +266,25 @@ class CometExecIterator(
}
}
- override def hasNext: Boolean = {
- if (closed) return false
-
- if (nextBatch.isDefined) {
- return true
- }
-
- // Close previous batch if any.
- // This is to guarantee safety at the native side before we overwrite the
buffer memory
- // shared across batches in the native side.
- if (prevBatch != null) {
- prevBatch.close()
- prevBatch = null
- }
-
- nextBatch = getNextBatch
-
- if (nextBatch.isEmpty) {
- close()
- false
- } else {
- true
- }
- }
-
- override def next(): ColumnarBatch = {
- if (currentBatch != null) {
- // Eagerly release Arrow Arrays in the previous batch
- currentBatch.close()
- currentBatch = null
- }
-
- if (nextBatch.isEmpty && !hasNext) {
- throw new NoSuchElementException("No more element")
- }
-
- currentBatch = nextBatch.get
- prevBatch = currentBatch
- nextBatch = None
- currentBatch
- }
-
+ /**
+ * Releases all resources associated with this iterator including native
memory and JNI handles.
+ *
+ * '''Resource Cleanup:'''
+ * - Closes current ColumnarBatch and releases Arrow arrays
+ * - Closes NativeUtil and associated Arrow allocators
+ * - Releases native execution plan and associated memory
+ * - Marks iterator as closed to prevent further operations
+ *
+ * '''Memory Leak Prevention:''' This method is critical for preventing
memory leaks. The native
+ * side holds references to Arrow arrays and execution context that must be
explicitly released.
+ *
+ * '''Thread Safety:''' This method is synchronized to prevent race
conditions during cleanup.
+ * Multiple threads calling close() concurrently will be serialized safely.
This is important
+ * because it may be invoked from Spark's task cleanup listener.
Review Comment:
> This is important because it may be invoked from Spark's task cleanup
listener
This sounds somewhat out of context IMO?
--
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...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
