sunchao commented on PR #5421: URL: https://github.com/apache/datafusion-comet/pull/5421#issuecomment-5442592626
Updated in [f1e5868d8](https://github.com/apache/datafusion-comet/commit/f1e5868d823e1d3e708f4697b53157ebc8a332e0). I split the compatibility checks by direction and restored COUNT's safe native-partial-to-Spark-final path. The reverse direction stays disabled for the documented AQE/count-bug reasons. The new execution test covers grouped, all-null, and empty inputs with AQE on/off, and both early and post-conversion fallback. I have not enabled the remaining functions based only on the ordinary-input sweep; their buffer layout, overflow modes, and merge behavior still need targeted validation. The PR now commits an independent `collect_list`/`collect_set` reproduction with Comet shuffle enabled but native hash partitioning disabled, using integer keys so it does not depend on #5420. Each AQE mode checks Spark-equivalent results, zero native aggregate stages, the unsafe-partial tag, and preservation of the native filter; re-enabling eligible native shuffle checks the two-native-stage control. These buffers are native arrays versus Spark's serialized binary state, so fixing AVG's empty sum/count in #5420 does not fix this boundary. The description now leads with that concrete case. #5421 remains the prerequisite, and I am synchronizing this update into #5420. For the DISTINCT concern, Spark's one-DISTINCT rewrite has intermediate PartialMerge and mixed Partial/PartialMerge stages, but its final stage contains Final-mode expressions for both ordinary and distinct functions. I checked this across the supported Spark sources and in an actual four-stage plan; there was no Final+Complete final stage. The existing regression also exercised the new repair: the early pass added zero unsafe tags, conversion produced one native aggregate, and post-conversion repair removed it. Reapplying conversion preserved the same tree. I therefore kept the Final-only condition and did not broaden it to address an unobserved mode combination. `revertChain(None)` intentionally stops at an already-Spark producer, materialized/reused stage, or unrelated boundary. An unconditional warning/assertion would flag those safe cases. Existing Comet nodes pass through conversion, and repair invokes conversion rather than recursively invoking the repair pass, so repeated-tree work is bounded by the finite plan. The current tests do not show an idempotence or planning-time defect. The separate pre-existing missing ObjectHashAggregate fallback explanation is tracked in https://github.com/apache/datafusion-comet/issues/5500; it is not introduced by this buffer-compatibility change. Validation for this update: full Spark 4.1.3 JVM reactor, Spotless, and Scalastyle passed, along with 44 planner/execution tests. Local execution reused the existing OSS native library and is not a fresh native build. CI for the new commit remains to be confirmed. -- 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]
