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

Jump to bottom

Adds the end-to-end JSON parser implementation #11388

Merged
merged 97 commits into from
Aug 12, 2022
Merged

Adds the end-to-end JSON parser implementation #11388

merged 97 commits into from
Aug 12, 2022

Conversation

elstehle
Copy link
Contributor

Description

This PR builds on the JSON tokenizer algorithm to implement an end-to-end JSON parser that parses to a table_with_metadata.

Chained PR depending on:
⛓️ #11264

Checklist

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

Thanks to @karthikeyann who provided the initial implementation for converting internal columnar data to cudf columns.

elstehle added 30 commits July 13, 2022 00:53
@elstehle
squashed with bracket/brace test
0557d41
@elstehle
clean up & addressing review comments
355d1e4
@elstehle
refactored lookup tables
39a6b65
@elstehle
put lookup tables into their own cudf file
239f138
@elstehle
Change interface for FST to not need temp storage
39cff80
@elstehle
removing unused var post-cleanup
e24a133
@elstehle
unified usage of pragma unrolls
caf6195
@elstehle
Adding hostdevice macros to in-reg array
ea79a81
@elstehle
making const vars const
17dcbfd
@elstehle
refactor lut sanity check
6fdd24a
@elstehle
fixes sg-count & uses rmm stream in fst tests
eccf970
@elstehle
minor doxygen fix
9fe8e4b
@elstehle
adopts suggested fst test changes
694a365
@elstehle
adopts device-side test data gen
f656f49
@elstehle
adopts c++17 namespaces declarations
485a1c6
@elstehle
removes state vector-wrapper in favor of vanilla array
5f1c4b5
@elstehle
some west-const remainders & unifies StateIndexT
e6f8def
@elstehle
adds check for state transition narrowing conversion
a798852
@elstehle
fixes logical stack test includes
eb24962
@elstehle
replaces enum with typed constexpr
f52e614
@elstehle
adds excplitis error checking
3038058
@elstehle
addresses style review comments & fixes a todo
d351e5c
@elstehle
replaces gtest asserts with expects
3f47952
@elstehle
fixes style in dispatch dfa
cba1619
@elstehle
replaces vanilla loop with iota
bea2a02
@elstehle
rephrases documentation on in-reg array
8a184e9
@elstehle
Merge remote-tracking branch 'upstream/branch-22.08' into feature/fin…
78dd893 
…ite-state-transducer-trimmed
@elstehle
Merge remote-tracking branch 'upstream/branch-22.08' into feature/fin…
7a19f64 
…ite-state-transducer-trimmed
@elstehle
improves style in fst test
4783aae
@elstehle
adds comments in in_reg array
6203709
@github-actions github-actions bot removed the CMake CMake build issue label Aug 6, 2022
@elstehle elstehle marked this pull request as ready for review August 8, 2022 08:08
@upsj upsj self-requested a review August 9, 2022 08:18
@vuule vuule requested a review from nvdbaranec August 10, 2022 01:01
vuule
vuule requested changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

High level review.
Good stuff, mostly docs/naming need to be ironed out.

cpp/tests/io/nested_json_test.cu Outdated Show resolved Hide resolved
cpp/tests/io/nested_json_test.cu Outdated Show resolved Hide resolved
cpp/src/io/json/nested_json.hpp Outdated Show resolved Hide resolved
cpp/src/io/json/nested_json.hpp Outdated Show resolved Hide resolved
cpp/src/io/json/nested_json.hpp Outdated Show resolved Hide resolved
cpp/src/io/json/nested_json.hpp Outdated Show resolved Hide resolved
cpp/src/io/json/nested_json.hpp Show resolved Hide resolved
cpp/src/io/json/nested_json.hpp Outdated Show resolved Hide resolved
cpp/src/io/json/nested_json_gpu.cu
Comment on lines +1247 to +1249
//--------------------------------------------------------------------------------
// INITIALIZE JSON ROOT NODE
//--------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider creating a new function instead of this kind of sections.

Suggested change
//--------------------------------------------------------------------------------
// INITIALIZE JSON ROOT NODE
//--------------------------------------------------------------------------------

cpp/src/io/json/nested_json_gpu.cu
auto const& token = tokens_gpu[offset];

#ifdef NJP_DEBUG_PRINT
std::cout << "[" << token_to_string(token) << "]\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

dev: we need to add logging
(fictional) manager: we have logging at home
logging at home:

@github-actions github-actions bot added the CMake CMake build issue label Aug 11, 2022
bdice
bdice approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving CMake changes - and requesting a couple small C++ include changes.

cpp/tests/io/nested_json_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/nested_json_test.cpp Outdated Show resolved Hide resolved
PointKernel
PointKernel approved these changes Aug 12, 2022
View reviewed changes
cpp/src/io/json/nested_json_gpu.cu Outdated Show resolved Hide resolved
@vuule
Copy link
Contributor

vuule commented Aug 12, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6035cc2 into rapidsai:branch-22.10 Aug 12, 2022
rapids-bot bot pushed a commit that referenced this pull request Sep 1, 2022
@karthikeyann
Adds Nested Json benchmark (#11466)
e5c8776 
Adds nvbench for nested json parser.
Depends on #11388

Authors:
  - Karthikeyan (https://github.com/karthikeyann)
  - Elias Stehle (https://github.com/elstehle)

Approvers:
  - David Wendt (https://github.com/davidwendt)
  - Nghia Truong (https://github.com/ttnghia)

URL: #11466
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@bdice bdice bdice approved these changes

@vuule vuule vuule approved these changes

@PointKernel PointKernel PointKernel approved these changes

@hyperbolic2346 hyperbolic2346 Awaiting requested review from hyperbolic2346 hyperbolic2346 is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@karthikeyann karthikeyann Awaiting requested review from karthikeyann

@upsj upsj Awaiting requested review from upsj

Assignees
No one assigned
Labels
CMake CMake build issue cuIO cuIO issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
Nested JSON reader
Development

Successfully merging this pull request may close these issues.
8 participants
@elstehle @vuule @upsj @bdice @karthikeyann @PointKernel @nvdbaranec @GregoryKimball