andygrove commented on PR #4987: URL: https://github.com/apache/datafusion-comet/pull/4987#issuecomment-5071213405
Thanks @0lai0, this is a clean removal and I appreciate that you tracked down the exact upstream fix. (Reviewed with LLM assistance.) I confirmed the core claim: the comparator fix from apache/datafusion#22174 shipped in DataFusion 54.0.0, and we pin `54.1.0` in `native/Cargo.toml`, so it's present. The precision-ignoring-when-scales-match comparison is also the right semantics for RANGE boundaries, since Spark's decimal +/- widens precision but keeps scale, so boundary and current value share a scale and differ only in precision. The updated comment explaining the DATE-vs-DECIMAL coercion difference is accurate and helpful. The twelve `checkSparkAnswerAndOperator` cases are thorough, and I like that the `DECIMAL(38,0)` case is deliberately built to stay off the overflow edge. **The overflow case needs to change from default-native to opt-in.** Right now the DECIMAL RANGE path runs natively by default, and the near-max-precision overflow divergence is captured only in prose in `operators.md`. Under our compatibility contract that's the wrong default. The native result diverges from Spark there, and the ANSI flavor is the concerning one: Spark throws on the overflow, but Comet would silently return a partition-edge value, which turns an error into a wrong answer. A behavior that isn't Spark-compatible shouldn't run by default. Two ways to resolve it, in order of preference: 1. **Make it actually compatible (best).** Fix the native boundary overflow to match Spark: `NULL` under non-ANSI, throw under ANSI. Then it stays `Compatible` with no opt-in and no caveat. This probably means an upstream DataFusion change, so it's more work, but it's the real fix. 2. **Mark it `Incompatible` and gate on opt-in.** Default falls back to Spark (exact), and a user sets `spark.comet.operator.<name>.allowIncompatible=true` to get the native path with the documented caveat. This follows the existing `CometDataWritingCommand` precedent, which returns `Incompatible(Some(...))` and only runs native when the user opts in. Documenting it as a known difference while still running native by default isn't something we can ship. Whichever route you pick, a test that actually pins the chosen behavior (rather than avoiding the edge) would be good to have. Two smaller points: - **CI hasn't run** on the branch yet, the fork workflow is waiting for maintainer approval. I'll get that going. - **Coordinate with #4974.** Both PRs edit `CometWindowExec.scala` and `window_functions.sql`, one removing the DATE fallback and one removing DECIMAL. Whichever lands second will need a rebase, and the final comment should describe both DATE and DECIMAL as native rather than leaving a stale "still falls back" line. Worth reviewing the two together. -- 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]
