andygrove commented on PR #5420: URL: https://github.com/apache/datafusion-comet/pull/5420#issuecomment-5441402654
> **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. The `AvgAccumulator::state` fix is clearly correct and clearly valuable. Spark's `Average.mergeExpressions` is a plain `sum.left + sum.right` with no coalesce, so a partial that reports a null sum for an empty partition poisons the whole final. Getting decimal to distinguish "empty partial" from "overflowed partial" through the state round trip is the fiddly part and the new accumulator tests read well. I have concerns about how much else is in here. **The decimal precision restriction on native shuffle** Changing `CometShuffleExchangeExec` so that hash partitioning keys with `precision > 18` fall back is by far the widest-reaching change in this PR, and the justification given is that different partition assignments hide an overflow in the `AVG(DISTINCT ...)` regression. That is a very large hammer for making one test deterministic. Every query that hash-partitions on a `DECIMAL(38, s)` key now loses native shuffle, whether or not it involves `AVG`. The removed TODO points at #3079 and says wider decimals need `BigDecimal` conversion before hashing. If native hashing genuinely disagrees with Spark for `precision > 18`, that is a correctness bug in its own right and it deserves its own PR, its own tests, and a note in the migration guide, rather than arriving as a side effect of an `AVG` fix. If it is not a correctness bug and this is purely about the test, could the test pin the partitioning some other way, for example by constructing the input so the overflow occurs regardless of assignment? Either way, could you say what this costs? A before and after on a query that groups by a `DECIMAL(38, 2)` key would settle whether this is a minor or a major regression. **This PR contains #5421 verbatim** That is already being discussed on the other thread so I will not repeat it, except to note that the diff here is harder to review because of it. Once the ordering is settled, rebasing so this PR shows only its own changes would help a lot. **`aggregateSupportLevel` only covers the ungrouped case** The check is `op.groupingExpressions.isEmpty`, and the docs say grouped averages stay native. I believe the reasoning is that Spark's grouped path stores the buffer in an `UnsafeRow` at the declared precision, so it overflows at the same point Comet does, while the ungrouped codegen path keeps a wider local. Could you state that explicitly in the comment? And does the same hold for `SortAggregateExec` and for grouped `ObjectHashAggregateExec` after a spill, where the buffer round-trips through a different representation? **The relaxed ANSI condition in `AvgDecimalGroupsAccumulator::evaluate`** The guard changes from `!is_not_null && count > 0 && ansi` to `!is_not_null && ansi`. That is deliberate, and the comment explains why. But it means a group whose `is_not_null` is false and whose count is zero now throws in ANSI mode where it previously did not. I convinced myself that an all-null group keeps `is_not_null` true and so is unaffected, but I would rather see that pinned by a test: ANSI mode, grouped decimal `AVG`, with one group that is entirely null alongside a group that really does overflow. If that test already exists somewhere in the 381 new lines in `CometAggregateSuite`, pointing at it in the description would be enough. **Non-local return in `CometWindowExec`** The new block uses `return None` from inside three levels of nested `match` inside `getSupportLevel`. It matches the existing style just below it, so I am not asking you to change it. But four nested matches to reach one condition is hard to follow. Would a small predicate helper, along the lines of `isMaxPrecisionDecimalAverage(windowExpr)`, make both this block and the existing one below it easier to read? **Splitting** Counting them up, this PR contains: the float `AVG` empty-partial fix, the decimal `AVG` overflow-marker plumbing, an ungrouped decimal `AVG` operator fallback, a window `AVG` fallback, a native shuffle restriction on wide decimals, and all of #5421. The first two are tightly coupled and obviously belong together. The shuffle restriction in particular looks separable and would be much easier to evaluate on its own. -- 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]
