sunchao commented on PR #5470: URL: https://github.com/apache/datafusion-comet/pull/5470#issuecomment-5594785287
Thanks @viirya. Addressed the test and validation feedback in `2ea7ed82c`: - Added one AQE-enabled COUNT case to the existing regressions. It checks different final projections against Spark and explicit expected rows, requires an adaptive plan, and checks that equivalent aliases reuse an exchange above the native Final aggregate. The plan checks now traverse adaptive/query-stage wrappers. - Refreshed the validation section to distinguish the current commit's local results from historical CI on `b448894b`. All five committed regressions pass on Spark 3.4.3 / JDK 17 and Spark 4.1.3 / JDK 21, for 10 passing tests. Hosted CI for the new commit is pending. I kept `override lazy val allAttributes` in the shared implementation for Spark 3 compatibility. Both [Spark 3.4.3](https://github.com/apache/spark/blob/v3.4.3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L614) and [Spark 3.5.9](https://github.com/apache/spark/blob/v3.5.9/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala#L579) declare the inherited member as a `lazy val`; Scala rejects overriding it with a `def` (confirmed with Scala 2.12 and 2.13 compiler probes). I agree with the memory-retention rationale in newer Spark. Adopting `def` only on Spark 4 would require a version-specific implementation, so I left that outside this narrow correctness fix. -- 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]
