fix: route decode through codegen dispatcher to honor Spark 4.0 legacy flags#4639
Merged
andygrove merged 3 commits intoJun 12, 2026
Merged
Conversation
…yCharsets / legacyErrorAction Comet's existing lowering of `decode(bin, charset)` to a TRY-mode binary→string cast produced wrong results on invalid byte sequences across all supported Spark versions. The Spark 4.0 shim destructured the StaticInvoke arguments and discarded both legacy-mode flags before falling back into the same helper, so neither the substitute nor the strict-error behavior was reachable. Replace the native lowering with the codegen dispatcher: Spark's own doGenCode runs inside the Comet pipeline and honors both flags exactly, matching Spark for valid input, replacement-character substitution, and the Spark 4.0 default strict-mode error. Adds invalid-UTF-8 SQL fixtures for the legacy and strict modes. Closes apache#4465
- Register decode in the existing staticInvokeExpressions map (statics.scala) instead of carving a special case into Spark4xCometExprShim, deleting the bespoke 17-line StaticInvoke arm and its imports. The map already routes aesEncrypt/aesDecrypt through CometStaticInvokeCodegenDispatch; decode joins the same pattern. - Trim the CometStringDecode scaladoc from a 7-line history of the bug to a 2-line summary plus a apache#4465 link. - Drop the stale reference to a non-existent decode_invalid_utf8_legacy.sql in the decode.sql header. - Fix incorrect UTF-8 byte semantics in decode_invalid_utf8.sql comments (0xFF is not a continuation byte; 0xE9 is a valid 3-byte lead byte that is just missing its continuation bytes) and drop the redundant case-only-different `'UTF-8'` query.
mbutrovich
approved these changes
Jun 12, 2026
mbutrovich
left a comment
mbutrovich
left a comment
Contributor
There was a problem hiding this comment.
Nice! Thanks @andygrove!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #4465.
Rationale for this change
Comet currently lowers
decode(bin, charset)to a TRY-mode binary→string cast, which produces the wrong result for invalid byte sequences in every supported Spark version:legacyErrorAction = truelegacyErrorAction = false)MALFORMED_CHARACTER_CODINGThe Spark 4.0 shim destructured the
StaticInvokearguments and discarded bothlegacyCharsets/legacyErrorActionflags, so neither the substitute nor the strict-error path was reachable.What changes are included in this PR?
Spark4xCometExprShim) now routes theStringDecode.decodeStaticInvokethroughCometScalaUDF.emitJvmCodegenDispatchinstead of the buggy TRY-cast helper. The dispatcher runs Spark's owndoGenCodeinside the Comet pipeline, so the capturedlegacyCharsets/legacyErrorActionliterals are honored exactly.CometStringDecodecollapses toextends CometCodegenDispatch[StringDecode], so the 3.x replacement-character behavior also matches Spark exactly. The per-expressionspark.comet.expression.StringDecode.enabledflag still works because the framework checks it before callingconvert.CommonStringExprs.stringDecodehelper and theCommonStringExprsmixin from the 3.x / 4.x shims.decode.sqlto drop obsoleteexpect_fallbacklines (the dispatcher handles non-UTF-8 charsets too) and adds two new fixtures:decode_invalid_utf8.sql— replacement-character behavior on Spark 3.x default and Spark 4.0+ with both legacy flags enabled.decode_invalid_utf8_strict.sql— Spark 4.0+ default strict mode withexpect_error(MALFORMED_CHARACTER_CODING), including a sentinel valid-input query so the assertion does not pass vacuously through an operator-level fallback.How are these changes tested?
mvn compileclean on Spark 3.4 / 3.5 / 4.0 / 4.1 (spotless + scalastyle clean).CometSqlFileTestSuite decodepasses on Spark 3.4, 3.5, and 4.0; the strict-mode fixture is skipped on 3.x viaMinSparkVersion: 4.0and runs on 4.0.CometStringDecodeSuite(Spark 3.x only) still passes, confirming the per-expression enable flag continues to work after the refactor.