andygrove commented on PR #6076: URL: https://github.com/apache/datafusion-comet/pull/6076#issuecomment-5778787594
This closes the update path cleanly. I read `CentralMomentAgg` and `Corr` in 3.4.3 and 4.1.3 as well, which fills the gap @sunchao flagged, and both endpoints match the two formulas you picked. The routing lines up with Spark's class hierarchy in every case too: `RegrR2 extends PearsonCorrelation`, `RegrSXX` and `RegrSYY` reduce to `RegrReplacement` which is a `CentralMomentAgg`, and `RegrSlope` and `RegrIntercept` hold a literal `VariancePop`. All six construction sites in Comet agree with that. I think the merge path keeps #6044 open, though. `variance_merge` computes `ma*ca/nc + mb*cb/nc` for the mean and `d*d*ca*cb/nc` for m2, where Spark's `mergeExpressions` computes `avg.left + deltaN*n2` and `delta*deltaN*n1*n2`. The different operation order costs a ULP on the running mean at the first merge into the empty Final buffer, and the next merge squares it. Your Scala regression writes a single file so every value reaches one partial accumulator, which is why it does not show. On your head, real Spark 4.1 against real Comet, five doubles clustered at 1e17 split across two partitions with `CometHashAggregate` in both Partial and Final: ``` Spark var_pop = 61.44000000000001 Comet var_pop = 245.76 Spark stddev_pop = 7.8383671769061705 Comet stddev_pop = 15.67673435381234 exact var_pop = 61.44 ``` A differential search over the two merge formulas diverges on about 24% of randomly clustered inputs. The single-partition control is bit-identical to Spark after your fix, so this is the half the change does not reach rather than anything wrong with it. Would you rather fold the merge into this PR, or land this and open a follow-up so #6044 stays open for the merge half? Either is fine by me. If it becomes a follow-up, a two-file version of the Scala regression would keep it honest. -- 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]
