sunchao commented on PR #5722: URL: https://github.com/apache/datafusion-comet/pull/5722#issuecomment-5554808754
## Review result **No actionable defects found** in [PR #5722](https://github.com/apache/datafusion-comet/pull/5722) at `cedc3bd99`, against base `75fdddc92`. ### Correctness The cache preserves the existing Spark type conversion and per-block validation. It retains expected types, so later blocks still undergo logical-type and Arrow validation. I traced normal exhaustion, early task completion, initialization failure, decode failure, and cleanup exceptions. Ownership is cleared before release, remaining cleanup proceeds after failures, and the production callers decode synchronously. I found no reachable release race or introduced leak. [Iterator implementation](https://github.com/apache/datafusion-comet/blob/cedc3bd99ec4580226ffe77154a18fc7058a1547/spark/src/main/scala/org/apache/spark/sql/comet/execution/shuffle/NativeBatchDecoderIterator.scala#L104) ### Performance For an iterator containing **N blocks**, schema copying, protobuf parsing, and type construction decrease from **N times to once**. The cost is two additional lifecycle JNI calls and schema metadata retained until close. Single-block iterators cannot amortize that overhead. The expected benefit is strongest with many small blocks and wide or nested schemas. **The actual speedup remains unmeasured:** the PR’s benchmark explicitly excludes JNI and schema initialization. It measures remote decoding cost, not this cache’s benefit or whole-query performance. A before/after benchmark including those costs would establish the net gain. ### Design One immutable native object owned by one iterator is an appropriate lifetime. Lazy creation avoids allocation for empty or unused streams. Keeping initialization failures outside remote-corruption reporting also preserves the right failure classification. [Native implementation](https://github.com/apache/datafusion-comet/blob/cedc3bd99ec4580226ffe77154a18fc7058a1547/native/core/src/execution/jni_api.rs#L1255) ### Abstraction & complexity The wrapper is small and serves a clear ownership purpose. I see no unnecessary framework or abstraction, and no evidence that a global cache or combined initialization/decode API would justify additional complexity. ### Validation - **64/64 reader tests passed locally** on Spark 4.1, including reuse, later-frame validation, and cleanup cases. - JVM reactor build, formatting checks, and Rust formatting passed. - Local native compilation was blocked by the dependency mirror. Tests used a checksum-verified CI library whose source tree matches this commit. - Latest CI: **59 passed, 6 pending, 7 skipped; no failures**. [CI run](https://github.com/apache/datafusion-comet/actions/runs/33989024299) I would not request code changes on this revision. -- 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]
