-
Notifications
You must be signed in to change notification settings - Fork 28.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-48490][CORE][FOLLOWUP] Properly process escape sequences #47050
Conversation
cc @panbingkun |
+1, LGTM. |
Wait, there may still be some minor issues. Let me verify them in UT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sb.append(processedParts.next())
->
sb.append(StringContext.processEscapes(processedParts.next()))
@HyukjinKwon Can we merge this? |
Thanks @yaooqinn |
…s scene ### What changes were proposed in this pull request? The pr is followup #47050 ### Why are the changes needed? Add some UT for the Windows paths scene. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47051 from panbingkun/SPARK-48490_FOLLOWUP_TESTS. Authored-by: panbingkun <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>
### What changes were proposed in this pull request? Even with the fix in apache#46824, the escape sequences (`\r`, `\n`, `\t`, etc) are not handled properly. For example, when we use `log"\n"`, the StringContext interprets `\n` as a literal backslash `\` followed by `n` instead of a newline character. As a result, the bytes of `log"\n".message` becomes `[92, 110]`, instead of `[10]`. This PR is to fix the issue by using the method StringContext.processEscapes in `LogStringContext`. ### Why are the changes needed? To ensure that escape sequences are properly processed in Spark logs ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New UT ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#47050 from gengliangwang/fixEscape. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Kent Yao <[email protected]>
…s scene ### What changes were proposed in this pull request? The pr is followup apache#47050 ### Why are the changes needed? Add some UT for the Windows paths scene. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47051 from panbingkun/SPARK-48490_FOLLOWUP_TESTS. Authored-by: panbingkun <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>
What changes were proposed in this pull request?
Even with the fix in #46824, the escape sequences (
\r
,\n
,\t
, etc) are not handled properly. For example, when we uselog"\n"
, the StringContext interprets\n
as a literal backslash\
followed byn
instead of a newline character. As a result, the bytes oflog"\n".message
becomes[92, 110]
, instead of[10]
.This PR is to fix the issue by using the method StringContext.processEscapes in
LogStringContext
.Why are the changes needed?
To ensure that escape sequences are properly processed in Spark logs
Does this PR introduce any user-facing change?
No
How was this patch tested?
New UT
Was this patch authored or co-authored using generative AI tooling?
No