JSON quote normalization #14545

shrshi · 2023-12-02T00:03:00Z

Description

The goal of this PR is to address PR 10004 by supporting parsing of JSON files containing single quotes for field/value strings.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

copy-pr-bot · 2023-12-02T00:03:03Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

shrshi · 2023-12-02T00:03:47Z

/ok to test

elstehle

That's already looking great. I think one important point is that we make use of a function object for the translation instead of using a translation table. This allows us to simply return the input symbol for all input symbols that we do not need to modify.
I've slightly modified your FST to accommodate that change, which also simplifies the symbol groups amongst which we need to distinguish.

Also, I've run into a corner case with the current symbol-to-symbol-group-id lookup table implementation for this lookup table we need here. So I temporarily replaced it that lookup with the SymbolToSymbolGroup function object to unblock you here. Sorry for the inconvenience. I'll work on a fix in the meanwhile.

cpp/src/io/fst/lookup_tables.cuh

cpp/tests/io/fst/quote_normalization_test.cu

shrshi · 2023-12-07T20:22:24Z

Thank you for the detailed feedback, Elias. I have included all your suggested changes in the most recent commit. The solution for the other corner case that we discussed - the escaped single quote within a single-quoted input string - has been handled as well. There are two examples that need further thought - (i) retaining an escaped single-quote within a double-quoted input string, and (ii) ill-formed JSON with extra single quote before closing brace.

shrshi · 2023-12-11T23:42:18Z

/ok to test

elstehle

Looks good so far! Left a few minor comments.

I assume the next step is to integrate this preprocessing stage into the reader depending on a new reader option?

cpp/tests/io/fst/quote_normalization_test.cu

hyperbolic2346

This is looking good to me. Some comments and questions.

cpp/tests/io/fst/quote_normalization_test.cu

shrshi · 2023-12-12T22:35:42Z

/ok to test

vuule · 2023-12-12T22:57:41Z

cpp/tests/io/fst/quote_normalization_test.cu

+// Base test fixture for tests
+struct FstTest : public cudf::test::BaseFixture {};
+
+void run_test(std::string& input, std::string& output)


What is the plan for this PR in terms of code location? Here, in tests?

We can place this code in io/fst and have the user read their JSON with a pre-process reader option, or have a separate API that reads this file from device buffer and runs this FST as a pre-processing step.

Sorry I don't understand. Everything here is in a test file, so there is no new feature or API was added. What does this PR actually offer?

The plan is to integrate this pre-processing FST into libcudf in a follow-up PR to enable parsing the single-quote variant of JSON. This PR constructs the normalizing FST and checks if it can correctly handle valid and invalid JSON cases.

If this is just a proof-of-concept that is implemented directly into the unit tests then I don't recommend merging it, as the new unit tests do not actually test any existing API. Only when we have a new API implemented (in the follow up PR) then we merge its corresponding unit tests.

We discussed this and decided it would be beneficial to merge this POC before holidays and then build up the API next year.
IMO the unit tests are pretty useful even with POC because of the run_test abstraction, which is the sole entry point. We can easily update the test util code to call the new API from run_test, without even changing each test.

shrshi · 2023-12-14T20:56:04Z

/ok to test

…cial characters

shrshi · 2023-12-14T23:43:17Z

/ok to test

shrshi · 2023-12-15T00:06:45Z

/ok to test

cpp/tests/io/fst/quote_normalization_test.cu

shrshi · 2023-12-15T20:27:54Z

/ok to test

elstehle · 2023-12-19T21:13:06Z

cpp/tests/io/fst/quote_normalization_test.cu

+  std::cout << "Expected output: " << output << std::endl << "Computed output: ";
+  for (size_t i = 0; i < output_gpu_size[0]; i++)
+    std::cout << output_gpu[i];
+  std::cout << std::endl;


suggestion: I think we want to remove printing to stdout to not pollute test logs?

shrshi · 2024-01-02T19:30:50Z

/ok to test

shrshi · 2024-01-02T19:57:10Z

/ok to test

shrshi · 2024-01-02T22:41:32Z

/ok to test

shrshi · 2024-01-03T01:37:07Z

/merge

The goal of this PR is to address [10004](#10004) by supporting parsing of JSON files containing single quotes for field/value strings. This is a follow-up work to the POC [PR 14545](#14545) Authors: - Shruti Shivakumar (https://github.com/shrshi) Approvers: - Andy Grove (https://github.com/andygrove) - Vyas Ramasubramani (https://github.com/vyasr) - Vukasin Milovanovic (https://github.com/vuule) - Elias Stehle (https://github.com/elstehle) - Robert (Bobby) Evans (https://github.com/revans2) URL: #14729

The goal of this PR is to address [10004](rapidsai#10004) by supporting parsing of JSON files containing single quotes for field/value strings. This is a follow-up work to the POC [PR 14545](rapidsai#14545) Authors: - Shruti Shivakumar (https://github.com/shrshi) Approvers: - Andy Grove (https://github.com/andygrove) - Vyas Ramasubramani (https://github.com/vyasr) - Vukasin Milovanovic (https://github.com/vuule) - Elias Stehle (https://github.com/elstehle) - Robert (Bobby) Evans (https://github.com/revans2) URL: rapidsai#14729

github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Dec 2, 2023

shrshi added the feature request New feature or request label Dec 2, 2023

shrshi added the non-breaking Non-breaking change label Dec 2, 2023

shrshi added 2 commits December 2, 2023 00:38

simple quote normalization test

3c7ac1e

added more test cases

f77bd26

shrshi force-pushed the fst branch from fe9b582 to f77bd26 Compare December 2, 2023 00:52

elstehle reviewed Dec 4, 2023

View reviewed changes

shrshi added 2 commits December 7, 2023 20:05

edge case handling; translation table -> translation functor

48287ae

forgot to run pre-commit style check

0aa5f30

shrshi added 4 commits December 7, 2023 13:52

Merge branch 'branch-24.02' into fst

5f9eea8

fixing test cases after discussion

cf50ee7

Merge branch 'fst' of github.com:shrshi/cudf into fst

d9ea541

Merge branch 'branch-24.02' into fst

f524883

shrshi marked this pull request as ready for review December 11, 2023 23:46

shrshi requested a review from a team as a code owner December 11, 2023 23:46

shrshi requested review from vyasr and hyperbolic2346 December 11, 2023 23:46

elstehle reviewed Dec 12, 2023

View reviewed changes

GregoryKimball requested review from davidwendt and vuule December 12, 2023 19:18

hyperbolic2346 requested changes Dec 12, 2023

View reviewed changes

cpp/tests/io/fst/quote_normalization_test.cu Outdated Show resolved Hide resolved

cpp/tests/io/fst/quote_normalization_test.cu Outdated Show resolved Hide resolved

cpp/tests/io/fst/quote_normalization_test.cu Show resolved Hide resolved

shrshi added 2 commits December 12, 2023 22:33

addressing PR feedback - back to lut approach rather than functor

5f7798b

Merge branch 'fst' of github.com:shrshi/cudf into fst

e9b8cc7

vuule reviewed Dec 12, 2023

View reviewed changes

using the right stream!

470543d

clarifying the translation table - splitting into special and non-spe…

77c4613

…cial characters

shrshi requested a review from hyperbolic2346 December 15, 2023 00:05

Merge branch 'branch-24.02' into fst

18ab470

ttnghia reviewed Dec 15, 2023

View reviewed changes

cpp/tests/io/fst/quote_normalization_test.cu Show resolved Hide resolved

ttnghia reviewed Dec 15, 2023

View reviewed changes

cpp/tests/io/fst/quote_normalization_test.cu Show resolved Hide resolved

shrshi added 2 commits December 15, 2023 20:24

addressing PR reviews

124ab53

Merge branch 'fst' of github.com:shrshi/cudf into fst

0eede6f

ttnghia approved these changes Dec 15, 2023

View reviewed changes

hyperbolic2346 approved these changes Dec 19, 2023

View reviewed changes

elstehle approved these changes Dec 19, 2023

View reviewed changes

vuule added the 0 - Waiting on Author Waiting for author to respond to review label Dec 19, 2023

ttnghia and others added 3 commits December 21, 2023 10:49

Merge branch 'branch-24.02' into fst

e01fa85

Merge branch 'branch-24.02' into fst

ed41701

removed stdout logging

83da203

pre-commit run

367473b

Merge branch 'branch-24.02' into fst

91adbbd

rapids-bot bot merged commit af65d52 into rapidsai:branch-24.02 Jan 3, 2024
67 checks passed

shrshi mentioned this pull request Jan 9, 2024

JSON single quote normalization API #14729

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON quote normalization #14545

JSON quote normalization #14545

shrshi commented Dec 2, 2023 •

edited

Loading

copy-pr-bot bot commented Dec 2, 2023

shrshi commented Dec 2, 2023

elstehle left a comment

shrshi commented Dec 7, 2023

shrshi commented Dec 11, 2023

elstehle left a comment

hyperbolic2346 left a comment

shrshi commented Dec 12, 2023

vuule Dec 12, 2023

shrshi Dec 12, 2023

ttnghia Dec 14, 2023

shrshi Dec 14, 2023

ttnghia Dec 14, 2023

vuule Dec 14, 2023

shrshi commented Dec 14, 2023

shrshi commented Dec 14, 2023

shrshi commented Dec 15, 2023

shrshi commented Dec 15, 2023

elstehle Dec 19, 2023

shrshi commented Jan 2, 2024

shrshi commented Jan 2, 2024

shrshi commented Jan 2, 2024

shrshi commented Jan 3, 2024

JSON quote normalization #14545

JSON quote normalization #14545

Conversation

shrshi commented Dec 2, 2023 • edited Loading

Description

Checklist

copy-pr-bot bot commented Dec 2, 2023

shrshi commented Dec 2, 2023

elstehle left a comment

Choose a reason for hiding this comment

shrshi commented Dec 7, 2023

shrshi commented Dec 11, 2023

elstehle left a comment

Choose a reason for hiding this comment

hyperbolic2346 left a comment

Choose a reason for hiding this comment

shrshi commented Dec 12, 2023

vuule Dec 12, 2023

Choose a reason for hiding this comment

shrshi Dec 12, 2023

Choose a reason for hiding this comment

ttnghia Dec 14, 2023

Choose a reason for hiding this comment

shrshi Dec 14, 2023

Choose a reason for hiding this comment

ttnghia Dec 14, 2023

Choose a reason for hiding this comment

vuule Dec 14, 2023

Choose a reason for hiding this comment

shrshi commented Dec 14, 2023

shrshi commented Dec 14, 2023

shrshi commented Dec 15, 2023

shrshi commented Dec 15, 2023

elstehle Dec 19, 2023

Choose a reason for hiding this comment

shrshi commented Jan 2, 2024

shrshi commented Jan 2, 2024

shrshi commented Jan 2, 2024

shrshi commented Jan 3, 2024

shrshi commented Dec 2, 2023 •

edited

Loading