ulysses-you commented on PR #57742: URL: https://github.com/apache/spark/pull/57742#issuecomment-5188237700
Thanks @cloud-fan, all three are addressed in e1bbdd717e0, along with the config convergence you asked for earlier. **Policy is now a single contract.** The four configs collapse to two: | config | default | meaning | | --- | --- | --- | | `minRows` | `100000` | rows between two compaction-ratio evaluations | | `minCompaction` | `1.1` | the ratio below which the partial aggregation is bypassed | One predicate, evaluated at both check points -- periodically every `minRows` rows, and immediately before a spill. The separate no-spill/spill thresholds and the exponential resampling are gone, so the codegen and interpreted paths implement and test one invariant. `AdaptivePartialAggregationConfig` was dropped since only two values are threaded through now. `minRows` is a `longConf` so a large value genuinely disables the periodic check. **On the predicate form** -- it is written as a multiplication rather than a division: ```java processedRows < (double) totalKeys * minCompaction ``` Algebraically the same as `processedRows / totalKeys < minCompaction` for `totalKeys > 0`, but the division form has to handle `totalKeys == 0`: integer division throws, and floating-point division yields `Infinity`/`NaN` where `NaN` silently makes every comparison false. The multiplication degenerates to `rows < 0`, which is false, so the operator safely keeps aggregating without a separate guard. Cheaper too, though that is secondary since this only runs at check points. One thing worth flagging: I used strict `<`, so at exactly `rows / keys == minCompaction` the aggregation is **kept**. Your description said `<=`, which would bypass at that point. I picked the conservative direction, and `the spill check decides identically at the exact ratio boundary` locks in that choice (50 rows / 40 keys = 1.25 exactly; `1.25` keeps, `1.3` bypasses). Happy to flip it to `<=` if you would rather match DBR exactly -- it is a one-character change plus inverting that test's expectation. Benchmark numbers were removed from the PR description since they were measured under the old two-tier policy; I will regenerate and attach them. -- 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]
