andygrove commented on PR #4987: URL: https://github.com/apache/datafusion-comet/pull/4987#issuecomment-5441745659
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. Good to see a fallback removed because the underlying problem was actually fixed upstream, and the rewritten comment in `CometWindowExec` explaining that `WindowFrameTypeCoercion` casts the DECIMAL offset while leaving the DATE offset as `IntegerType` is much more useful than the one it replaces. Turning the `expect_fallback` fixture into a plain `query` is the right signal that the guard is gone. Two things. **The new "known difference" is a silent wrong answer under ANSI** > when `current +/- offset` overflows the underlying integer, DataFusion collapses the bound to the partition edge, whereas Spark evaluates the boundary through normal decimal arithmetic (returning `NULL` under non-ANSI, throwing under ANSI) Under ANSI, Spark throws and Comet returns a result. That is not a divergence in an edge-case value, it is a query that succeeds with wrong output where Spark would have failed. Comet's usual bar for that is to fall back rather than accept it. Could the guard be narrowed rather than removed? Falling back only when the `ORDER BY` decimal is at or near `DECIMAL_MAX_PRECISION` would keep native execution for every realistic decimal while preserving Spark's behavior for the overflow case. That seems much better than trading a whole class of queries becoming native for a new silent-divergence bullet. If you would rather accept the divergence, it needs a tracking issue linked from the docs bullet. Right now that paragraph is the only record that the difference exists, and there is nowhere for an affected user to go. **Confirm the upstream fix is actually in the pinned DataFusion** The description cites apache/datafusion#22174 as fixing the precision mismatch. Which DataFusion version does Comet pin today, and does it contain that PR? If Comet is still on a version without it, removing the guard would surface the original "Uncomparable values" error rather than a wrong answer. A line in the description confirming the pinned version includes the fix would settle it, and the 255 new lines in `CometWindowExecSuite` presumably demonstrate it, but it is worth stating. -- 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]
