-
Notifications
You must be signed in to change notification settings - Fork 89
from_json: null only schema-mismatched rows #4728
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
base: main
Are you sure you want to change the base?
Changes from 11 commits
267ce8b
273f2e9
1a79c86
d87d85e
c2f2e46
ddfb4ad
4729276
ac17dbd
ca0366b
bdc9e71
21ebd2b
fc162be
805559f
e97bb27
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 |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| /* | ||
| * Copyright (c) 2026, 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 "json_utils.hpp" | ||
|
|
||
| #include <cudf_test/column_wrapper.hpp> | ||
|
|
||
| #include <cudf/column/column.hpp> | ||
| #include <cudf/strings/strings_column_view.hpp> | ||
| #include <cudf/types.hpp> | ||
|
|
||
| #include <nvbench/nvbench.cuh> | ||
|
|
||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| namespace { | ||
|
|
||
| [[nodiscard]] std::unique_ptr<cudf::column> make_input(cudf::size_type num_rows, | ||
| cudf::size_type mismatch_percent) | ||
| { | ||
| std::string const valid = R"({"data":{"c2":[{"c3":19,"c4":"x"}],"c1":1},"id":10})"; | ||
| std::string const mismatched = R"({"data":{"c2":[19],"c1":2},"id":20})"; | ||
|
|
||
| std::vector<std::string> rows; | ||
| rows.reserve(num_rows); | ||
| for (cudf::size_type row = 0; row < num_rows; ++row) { | ||
| rows.push_back(mismatch_percent > 0 && row % 100 < mismatch_percent ? mismatched : valid); | ||
| } | ||
| return cudf::test::strings_column_wrapper(rows.begin(), rows.end()).release(); | ||
| } | ||
|
|
||
| std::vector<std::string> nested_schema_names() | ||
| { | ||
| return {"data", "c1", "c2", "element", "c3", "c4", "id"}; | ||
| } | ||
|
|
||
| std::vector<int> nested_schema_num_children() { return {2, 0, 1, 2, 0, 0, 0}; } | ||
|
|
||
| std::vector<int> nested_schema_types() | ||
| { | ||
| return {static_cast<int>(cudf::type_id::STRUCT), | ||
| static_cast<int>(cudf::type_id::INT32), | ||
| static_cast<int>(cudf::type_id::LIST), | ||
| static_cast<int>(cudf::type_id::STRUCT), | ||
| static_cast<int>(cudf::type_id::INT32), | ||
| static_cast<int>(cudf::type_id::STRING), | ||
| static_cast<int>(cudf::type_id::INT32)}; | ||
| } | ||
|
|
||
| std::vector<int> nested_schema_scales() { return {0, 0, 0, 0, 0, 0, 0}; } | ||
|
|
||
| std::vector<int> nested_schema_precisions() { return {-1, -1, -1, -1, -1, -1, -1}; } | ||
|
|
||
| } // namespace | ||
|
|
||
| void BM_from_json_to_structs(nvbench::state& state) | ||
| { | ||
| auto const num_rows = static_cast<cudf::size_type>(state.get_int64("num_rows")); | ||
| auto const mismatch_percent = static_cast<cudf::size_type>(state.get_int64("mismatch_percent")); | ||
| auto const input = make_input(num_rows, mismatch_percent); | ||
|
|
||
| auto const col_names = nested_schema_names(); | ||
| auto const num_children = nested_schema_num_children(); | ||
| auto const types = nested_schema_types(); | ||
| auto const scales = nested_schema_scales(); | ||
| auto const precisions = nested_schema_precisions(); | ||
|
|
||
| state.set_cuda_stream(nvbench::make_cuda_stream_view(cudf::get_default_stream().value())); | ||
| state.exec(nvbench::exec_tag::sync, [&](nvbench::launch&) { | ||
| [[maybe_unused]] auto const output = | ||
| spark_rapids_jni::from_json_to_structs(cudf::strings_column_view{input->view()}, | ||
| col_names, | ||
| num_children, | ||
| types, | ||
| scales, | ||
| precisions, | ||
| /*normalize_single_quotes=*/true, | ||
| /*allow_leading_zeros=*/true, | ||
| /*allow_nonnumeric_numbers=*/true, | ||
| /*allow_unquoted_control=*/true, | ||
| /*is_us_locale=*/true); | ||
| }); | ||
|
|
||
| state.add_buffer_size(num_rows, "rows", "Rows"); | ||
| } | ||
|
|
||
| NVBENCH_BENCH(BM_from_json_to_structs) | ||
|
Collaborator
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. 🟡 SHOULD FIX Could the regression include two mismatched top-level columns with different row-index vectors? The new lookup at src/main/cpp/src/from_json_to_structs.cu:974 builds one map entry per diagnostic column and selects rows by each schema name at src/main/cpp/src/from_json_to_structs.cu:990. The only current regression schema has one mismatch-capable top-level STRUCT ( Suggested fix: Add a two-column schema with Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.
Collaborator
Author
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. Done in ca0366b: added a two-column regression with independent mismatch rows for a and b. FromJsonToStructsTest passed 5/5. |
||
| .set_name("from_json_to_structs") | ||
| .add_int64_axis("num_rows", {10'000, 100'000}) | ||
| .add_int64_axis("mismatch_percent", {0, 1, 10, 50, 100}); | ||
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.
🟡 SHOULD FIX
Can the new benchmark include the owning
cudf::columnheader directly and keep its project-local header in the first include group?src/main/cpp/benchmarks/from_json_to_structs.cu:31namescudf::column, andsrc/main/cpp/benchmarks/from_json_to_structs.cu:81callscudf::column::view(), butsrc/main/cpp/benchmarks/from_json_to_structs.cu:17-23has no<cudf/column/column.hpp>and places the internalsrc/main/cpp/src/json_utils.hpp:17header after other RAPIDS headers using angle brackets. This makes the translation unit depend on another header exposing the completecudf::columndefinition and diverges from the project's local-first/internal-header include convention.Suggested fix: Move the project-local header to the first group using quotes and directly include
<cudf/column/column.hpp>in the cuDF group.Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.
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.
Thanks, updated.