peter-toth commented on PR #57241:
URL: https://github.com/apache/spark/pull/57241#issuecomment-4979380792

   The fix looks correct and the shape is right -- keeping it in the shared 
`getPercentile` covers `percentile` / `percentile_cont` / `median` and both 
`WITHIN GROUP` forms in one line, and `percentile_disc` / `percentile_approx` 
are untouched since they return observed values rather than interpolating.
   
   One thing the `DataFrameAggregateSuite` change surfaces: `percentile(year, 
0.3)` on the `Java` group goes `2012.2999999999997` -> `2012.3`, i.e. this 
isn't only a large-value change -- ordinary inputs shift in the last ULP too. 
Since exact `percentile` / `percentile_cont` / `median` can now return a value 
that differs from earlier releases, it might be worth a 
`docs/sql-migration-guide.md` entry to call out the behavior change. It's only 
~1 ULP and the new value is the more-correct one, so IMO no legacy config is 
needed -- just a note.
   
   IMO this reads more as a numerical-robustness improvement than a correctness 
bug fix: the old form was a standard, valid interpolation, results only move in 
the last ULP, and no documented monotonicity or exact-rounding guarantee is 
being broken. So shipping it in the next feature release without backporting to 
maintenance branches seems right.
   
   Suggested migration-guide bullet, under `## Upgrading from Spark SQL 4.2 to 
4.3`:
   
   > - Since Spark 4.3, the exact `percentile`, `percentile_cont`, and `median` 
aggregate functions (including their `WITHIN GROUP (ORDER BY ...)` forms) 
compute the linear interpolation between two neighboring values as `lower + 
fraction * (higher - lower)` instead of `(1 - fraction) * lower + fraction * 
higher`. The two are equal in exact arithmetic, but the new form is 
monotonically non-decreasing in the requested percentage and avoids a rounding 
error the old form could introduce. As a result these functions may return a 
value that differs from earlier releases in the last ULP. `percentile_disc` and 
`percentile_approx` are unaffected.
   


-- 
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]

Reply via email to