chore: move dead and defensive serde guards out of convert - #5595
chore: move dead and defensive serde guards out of convert#5595andygrove wants to merge 2 commits into
Conversation
Five serdes report Compatible from getSupportLevel and then decline inside convert. That combination 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, so a decline from convert always costs a whole-operator Spark fallback. None of these five had a user-visible effect, because each guard was either dead or unreachable: - CometFromUnixTime and CometUnixTimestamp re-checked in convert exactly what getSupportLevel had already reported as Unsupported, so convert was never reached for the declining input. - CometShuffle and CometUuid guarded a randomSeed their own comments note is always defined in a resolved plan. Moved to getSupportLevel. - CometScalarSubquery screened the data type in convert. Moved to getSupportLevel; the serializeDataType backstop stays, since that is a different predicate from supportedDataType. CometLiteral is deliberately left alone: supportedDataType admits CalendarIntervalType but the literal value match has no arm for it, so that guard is reachable rather than dead. Related to #5574.
| return None | ||
| } | ||
|
|
||
| // getSupportLevel reports an unsupported input type before reaching here, so no re-check. |
There was a problem hiding this comment.
[P2] Keep string-input rejection ahead of the collation opt-in
On Spark 4.0, unix_timestamp(s, collate('yyyy-MM-dd', 'UTF8_LCASE')) with a non-foldable plain STRING column s makes getSupportLevel return Incompatible before it checks isSupportedInputType. With spark.comet.expression.UnixTimestamp.allowIncompatible=true, exprToProto therefore calls this method. The deleted guard returned None for that input. This now serializes the string child into the native UnixTimestamp expression. The format is not serialized, so no collated scan or Collate serializer is required. Native SparkUnixTimestamp only accepts date/timestamp inputs, and its unsupported-string error propagates instead of falling back to Spark. Could we retain the input-type rejection even when collation selects Incompatible, and cover this live-string/collated-format case in a regression test? This path is source-traced. The SQL witness was not executed.
There was a problem hiding this comment.
Good catch, and it reproduces. getSupportLevel checked collation first, so unix_timestamp(s, 'yyyy-MM-dd HH:mm:ss' COLLATE UTF8_LCASE) over a plain STRING column returned Incompatible before it ever looked at the input type, and with allowIncompatible=true that goes straight to convert — which no longer has the guard. Running it on this branch gives CometNativeException: unix_timestamp does not support input type: Utf8 instead of a fallback.
Fixed in 559c0f9 by screening the input type ahead of the collation check: a non-date/timestamp input has no native path at all, so it should report Unsupported regardless of collation, and allowIncompatible should never be able to reach it. Collation still reports Incompatible for the input types that do have a native path, so the opt-in behavior there is unchanged. Added the live-string/collated-format case to CometTemporalExpressionSuite — it fails on the parent commit with the native error above and passes with the fix.
CometUnixTimestamp.getSupportLevel checked for a non-default collation first, so unix_timestamp(s, 'fmt' COLLATE UTF8_LCASE) over a plain STRING column reported Incompatible before it ever looked at the input type. With spark.comet.expression.UnixTimestamp.allowIncompatible=true, exprToProto waves Incompatible straight through to convert, and the input-type guard that used to live there is gone as of the previous commit. The string child was therefore serialized into the native UnixTimestamp expression, whose kernel accepts only date/timestamp: CometNativeException: unix_timestamp does not support input type: Utf8 Reorder getSupportLevel so an unsupported input type reports Unsupported regardless of collation. A non-date/timestamp input has no native path at all, so allowIncompatible must not be able to reach it. Collation remains Incompatible for the input types that do have a native path, so the existing opt-in behavior is unchanged.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 559c0f9a9746595567dc5afccb47dfbc93e358c1. The previously reported string-input/collation guard issue is fixed, with no new P1/P2 findings. Current CI shows 62 successful checks and 9 skipped. No local tests were run.
2 participants
Which issue does this PR close?
Part of #5574. First of a few small PRs moving
convert-side declines intogetSupportLevel; this one is deliberately the zero-risk slice.Rationale for this change
A serde that reports
CompatiblefromgetSupportLeveland then returnsNonefromconvertbreaks the serde invariant, and it is also the one shape the codegen dispatcher cannot see —dispatchIfFallbackis reached only from theUnsupportedandIncompatiblearms ofexprToProtoInternal(QueryPlanSerde.scala:941and:968), so a decline from insideconvertalways 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) andCometUnixTimestamp(datetime.scala) re-checked inconvertexactly whatgetSupportLevelhad already reported asUnsupported.CometFromUnixTimeadditionally mixes inCodegenDispatchFallback, so theUnsupportedresult is either dispatched or fails beforeconvert; either way the declining input never reached the duplicated check. Removed.CometShuffle(collectionOperations.scala) andCometUuid(nondetermenistic.scala) guarded arandomSeedthat their own comments note is always defined in a resolved plan. Moved togetSupportLevel;convertnow reads the seed directly, matching the existing pattern inCometKnownFloatingPointNormalizedandCometRandStr.CometScalarSubqueryscreened the data type inconvert. Moved togetSupportLevel. TheserializeDataTypebackstop stays — that is a genuinely different predicate fromsupportedDataType, as the scaladoc onserializeDataTypespells out.Also drops four
withFallbackReasonimports that became unused.Deliberately not included:
CometLiteral. Itsconvertguards look like the same pattern but are reachable, not dead —supportedDataTypeadmitsCalendarIntervalTypewhile the literal value match has no arm for it, so a non-null calendar-interval literal really does fall through tocase 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 testsCometSqlFileTestSuite(includesfrom_unix_time_enabled.sql,to_unix_timestamp_time_parser_policy.sql,to_unix_timestamp_time_parser_policy_corrected.sql) andCometExpressionSuite— 607 testsAll green on Spark 4.1.