-
Notifications
You must be signed in to change notification settings - Fork 926
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
Rename experimental JSON tests. #15568
Rename experimental JSON tests. #15568
Conversation
@karthikeyann / @vuule: Could one of you help me understand this line? cudf/cpp/tests/io/nested_json_test.cpp Line 624 in 7b9e815
This boolean parameter labeled "Experimental" appears to control whether we call |
IMO we can remove the host path, and thus this test. We now have a lot of tests of the new reader, I think we don't have to test this part of the implementation against the reference any more. If not, yes, renaming to DeviceParse sounds good to me |
Great. I renamed to |
/merge |
Yes. We could remove the host parsing logic. It may not be in sync with device code with a lot of recent changes in JSON tokenizer, and tree algorithms. @elstehle shall we remove host parsing code and their tests? |
Did you merge this with only one cpp review approval? |
Oops. @davidwendt I thought you were the second approver. My apologies. |
@vuule and/or @karthikeyann feel free to take a look over this. I thought Vukasin had approved first and David was second, but I now see that I imagined the first ✅ where there was none. I hope we don’t need to revert but I’ll be happy to apply any follow-ups in another PR if needed. |
How could you do this @bdice ?! |
This PR addresses a task from #15537 to remove the `host_parse_nested_json` code path and corresponding tests. See discussion in #15568 (comment). Authors: - Bradley Dice (https://github.com/bdice) Approvers: - David Wendt (https://github.com/davidwendt) - Mark Harris (https://github.com/harrism) URL: #15674
Description
This PR renames the "experimental" JSON reader tests. These are now production grade and not experimental.
This task is tracked in #15537.
Checklist