sunchao commented on PR #5421: URL: https://github.com/apache/datafusion-comet/pull/5421#issuecomment-5593850488
@comphead updated in [6c14796f5](https://github.com/apache/datafusion-comet/commit/6c14796f5146b5b9908bb8d9b7133eabe9765c96). 1. **Independent defaults:** `supportsNativePartialToSparkFinal` now defaults to `false`. The eight previously inherited opt-ins are explicit and document their empty/all-null state. Existing admission decisions are preserved, including SUM's decimal/TRY exclusions and COUNT/AVG's directional differences. A policy test verifies that opting into Spark-state consumption cannot implicitly enable native-state production. 2. **Unrepaired paths:** you are right that my earlier reply addressed unconditional warnings rather than the conditional diagnostic proposed. When repair cannot reach a remaining incompatible native producer, it now warns and records the reason on the Spark Final. The read-only check handles AQE/reused stages and attribute-preserving unary wrappers, while stopping at already-Spark producers and completed aggregates. It does not rewrite stage data or infer buffer forwarding through arbitrary branching/changed-output operators. Tests cover these boundaries and ensure the warning is not repeated when the rule runs again, with fallback logging both enabled and disabled. 3. **Shared restoration:** Celeborn and post-conversion repair now use the same helper to restore a Partial, retain its current children, tag it, and record its explanation. I kept their walks separate: Celeborn must restore native ancestors such as Sort and stop at exchanges, whereas `revertChain` crosses unmaterialized exchanges and follows aggregate chains. Directly substituting that traversal would break the existing Sort-ancestor regression. The early tagging and later transition-reversion checks also retain their distinct planning responsibilities. 4. **Immediate explanation:** the shared helper records `withFallbackReason` alongside `COMET_UNSAFE_PARTIAL`. A synthetic non-native-child regression verifies that the explanation does not depend on another Partial serde call. Validation: 208 tests passed on Spark 3.4.3/JDK 17 and 123 on Spark 4.1.3/JDK 21; compilation, Spotless, Scalastyle, and diff checks passed. These runs reused a native library built from identical native sources, not a fresh native rebuild. CI for the new commit is pending. -- 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]
