You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2025/08/16 17:08:42 UTC
[PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
andygrove opened a new pull request, #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
N/A
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
- Add scaladoc documentation
- Move methods around so for easier code comprehension; hasNext / next / close are in that order now, and helper methods are moved to the bottom
-
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2280511357
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -240,29 +287,37 @@ class CometExecIterator(
traceMemoryUsage()
}
- // The allocator thoughts the exported ArrowArray and ArrowSchema structs are not released,
- // so it will report:
- // Caused by: java.lang.IllegalStateException: Memory was leaked by query.
- // Memory leaked: (516) Allocator(ROOT) 0/516/808/9223372036854775807 (res/actual/peak/limit)
- // Suspect this seems a false positive leak, because there is no reported memory leak at JVM
- // when profiling. `allocator` reports a leak because it calculates the accumulated number
- // of memory allocated for ArrowArray and ArrowSchema. But these exported ones will be
- // released in native side later.
- // More to clarify it. For ArrowArray and ArrowSchema, Arrow will put a release field into the
- // memory region which is a callback function pointer (C function) that could be called to
- // release these structs in native code too. Once we wrap their memory addresses at native
- // side using FFI ArrowArray and ArrowSchema, and drop them later, the callback function will
- // be called to release the memory.
- // But at JVM, the allocator doesn't know about this fact so it still keeps the accumulated
- // number.
- // Tried to manually do `release` and `close` that can make the allocator happy, but it will
- // cause JVM runtime failure.
-
- // allocator.close()
Review Comment:
This comment refers to an `allocator` that no longer exists so it doesn't seem very useful to keep around
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282958558
##########
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:
I have updated this. I wanted to explain that this method is called both from the current class and also from other Spark threads, which is why it is synchronized
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282685256
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
+ * environments. This iterator orchestrates native query execution on Arrow columnar batches while
+ * managing sophisticated memory ownership semantics across the JNI boundary.
*
+ * '''Architecture Overview:'''
+ * - \1. Consumes input ColumnarBatch iterators from Spark operators
Review Comment:
Scalastyle required this. I do not understand why. I'll take another look today.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282629119
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
+ * environments. This iterator orchestrates native query execution on Arrow columnar batches while
+ * managing sophisticated memory ownership semantics across the JNI boundary.
*
+ * '''Architecture Overview:'''
+ * - \1. Consumes input ColumnarBatch iterators from Spark operators
Review Comment:
```suggestion
* - 1. Consumes input ColumnarBatch iterators from Spark operators
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282957342
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
Review Comment:
Thanks. i've pushed a more concise version
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#issuecomment-3197797891
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/2169?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `72.22222%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 34.64%. Comparing base ([`f09f8af`](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7280b79`](https://app.codecov.io/gh/apache/datafusion-comet/commit/7280b79c7928d3e900b8f3c27a8b4cd864d59195?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 389 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/2169?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ain/scala/org/apache/comet/CometExecIterator.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/2169?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2FCometExecIterator.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9Db21ldEV4ZWNJdGVyYXRvci5zY2FsYQ==) | 72.22% | [4 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/2169?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2169 +/- ##
=============================================
- Coverage 56.12% 34.64% -21.49%
+ Complexity 976 854 -122
=============================================
Files 119 143 +24
Lines 11743 13113 +1370
Branches 2251 2358 +107
=============================================
- Hits 6591 4543 -2048
- Misses 4012 7658 +3646
+ Partials 1140 912 -228
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/2169?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282630844
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
+ * environments. This iterator orchestrates native query execution on Arrow columnar batches while
+ * managing sophisticated memory ownership semantics across the JNI boundary.
*
+ * '''Architecture Overview:'''
+ * - \1. Consumes input ColumnarBatch iterators from Spark operators
+ * - 2. Transfers Arrow array ownership to native DataFusion execution engine via JNI (* see
+ * note below)
+ * - 3. Executes queries natively using DataFusion's columnar processing
+ * - 4. Returns results as ColumnarBatch with ownership transferred back to JVM
+ *
+ * * This isn't quite true. Comet does not currently implement best practice when passing batches
Review Comment:
> * * This isn't quite true.
is it needed??
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282712296
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
+ * environments. This iterator orchestrates native query execution on Arrow columnar batches while
+ * managing sophisticated memory ownership semantics across the JNI boundary.
Review Comment:
"sophisticated" isn't adding anything here.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed pull request #2169: chore: Improve documentation for `CometExecIterator`
URL: https://github.com/apache/datafusion-comet/pull/2169
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282717634
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
Review Comment:
I would simplify this down (along with CometBatchIterator) down to something that just clearly lays out "iterator for passing Arrow batches from JVM to Native" or vice versa. There are a lot of words here that don't really make clear what it's doing (some with https://github.com/apache/datafusion-comet/pull/2168)
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
Review Comment:
I would simplify this down (along with CometBatchIterator) down to something that just clearly lays out "iterator for passing Arrow batches from JVM to Native" or vice versa. There are a lot of words here that don't really make clear what it's doing (same with https://github.com/apache/datafusion-comet/pull/2168)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
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:
```suggestion
* because it may be invoked from Spark's task cleanup listener.
```
This sounds somewhat weird?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #2169:
URL: https://github.com/apache/datafusion-comet/pull/2169#discussion_r2282630844
##########
spark/src/main/scala/org/apache/comet/CometExecIterator.scala:
##########
@@ -35,22 +35,66 @@ import org.apache.comet.Tracing.withTrace
import org.apache.comet.vector.NativeUtil
/**
- * An iterator class used to execute Comet native query. It takes an input iterator which comes
- * from Comet Scan and is expected to produce batches of Arrow Arrays. During consuming this
- * iterator, it will consume input iterator and pass Arrow Arrays to Comet native engine by
- * addresses. Even after the end of input iterator, this iterator still possibly continues
- * executing native query as there might be blocking operators such as Sort, Aggregate. The API
- * `hasNext` can be used to check if it is the end of this iterator (i.e. the native query is
- * done).
+ * Comet's primary execution iterator that bridges JVM (Spark) and native (Rust) execution
+ * environments. This iterator orchestrates native query execution on Arrow columnar batches while
+ * managing sophisticated memory ownership semantics across the JNI boundary.
*
+ * '''Architecture Overview:'''
+ * - \1. Consumes input ColumnarBatch iterators from Spark operators
+ * - 2. Transfers Arrow array ownership to native DataFusion execution engine via JNI (* see
+ * note below)
+ * - 3. Executes queries natively using DataFusion's columnar processing
+ * - 4. Returns results as ColumnarBatch with ownership transferred back to JVM
+ *
+ * * This isn't quite true. Comet does not currently implement best practice when passing batches
Review Comment:
```suggestion
* * This isn't quite true.
```
is it needed??
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Improve documentation for `CometExecIterator` [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org