andygrove opened a new pull request, #4595: URL: https://github.com/apache/datafusion-comet/pull/4595
## Which issue does this PR close? Closes #4501. ## Rationale for this change #4501 collected support-level and serde consistency follow-ups deferred from the cast expression audit in #4493. While working through them, several of the issue's line references turned out to be stale against current `main`, so the fixes here reflect the current state of `CometCast.scala` and `GenerateDocs.scala`. Findings per item: 1. `canCastFromFloat` / `canCastFromDouble` / `canCastFromDecimal` ignored `evalMode` while every other arm of `isSupported` threads it through. The native code does branch on eval mode for these narrowing casts (ANSI throws on overflow/NaN, LEGACY/TRY wrap), and `CometCastSuite` exercises them in all three modes and passes, so Comet matches Spark in every mode. Threading `evalMode` is therefore a consistency fix that changes no support-level value but lets a future per-mode divergence be reported without touching the call sites. 2. For `Cast`, `getIncompatibleReasons` / `getUnsupportedReasons` are never consumed. `cast.md` is generated directly from the per-pair `isSupported` matrix (`GenerateDocs.writeCastMatrixForMode`, including per-pair note annotations), and EXPLAIN surfaces the per-pair reason via `getSupportLevel`. `Cast` is not in `categoryPages`, the only caller of those static methods. The generic overrides were dead, so they are removed in favor of the matrix as the single source of truth. 3. The array-of-binary `Incompatible()` branch described in the issue no longer exists; binary to string (including arrays) is currently `Compatible()`. The only remaining bare `Incompatible()` was the negative-scale decimal to string branch, which now carries a concise reason. The separate `#4488` `from_utf8_unchecked` question for binary to string is a behavior change and is left to its own issue. 4. The literal short-circuit in `getSupportLevel` is correct: `convert()` folds literal casts via `cast.eval()` in Spark at planning time, so the cast never executes natively and the result matches Spark by definition. Only the comment was misleading; it is clarified. ## What changes are included in this PR? - Add an `evalMode` parameter to `canCastFromFloat`, `canCastFromDouble`, and `canCastFromDecimal` and pass it from `isSupported`. - Remove the unused `getIncompatibleReasons` / `getUnsupportedReasons` overrides from `CometCast` and document that the per-pair matrix is the canonical reason source. - Fill in the empty `Incompatible()` reason for negative-scale decimal to string. - Clarify the literal short-circuit comment in `getSupportLevel`. ## How are these changes tested? Covered by existing `CometCastSuite` coverage, which exercises the float, double, and decimal cast paths in LEGACY, ANSI, and TRY modes. The negative-scale decimal to string test's support-level assertion is updated to expect the new reason string. No behavior changes, so no golden file or generated doc regeneration is required. -- 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]
