andygrove commented on PR #5421: URL: https://github.com/apache/datafusion-comet/pull/5421#issuecomment-5441305383
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. I read the existing thread and do not want to relitigate the "what does this add on top of #5420" question, which is already being worked out between you two. Two things I did not see raised there. **`modes.distinct == Seq(Final)` misses the distinct-rewrite final stage** Both `tagUnsafePartialAggregates` and the new `revertUnsafePartialAggregates` gate on `agg.aggregateExpressions.map(_.mode).distinct == Seq(Final)`. Spark's `AggUtils.planAggregateWithOneDistinct` produces a fourth stage whose modes are `Final` for the regular aggregates and `Complete` for the distinct one, so a query like ```sql SELECT AVG(CAST(v AS DECIMAL(20,2))), COUNT(DISTINCT k) FROM t GROUP BY g ``` lands on `Seq(Final, Complete)` and matches neither guard. The `Complete` half reads raw input so it is harmless on its own, but the `Final` half still consumes the buffer produced by stage three, which is exactly the boundary this PR exists to protect. Is that shape actually safe for some reason I am missing, or should the predicate be "contains `Final`" rather than "is exactly `Final`"? The description mentions distinct aggregations with intermediate merge stages as an affected case, which is what made me look. The new `CometExecRuleSuite` test does cover `distinct=true`, but it asserts zero `CometHashAggregateExec` in the fallback configurations, and I think that outcome comes from the pre-existing tagging plus the `missingCometProducer` cascade rather than from `revertChain` reaching the stage-four aggregate. It would be worth adding an assertion that distinguishes those two mechanisms, otherwise the test passes whether or not the new pass handles the shape. **Re-running `transform` over a partially converted subtree** The `transformUp` body calls `transform(agg.withNewChildren(Seq(child)))`. The `child` returned by `revertChain` is Spark operators down to the point where native work resumes, so the subtree handed back to `transform` still contains `CometFilterExec`, `CometScanExec`, and friends from the first pass. Is `transform` idempotent over already-converted operators? I would expect it to be, but the comment above the call only explains why rebuilding is necessary, not that re-entry is safe. A note there would help, and if there is any doubt, a test that puts a second unsafe `Final` above the first would exercise the nested case where `transformUp` triggers this twice on overlapping subtrees. Related to that, is there a bound on how much work this can redo? With `transformUp`, N stacked unsafe finals means N re-conversions of progressively larger subtrees. Probably irrelevant for real plans, but worth knowing it is bounded. -- 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]
