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.

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

Update from_json to use new cudf features #11497

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Sep 24, 2024

This depends on rapidsai/cudf#16545 but enables a lot more tests to pass. This does not yet enable from_json by default, but a follow on issue probably will with some restrictions on the types that are supported.

@revans2 revans2 marked this pull request as ready for review September 26, 2024 14:17
@revans2
Copy link
Collaborator Author

revans2 commented Sep 26, 2024

The dependency was merged in yesterday so moving to ready for review

@revans2
Copy link
Collaborator Author

revans2 commented Sep 26, 2024

build

@@ -336,6 +336,8 @@ object GpuJsonReadCommon {
.withLeadingZeros(options.allowNumericLeadingZeros)
.withNonNumericNumbers(options.allowNonNumericNumbers)
.withUnquotedControlChars(allowUnquotedControlChars)
.withCudfPruneSchema(true)
.withExperimental(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious on what the plan is for the withExperimental flag to be deprecated.

As it is here, I don't know what it does, but the cuDF code does say that it enables features such as utf-8 and new column tree construction. Is that the extent? Should we add a comment here that says why we set it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The experimental flag enables most of the fixes. They are things that should be okay to work with the python side of things, but to reduce the risk for the 24.10 release they were put under this flag. It is likely that it will be removed at some point in 24.12, but we can wait and see

abellina
abellina previously approved these changes Sep 26, 2024
Copy link
Collaborator

@abellina abellina left a comment

Choose a reason for hiding this comment

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

I had a nit but otherwise looks great, so many tests getting enabled.

@revans2
Copy link
Collaborator Author

revans2 commented Sep 26, 2024

looks like I missed some test failures in 3.2.0. I will investigate what changed so that we can get the proper things filed.

@revans2
Copy link
Collaborator Author

revans2 commented Sep 26, 2024

build

@revans2
Copy link
Collaborator Author

revans2 commented Sep 26, 2024

@abellina please take another look

@revans2 revans2 merged commit fbd4db9 into NVIDIA:branch-24.10 Sep 27, 2024
45 checks passed
@revans2 revans2 deleted the update_tests_for_cudf_fixes branch September 27, 2024 13:08
@sameerz sameerz added the task Work required that improves the product but is not user facing label Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Work required that improves the product but is not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants