[SPARK-46187][SQL] Align codegen and non-codegen implementation of StringDecode
#44094
Conversation
This file contains 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
MaxGekk
changed the title
StringDecodeStringDecode
|
@srielau @cloud-fan One more expression where need align codegen/non-codegen and restrict encodings. |
cloud-fan
approved these changes
Nov 30, 2023
cloud-fan
reviewed
Nov 30, 2023
| @@ -2648,18 +2648,26 @@ case class StringDecode(bin: Expression, charset: Expression) | |||
|
|
|||
| protected override def nullSafeEval(input1: Any, input2: Any): Any = { | |||
| val fromCharset = input2.asInstanceOf[UTF8String].toString | |||
| UTF8String.fromString(new String(input1.asInstanceOf[Array[Byte]], fromCharset)) | |||
| try { | |||
| UTF8String.fromString(new String(input1.asInstanceOf[Array[Byte]], fromCharset)) | |||
There was a problem hiding this comment.
since we are here, can we rewrite this expression using StaticInvoke? To avoid this problem completely.
There was a problem hiding this comment.
Shall we introduce the legacyCharsets and supportedCharsets too?
There was a problem hiding this comment.
Shall we introduce the legacyCharsets and supportedCharsets too?
Yep, let me merge this and introduce the restrictions + tests, update of the migration guide and so on.
|
Merging to master. Thank you, @cloud-fan and @beliefer for review. |
Late LGTM. |
asl3
pushed a commit
to asl3/spark
that referenced
this pull request
Dec 5, 2023
…tringDecode` ### What changes were proposed in this pull request? In the PR, I propose to change the implementation of interpretation mode of `StringDecode` and apparently of the `decode` function. And make it consistent to codegen. Both implementation raise the same error with of the error class `INVALID_PARAMETER_VALUE.CHARSET`. ### Why are the changes needed? To make codegen and non-codegen of the `StringDecode` expression consistent. So, users will observe the same behaviour in both modes. ### Does this PR introduce _any_ user-facing change? Yes, if user code depends on error from `decode()`. ### How was this patch tested? By running the following test suites: ``` $ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z string-functions.sql" $ build/sbt "core/testOnly *SparkThrowableSuite" $ build/sbt "test:testOnly *.StringFunctionsSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#44094 from MaxGekk/align-codegen-stringdecode. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changes were proposed in this pull request?
In the PR, I propose to change the implementation of interpretation mode of
StringDecodeand apparently of thedecodefunction. And make it consistent to codegen. Both implementation raise the same error with of the error classINVALID_PARAMETER_VALUE.CHARSET.Why are the changes needed?
To make codegen and non-codegen of the
StringDecodeexpression consistent. So, users will observe the same behaviour in both modes.Does this PR introduce any user-facing change?
Yes, if user code depends on error from
decode().How was this patch tested?
By running the following test suites:
Was this patch authored or co-authored using generative AI tooling?
No.