Restructure JSON code to correctly reflect legacy/experimental status #13757
Conversation
vuule
added
tech debt
improvement
github-actions
bot
added
libcudf
vuule
changed the title
| @@ -547,7 +543,7 @@ void convert_json_to_columns(parse_options_view const& opts, | |||
| } | |||
|
|
|||
| /** | |||
| * @copydoc cudf::io::gpu::detect_data_types | |||
| * @copydoc cudf::io::json::detail::legacy::detect_data_types | |||
There was a problem hiding this comment.
the namespace was incorrect here AFAICT
…vuule/cudf into fea-read_json-namespace-cleanup
| @@ -33,7 +33,7 @@ namespace cudf::io::json::detail { | |||
| * | |||
| * @return cudf::table object that contains the array of cudf::column. | |||
| */ | |||
| table_with_metadata read_json(std::vector<std::unique_ptr<cudf::io::datasource>>& sources, | |||
| table_with_metadata read_json(host_span<std::unique_ptr<datasource>> sources, | |||
There was a problem hiding this comment.
Out of scope for this PR, but a host_span<unique_ptr> feels off when it comes to ownership semantics. This pattern seems prevalent in cuIO though. Also I don't know if there's a nice idiomatic way to write this otherwise though since most implicit conversions won't quite work and there's limited interop between implicit conversions and template deduction.
There was a problem hiding this comment.
Yeah, really sends a mixed message :D
Surely we can lose the unique_ptrs somewhere along the call chain...
There was a problem hiding this comment.
Why do we need this new header?
There was a problem hiding this comment.
Because we now call legacy::read_json from read_json.cu. It does make me wonder if we still need the other read_json.hpp.
There was a problem hiding this comment.
Just checked, the other header is needed for tests; keeping the files as they are.
|
/merge |
Description
Closes #11982
Moved files to correct directories.
Updated namespaces: experimental -> base, base -> legacy
Improved namespace nesting to reduce the need for fully qualified names (
json::detailinstead ofdetail::json).Use
host_spaninstead ofstd::vector &in allread_jsonvariants.Checklist