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.

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

Add a flag for allowing single quotes in JSON strings. #8144

Merged

Conversation

nvdbaranec
Copy link
Contributor

@nvdbaranec nvdbaranec commented May 3, 2021

Add a flag that allows get_json_object() to accept JSON with strings using single quotes. Also adds an explicit get_json_object_options struct for allowing the user to customize what behaviors they want.

Note: stripping quotes from individually returned string values has been left on as default.

…n_object() to take an explicit

options struct for controlling behaviors.
@nvdbaranec nvdbaranec added libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS improvement Improvement / enhancement to an existing function breaking Breaking change labels May 3, 2021
@nvdbaranec nvdbaranec requested a review from a team as a code owner May 3, 2021 16:52
@nvdbaranec nvdbaranec requested review from trxcllnt and ttnghia May 3, 2021 16:52
@nvdbaranec
Copy link
Contributor Author

Pinging @jlowe for any input on the flipping of the logic (previously, default behavior was Spark-specific, now it is the opposite and the plugin will need to set set_allow_single_quotes(true) and set_strip_quotes_from_single_strings(true) .

@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.06@8406522). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #8144   +/-   ##
===============================================
  Coverage                ?   82.84%           
===============================================
  Files                   ?      105           
  Lines                   ?    17865           
  Branches                ?        0           
===============================================
  Hits                    ?    14800           
  Misses                  ?     3065           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8406522...a2b6005. Read the comment docs.

@trxcllnt
Copy link
Contributor

trxcllnt commented May 7, 2021

Stripping the quotes from a string seems like the right thing to do by default for JSON:

const str = JSON.parse(`{"str":"foo"}`).str;

assert(str === `foo`) // str is `foo`, not `"foo"`

cpp/include/cudf/strings/json.hpp Show resolved Hide resolved
cpp/include/cudf/strings/json.hpp Show resolved Hide resolved
@nvdbaranec
Copy link
Contributor Author

As per Paul's comment, #8144 (comment) I have switched the default behavior for stripping quotes on single-string return values back to the way they were. Note: allowing single-quotes is still off by default, so the Spark plugin will need to toggle this on.

cpp/tests/strings/json_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/strings/json_tests.cpp Show resolved Hide resolved
@nvdbaranec nvdbaranec requested a review from davidwendt May 17, 2021 16:36
@nvdbaranec nvdbaranec requested a review from davidwendt May 17, 2021 21:00
Copy link
Contributor

@hyperbolic2346 hyperbolic2346 left a comment

Choose a reason for hiding this comment

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

Looks good to me

cpp/include/cudf/strings/json.hpp Show resolved Hide resolved
cpp/include/cudf/strings/detail/json.hpp Show resolved Hide resolved
cpp/src/strings/json/json_path.cu Show resolved Hide resolved
cpp/include/cudf/strings/json.hpp Show resolved Hide resolved
@nvdbaranec nvdbaranec added non-breaking Non-breaking change and removed breaking Breaking change labels May 19, 2021
@nvdbaranec
Copy link
Contributor Author

Changing this to non-breaking as it won't alter existing behavior.

@nvdbaranec
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 32c1bac into rapidsai:branch-21.06 May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants