andygrove opened a new pull request, #5595: URL: https://github.com/apache/datafusion-comet/pull/5595
## Which issue does this PR close? Part of #5574. First of a few small PRs moving `convert`-side declines into `getSupportLevel`; this one is deliberately the zero-risk slice. ## Rationale for this change A serde that reports `Compatible` from `getSupportLevel` and then returns `None` from `convert` breaks the serde invariant, and it is also the one shape the codegen dispatcher cannot see — `dispatchIfFallback` is reached only from the `Unsupported` and `Incompatible` arms of `exprToProtoInternal` (`QueryPlanSerde.scala:941` and `:968`), so a decline from inside `convert` always costs a whole-operator Spark fallback. Auditing all 35 `convert`-side declines, five are node-local *and* had no user-visible effect, because each guard was already dead or unreachable. Those are worth moving first: the change is provably behavior-preserving, and it shrinks the surface the harder follow-ups have to reason about. ## What changes are included in this PR? - **`CometFromUnixTime`** (`unixtime.scala`) and **`CometUnixTimestamp`** (`datetime.scala`) re-checked in `convert` exactly what `getSupportLevel` had already reported as `Unsupported`. `CometFromUnixTime` additionally mixes in `CodegenDispatchFallback`, so the `Unsupported` result is either dispatched or fails before `convert`; either way the declining input never reached the duplicated check. Removed. - **`CometShuffle`** (`collectionOperations.scala`) and **`CometUuid`** (`nondetermenistic.scala`) guarded a `randomSeed` that their own comments note is always defined in a resolved plan. Moved to `getSupportLevel`; `convert` now reads the seed directly, matching the existing pattern in `CometKnownFloatingPointNormalized` and `CometRandStr`. - **`CometScalarSubquery`** screened the data type in `convert`. Moved to `getSupportLevel`. The `serializeDataType` backstop stays — that is a genuinely different predicate from `supportedDataType`, as the scaladoc on `serializeDataType` spells out. Also drops four `withFallbackReason` imports that became unused. **Deliberately not included:** `CometLiteral`. Its `convert` guards look like the same pattern but are reachable, not dead — `supportedDataType` admits `CalendarIntervalType` while the literal value match has no arm for it, so a non-null calendar-interval literal really does fall through to `case dt =>`. Left as-is. ## How are these changes tested? No new tests, because there is no new behavior — the point of this slice is that the removed guards were unreachable. Covered by existing suites: - `CometTemporalExpressionSuite`, `CometUuidExpressionSuite`, `CometArrayExpressionSuite` — 92 tests - `CometSqlFileTestSuite` (includes `from_unix_time_enabled.sql`, `to_unix_timestamp_time_parser_policy.sql`, `to_unix_timestamp_time_parser_policy_corrected.sql`) and `CometExpressionSuite` — 607 tests All green on Spark 4.1. -- 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]
