feat: add codegen dispatch fallback for CometCast incompatible/unsupported cases (including legacy config paths) - #5079
Conversation
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Thanks @comphead. I like the direction of routing these casts through the codegen dispatcher instead of falling the whole projection back to Spark. I left a few comments. The main one is in the existing thread about timeParserPolicy, where I confirmed against Spark source that the policy has no effect on cast.
Also, could you fill in the PR template sections? "Closes #." is empty and the rationale and testing sections are still the template comments, even though the free-form description at the top is good.
| return Unsupported(Some(legacyCastComplexTypesToStringReason)) | ||
| if (toType == DataTypes.StringType && legacyCastComplexTypesToString && isComplexType( | ||
| fromType)) { | ||
| return Incompatible(Some(legacyCastComplexTypesToStringReason)) |
There was a problem hiding this comment.
Since CodegenDispatchFallback already routes Unsupported through the dispatcher, keeping this as Unsupported would give the same in-pipeline behavior. Switching to Incompatible additionally allows the native cast to run when spark.comet.expr.allowIncompatible=true, and the native cast produces the default formatting rather than the legacy formatting the user explicitly asked for with the legacy flag. That would be a silent results change for those users, where today they always get correct results via fallback. Was that intentional? If the goal is just to stay in the Comet pipeline, Unsupported plus the mixin achieves it without that risk.
| // into the DataFusion plan would fail in `serializeDataType`, and the codegen dispatcher | ||
| // cannot compile Variant read/write kernels either. Detect it via the version-shimmed | ||
| // `isVariantType` (which returns false on Spark 3.x where the class does not exist) and | ||
| // report `Unsupported` so the enclosing operator falls back to Spark. |
There was a problem hiding this comment.
This comment (and the one in getSupportLevel above) says the guard forces Spark fallback, but with the CodegenDispatchFallback mixin an Unsupported result first attempts the codegen dispatcher. Variant casts fall back only because the dispatcher fails to serialize VariantType in the data args or return type. The end behavior is the same, but could the comments describe the actual routing?
It might also be worth adding a Spark 4.x test that casts to and from variant, for example CAST(parse_json('{"a":1}') AS STRING) on a column, to pin down that this path cleanly falls back and matches Spark. I could not find any Variant cast coverage in the repo today.
|
Thanks @andygrove for the review, I'm going through it |
Mix CodegenDispatchFallback into CometCast so casts that Comet cannot match Spark bit-for-bit run Spark's own doGenCode inside the Comet pipeline instead of falling the full projection back to Spark: - spark.sql.legacy.castComplexTypesToString.enabled=true (struct/array/map to string) is now Incompatible, was Unsupported which forced full fallback. - Non-CORRECTED spark.sql.legacy.timeParserPolicy (LEGACY, EXCEPTION) on string to date/timestamp casts is now Incompatible. - Pre-existing Incompatible cases in CometCast (negative-scale decimal to string, float/double to decimal rounding) also start using the codegen dispatcher through the mixin. VariantType casts remain Unsupported and reach the dispatcher too. The mixin surfaces whatever error Spark itself would emit for Variant. isVariantType is added to CometTypeShim for the guard. Tests: - cast_complex_types_to_string_legacy.sql: drop expect_fallback since these casts now stay in the Comet pipeline. - New cast_string_to_date_time_parser_policy_legacy.sql: exercise string to date/timestamp/timestamp_ntz casts under LEGACY timeParserPolicy. - CometCastSuite: cast ArrayType(DateType) to unsupported ArrayType now checks the Comet operator rather than a Spark fallback reason.
…_to_string Map to string and struct/array containing Map to string casts are Unsupported in CometCast, but with the CodegenDispatchFallback mixin they now run through Spark's own doGenCode inside the Comet pipeline. The Comet operator stays in the plan, so expect_fallback assertions no longer hold.
…ypes_to_string
- Add a query directive above `SELECT cast(array(map('k', 1)) as string)` so the
parser actually runs the array-of-map case (previously an orphan SELECT).
- Rewrite the Map to string section header to describe codegen dispatch behavior.
…ery mode Under LEGACY timeParserPolicy the CodegenDispatchFallback mixin keeps the CAST inside the Comet pipeline via Spark's doGenCode, so the Comet operator should be present in the plan. Use plain `query` (not `spark_answer_only`) so the test verifies both the result and that Comet is executing the cast.
… codegen dispatch The `explain comet` test asserted that `cast(make_interval(...) as string)` falls back to Spark with reason `Cast from CalendarIntervalType to StringType is not supported`. With the CodegenDispatchFallback mixin on CometCast, that cast now runs through Spark's doGenCode inside the Comet pipeline, so the operator stays Comet-native and no fallback reason is emitted: - Drop the standalone `SELECT cast(make_interval(...) as string)` case whose only expectation was that reason. - Prune the same reason from the join case; the shuffle-disabled reason still applies to the aggregate side of the join.
9b35d81 to
25c3d1c
Compare
|
Thanks for iterating on this @comphead. I think this is close now. I left feedback. Could you also update the PR title and description as they are now outdated. |
…sertion VariantEndToEndSuite has a codegen-mode plan-type assertion that expects WholeStageCodegenExec at the top. Under Comet the plan is wrapped in CometNativeColumnarToRowExec, so the strict class check fails once CometCast (with CodegenDispatchFallback) keeps the Variant cast inside the Comet pipeline. Loosen the assertion to also accept CometNativeColumnarToRowExec in dev/diffs/4.0.2.diff and 4.1.2.diff. Regenerated by applying the delta to Spark v4.0.2 and v4.1.2 clones and verifying with `git apply --check` on both.
- Restore the removed comment above legacyCastComplexTypesToStringReason explaining the legacy formatting differences, updated to describe the codegen dispatcher routing instead of a Spark fallback. - Drop the "natively" qualifier from legacyCastComplexTypesToStringReason. Incompatibility reasons already document ways the native implementation differs from Spark, so the qualifier is redundant with the docstring on getIncompatibleReasons. - Restore the removed comment on the negative-scale decimal to string test in CometCastSuite, explaining why localTableScan is toggled on.
Fold the outer `isVariantType` guard in `CometCast.getSupportLevel` into the `Literal` short-circuit branch. The non-literal path already runs the same guard via `isSupported`, so keeping both fires it twice per plan. Reorder `isComplexType(fromType) && legacyCastComplexTypesToString` so the SQLConf lookup only runs when the source is complex. Trim duplicate justification comments across `CometCast.scala`, the two `cast_complex_types_to_string*.sql` headers, and the `VariantType` test in `CometCastSuite`.
comphead
changed the title
|
Once this merges, we can stop enabling incompatible casts in the stability suite |
andygrove
changed the title
|
Thanks @andygrove for the cycles spent on review :) |
2 participants
Split from #4799
Summary
CodegenDispatchFallbackintoCometCastso cast expressions that Comet declines to run natively (e.g.spark.sql.legacy.castComplexTypesToString.enabled=true) can execute via the JVM codegendispatcher instead of falling all the way back to Spark.
VariantType(Spark 4) inCometTypeShimand reject Variant casts inCometCast.convert/isSupported, since neither the native serde nor the codegen dispatcher can carryVariantTypeindata args or return type.
DataTypeSupport.isComplexTypeand trim thelegacyCastComplexTypesToStringcomment to reflect the new dispatch path.expect_fallbackmarkers from non-legacycast_complex_types_to_string, keep them on the legacy variant, and runcast_string_to_date_time_parser_policy_legacyinplain query mode.
CometCastSuite/CometExpressionSuiteexpectations and the4.0.2/4.1.2spark diffs soVariantEndToEndSuiteaccepts a Comet plan under codegen dispatch.