mvanhorn commented on PR #26784: URL: https://github.com/apache/datafusion/pull/26784#issuecomment-6439314532
Thanks, that's right. `[f64::MAX, f64::MAX]` followed by retracting one `f64::MAX` stays at `+inf` here. It does on main too, though: `SlidingSumAccumulator` adds and subtracts plain f64, so `inf - MAX` is `inf` either way. This PR fixes the case where an input is itself non-finite; finite inputs whose sum overflows are a separate gap that predates it. Closing that gap means keeping the finite part in a form that can't overflow. Two options: 1. Accumulate finite values scaled by `2^-64` and scale back on `evaluate`. That can't overflow for fewer than 2^64 rows, but values below roughly `2^-1010` lose precision. 2. Count the finite contributions that overflowed (the way the PR counts `+inf`/`-inf`/`NaN`) and return `+inf`/`-inf` only while that count is non-zero, at the cost of a slower path once a batch overflows. I'd lean toward doing it as a follow-up with its own overflow/retraction regression tests, so this PR stays about non-finite inputs. If you'd rather have it here, I can add option 2 to this PR. <!-- r:846ecb9b453b.72256aba66f3 --> -- 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]
