andygrove commented on PR #4792: URL: https://github.com/apache/datafusion-comet/pull/4792#issuecomment-4895501437
Thanks for the quick turnaround. The follow-up addresses everything from the last round. - The `expressions.md` rows for `median`, `percentile`, and `percentile_cont` now correctly say these run natively, with the array/frequency/`DESC` fallbacks still called out. - Routing `CometHashAggregateExec.createExec` through `op.output` directly is the right call, since a plain `HashAggregateExec` never carries a `TypedImperativeAggregate`. The widened `subsetOf(Set(Partial, PartialMerge))` check stays where it belongs on `CometBaseAggregate` for the ObjectHash path. - `test_percentile_special` now exercises `NaN`, `-0.0`, and `Infinity` end to end, so `spark_double_cmp` ordering is covered by CI. One thing still open from before: this touches a performance-sensitive aggregate path and calls `select_nth_unstable_by` twice per output. Could you share before/after numbers from `CometAggregateExpressionBenchmark`, or confirm there's no meaningful regression? Happy to approve once that's settled. _Disclosure: I used an LLM to help review this PR and draft this comment._ -- 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]
