Skip to content

fix: clean up CometCast support-level reporting (#4501)#4595

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:fix-4501-cast-audit-followups
Jun 4, 2026
Merged

fix: clean up CometCast support-level reporting (#4501)#4595
andygrove merged 1 commit into
apache:mainfrom
andygrove:fix-4501-cast-audit-followups

Conversation

@andygrove
Copy link
Copy Markdown
Member

@andygrove andygrove commented Jun 4, 2026

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. The issue noted that canCastFromFloat / canCastFromDouble / canCastFromDecimal ignore evalMode while every other arm of isSupported threads it through, and suggested the support level should reflect the eval mode. Investigation shows there is nothing to reflect: the native code does branch on eval mode for these narrowing casts (ANSI throws on overflow/NaN, LEGACY/TRY wrap), but Comet matches Spark in every mode, and CometCastSuite exercises them in LEGACY, ANSI, and TRY and passes. So there is no per-mode support-level divergence, and these three helpers (like canCastFromTimestamp) correctly take no evalMode. No code change needed.

  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?

  • 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, sharing the string with the test via a constant to avoid drift.
  • 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 reference the shared reason constant. No behavior changes, so no golden file or generated doc regeneration is required.

@andygrove andygrove marked this pull request as draft June 4, 2026 20:14
@andygrove andygrove force-pushed the fix-4501-cast-audit-followups branch from 86e9580 to d6d1ea4 Compare June 4, 2026 20:57
Address the cast expression audit follow-ups from apache#4493. Several of the issue's
line references were stale; the resolutions reflect current main:

- canCastFromFloat/Double/Decimal need no evalMode parameter. The native code
  branches on eval mode for these narrowing casts but matches Spark in every
  mode (CometCastSuite passes in LEGACY, ANSI, and TRY), so there is no per-mode
  support-level divergence to report. No change needed.
- Remove the vestigial getIncompatibleReasons/getUnsupportedReasons overrides.
  For Cast they are never consumed: cast.md is generated directly from the
  per-pair isSupported matrix and EXPLAIN surfaces the per-pair reason via
  getSupportLevel.
- Fill in the previously empty Incompatible() reason for negative-scale decimal
  to string, sharing the string with the test via a constant to avoid drift.
- Clarify the literal short-circuit comment in getSupportLevel: literal casts
  are folded by Spark at planning time, so they never execute natively.
@andygrove andygrove force-pushed the fix-4501-cast-audit-followups branch from d6d1ea4 to b3d4339 Compare June 4, 2026 21:02
@andygrove andygrove changed the title fix: thread evalMode and tidy cast support reasons in CometCast (#4501) fix: clean up CometCast support-level reporting (#4501) Jun 4, 2026
@andygrove andygrove marked this pull request as ready for review June 4, 2026 21:11
@andygrove
Copy link
Copy Markdown
Member Author

@coderfender could you review?

@coderfender
Copy link
Copy Markdown
Contributor

Thank you @andygrove

@andygrove andygrove merged commit 577a793 into apache:main Jun 4, 2026
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cast expression audit follow-ups (from #4493)

2 participants