Skip to content
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.

Sign up for GitHub

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

Escape quotes and newlines when converting strings to json format in to_json #9612

Merged
merged 9 commits into from
Nov 14, 2023
Merged

Escape quotes and newlines when converting strings to json format in to_json #9612

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dc45aee
escape quotes in when converting strings to json format
andygrove Nov 2, 2023
e7d502c
move withResource earlier
andygrove Nov 2, 2023
1682d6a
signoff
andygrove Nov 2, 2023
e31fd6e
update compatibility guide
andygrove Nov 2, 2023
3aadf3b
Escape newlines
andygrove Nov 2, 2023
82ee057
address feedback
andygrove Nov 3, 2023
f0b0d86
upmerge
andygrove Nov 7, 2023
3a07681
Merge remote-tracking branch 'nvidia/branch-23.12' into to_json_escape
andygrove Nov 14, 2023
c55b5d3
add link to issue
andygrove Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion integration_tests/src/main/python/json_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ def test_read_case_col_name(spark_tmp_path, v1_enabled_list, col_name):
pytest.param(double_gen, marks=pytest.mark.xfail(reason='https://github.com/NVIDIA/spark-rapids/issues/9350')),
pytest.param(date_gen, marks=pytest.mark.xfail(reason='https://github.com/NVIDIA/spark-rapids/issues/9515')),
pytest.param(timestamp_gen, marks=pytest.mark.xfail(reason='https://github.com/NVIDIA/spark-rapids/issues/9515')),
StringGen('[A-Za-z0-9\'"\\\\]{0,10}', nullable=True) \
StringGen('[A-Za-z0-9\r\n\'"\\\\]{0,10}', nullable=True) \
.with_special_case('\u1f600') \
.with_special_case('"a"') \
.with_special_case('\\"a\\"') \
Expand Down
4 changes: 2 additions & 2 deletions sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuCast.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1170,8 +1170,8 @@ object GpuCast {

private def escapeJsonString(cv: ColumnVector): ColumnVector = {
withResource(cv) { _=>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original function did not close the input column, but this does. Are we sure this or the caller is doing the right thing here? Looks like there's an artificial incref occurring in the calling code that's confusing to follow. Seems like we do not want this method to close its input in practice, forcing callers to artifically incref to work around.

val chars = Seq("\\", "\"")
val escaped = chars.map("\\" + _)
val chars = Seq("\r", "\n", "\\", "\"")
val escaped = Seq("\\r", "\\n", "\\\\", "\\\"")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: less hard-coding

Suggested change
val escaped = Seq("\\r", "\\n", "\\\\", "\\\"")
val escaped = chars.map(org.apache.commons.text.StringEscapeUtils.escapeJava)

withResource(ColumnVector.fromStrings(chars: _*)) { search =>
withResource(ColumnVector.fromStrings(escaped: _*)) { replace =>
cv.stringReplace(search, replace)
Expand Down