0lai0 commented on PR #4987: URL: https://github.com/apache/datafusion-comet/pull/4987#issuecomment-6234186969
Thanks for the careful review. Addressed in the latest commit: - **Overflow**: `CometWindowExec` now falls back only when the bound can overflow `Decimal128`, i.e. when the largest value of the ORDER BY type plus the offset exceeds `2^127 - 1 `unscaled. DataFusion rescales both operands to the larger scale and adds them as i128, so `DECIMAL(p, s)` with `p <= 37` can never overflow and stays native; only `DECIMAL(38, s)` with an offset above ~7.01e37 falls back. - **Docs**: you're right. `WindowEvaluatorFactoryBase` builds the bound with `DecimalAddNoOverflowCheck`, so Spark neither returns NULL nor throws. I removed the "Known differences" entry and replaced it with a fallback-list entry describing the actual divergence (the overflowing native bound becomes the partition edge and takes in the `NULL` keys there). - **Tests**: added your `[0, N, NULL] / NULLS LAST` case under both ANSI settings; it falls back and returns `[2,2,1]`. With the guard removed it reproduces `[2,3,1]`. Also added a case at the largest safe offset over the type's extremes and a `NULL`, across ASC/DESC and NULLS FIRST/LAST under both ANSI settings, which stays native and matches Spark. One note: Spark negates a `PRECEDING` decimal offset with 34 significant digits, so the test uses the largest 34-digit offset under the limit. - Fixed the DataFusion 54.0.0 wording and the stale `DATE` comment in `window_functions.sql `([apache/datafusion-comet#4974](https://github.com/apache/datafusion-comet/pull/4974) has merged). While checking this I found that the DATE threshold from [apache/datafusion-comet#4974](https://github.com/apache/datafusion-comet/pull/4974) has the same NULL-key issue once arrow's chrono range overflows. I'll fix that in a separate PR under [apache/datafusion-comet#5022](https://github.com/apache/datafusion-comet/issues/5022). -- 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]
