[SPARK-47307][SQL][FOLLOWUP] Promote spark.sql.legacy.chunkBase64String.enabled from a legacy/internal config to a regular/public one #47410
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
…legacy.chunkBase64String.enabled` to true
yaooqinn
reviewed
Jul 19, 2024
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
yaooqinn
approved these changes
Jul 19, 2024
|
cc @gatorsmile too |
wForget
commented
Jul 19, 2024
|
|
||
| // check if unbase64 works well for chunked and non-chunked encoded strings | ||
| checkEvaluation(StringDecode(UnBase64(Literal(encoded)), Literal("utf-8")), longString) | ||
| checkEvaluation(StringDecode(UnBase64(Literal(chunkEncoded)), Literal("utf-8")), longString) |
There was a problem hiding this comment.
I added this case to confirm that the chunkEncoded string can be decoded correctly. cc @cloud-fan
|
BTW, @wForget , if you change the configuration name in this PR, the PR title and description should be updated accordingly. Currently, the PR title and description is not matched. |
spark.sql.legacy.chunkBase64String.enabled to true
Updated, thank you. |
dongjoon-hyun
approved these changes
Jul 19, 2024
|
Let's wait for @gatorsmile and @cloud-fan 's review. |
|
The test golden files need to be regenerated, I am doing it. |
|
Oh, this one? |
yeah |
|
Merged to master. Hi @wForget, please help backport this to 3.5. Thank you |
wForget
added a commit
to wForget/spark
that referenced
this pull request
Jul 19, 2024
…ng.enabled from a legacy/internal config to a regular/public one ### What changes were proposed in this pull request? + Promote spark.sql.legacy.chunkBase64String.enabled from a legacy/internal config to a regular/public one. + Add test cases for unbase64 ### Why are the changes needed? Keep the same behavior as before. More details: apache#47303 (comment) ### Does this PR introduce _any_ user-facing change? yes, revert behavior change introduced in apache#47303 ### How was this patch tested? existing unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47410 from wForget/SPARK-47307_followup. Lead-authored-by: wforget <643348094@qq.com> Co-authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit af5eb08)
|
It's good to keep the old behavior by default, but I'd like to understand the difference. Which one is more commonly supported in the ecosystem (decoding)? The chunked one or the unchunked one? |
|
Unchunking is the behavior before 3.2(inclusive). Chunking was the behavior introduced accidentally in 3.3. I suggested @wForget in #47303 to make chunking as legacy behavior and unchunking as default, considering that unchunking is more reasonable here and users may not upgrade so eagerly, while chunking is mostly for editors who can not handle long lines. For the unbase64 part, both are valid for decoding. |
|
Hive, Trino, and Clickhouse all seem to be unchunked. |
yaooqinn
pushed a commit
that referenced
this pull request
Jul 20, 2024
…4String.enabled from a legacy/internal config to a regular/public one Backports #47410 to 3.5 ### What changes were proposed in this pull request? + Promote spark.sql.legacy.chunkBase64String.enabled from a legacy/internal config to a regular/public one. + Add test cases for unbase64 ### Why are the changes needed? Keep the same behavior as before. More details: #47303 (comment) ### Does this PR introduce _any_ user-facing change? yes, revert behavior change introduced in #47303 ### How was this patch tested? existing unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes #47416 from wForget/SPARK-47307_followup_3.5. Authored-by: wforget <643348094@qq.com> Signed-off-by: Kent Yao <yao@apache.org>
jingz-db
pushed a commit
to jingz-db/spark
that referenced
this pull request
Jul 22, 2024
…ng.enabled from a legacy/internal config to a regular/public one ### What changes were proposed in this pull request? + Promote spark.sql.legacy.chunkBase64String.enabled from a legacy/internal config to a regular/public one. + Add test cases for unbase64 ### Why are the changes needed? Keep the same behavior as before. More details: apache#47303 (comment) ### Does this PR introduce _any_ user-facing change? yes, revert behavior change introduced in apache#47303 ### How was this patch tested? existing unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47410 from wForget/SPARK-47307_followup. Lead-authored-by: wforget <643348094@qq.com> Co-authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org>
attilapiros
pushed a commit
to attilapiros/spark
that referenced
this pull request
Oct 4, 2024
…ng.enabled from a legacy/internal config to a regular/public one ### What changes were proposed in this pull request? + Promote spark.sql.legacy.chunkBase64String.enabled from a legacy/internal config to a regular/public one. + Add test cases for unbase64 ### Why are the changes needed? Keep the same behavior as before. More details: apache#47303 (comment) ### Does this PR introduce _any_ user-facing change? yes, revert behavior change introduced in apache#47303 ### How was this patch tested? existing unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47410 from wForget/SPARK-47307_followup. Lead-authored-by: wforget <643348094@qq.com> Co-authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org>
himadripal
pushed a commit
to himadripal/spark
that referenced
this pull request
Oct 19, 2024
…ng.enabled from a legacy/internal config to a regular/public one ### What changes were proposed in this pull request? + Promote spark.sql.legacy.chunkBase64String.enabled from a legacy/internal config to a regular/public one. + Add test cases for unbase64 ### Why are the changes needed? Keep the same behavior as before. More details: apache#47303 (comment) ### Does this PR introduce _any_ user-facing change? yes, revert behavior change introduced in apache#47303 ### How was this patch tested? existing unit test ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47410 from wForget/SPARK-47307_followup. Lead-authored-by: wforget <643348094@qq.com> Co-authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org>
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?
Why are the changes needed?
Keep the same behavior as before. More details: #47303 (comment)
Does this PR introduce any user-facing change?
yes, revert behavior change introduced in #47303
How was this patch tested?
existing unit test
Was this patch authored or co-authored using generative AI tooling?
No