-
Notifications
You must be signed in to change notification settings - Fork 915
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
Restructure JSON code to correctly reflect legacy/experimental status #13757
Changes from 2 commits
82a1913
313452c
bf0950b
8c71dcc
018dd11
40c2954
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,11 +45,7 @@ | |
|
||
using cudf::device_span; | ||
|
||
namespace cudf { | ||
namespace io { | ||
namespace json { | ||
namespace gpu { | ||
using namespace ::cudf; | ||
namespace cudf::io::json::detail::legacy { | ||
|
||
namespace { | ||
/** | ||
|
@@ -515,7 +511,7 @@ __global__ void collect_keys_info_kernel(parse_options_view const options, | |
} // namespace | ||
|
||
/** | ||
* @copydoc cudf::io::json::gpu::convert_json_to_columns | ||
* @copydoc cudf::io::json::detail::legacy::convert_json_to_columns | ||
*/ | ||
void convert_json_to_columns(parse_options_view const& opts, | ||
device_span<char const> const data, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the namespace was incorrect here AFAICT |
||
*/ | ||
|
||
std::vector<cudf::io::column_type_histogram> detect_data_types( | ||
|
@@ -592,7 +588,7 @@ std::vector<cudf::io::column_type_histogram> detect_data_types( | |
} | ||
|
||
/** | ||
* @copydoc cudf::io::json::gpu::gpu_collect_keys_info | ||
* @copydoc cudf::io::json::detail::legacy::collect_keys_info | ||
*/ | ||
void collect_keys_info(parse_options_view const& options, | ||
device_span<char const> const data, | ||
|
@@ -615,7 +611,4 @@ void collect_keys_info(parse_options_view const& options, | |
CUDF_CHECK_CUDA(stream.value()); | ||
} | ||
|
||
} // namespace gpu | ||
} // namespace json | ||
} // namespace io | ||
} // namespace cudf | ||
} // namespace cudf::io::json::detail::legacy |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this new header? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we now call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just checked, the other header is needed for tests; keeping the files as they are. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* Copyright (c) 2023, NVIDIA CORPORATION. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
#include <cudf/types.hpp> | ||
|
||
#include <rmm/cuda_stream_view.hpp> | ||
|
||
#include <thrust/mr/memory_resource.h> | ||
|
||
#include <memory> | ||
#include <vector> | ||
|
||
namespace cudf::io::json::detail::legacy { | ||
|
||
table_with_metadata read_json(host_span<std::unique_ptr<datasource>> sources, | ||
json_reader_options const& reader_opts, | ||
rmm::cuda_stream_view stream, | ||
rmm::mr::device_memory_resource* mr); | ||
|
||
} // namespace cudf::io::json::detail::legacy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, really sends a mixed message :D
Surely we can lose the
unique_ptr
s somewhere along the call chain...