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 JsonToStructs and ScanJson to have white space normalization #10575

Merged
merged 6 commits into from
Mar 15, 2024

Conversation

revans2
Copy link
Collaborator

@revans2 revans2 commented Mar 12, 2024

This also contributes to #10491 in a very small way by adding in a few more tests.

Mostly it turns on white space normalization and tries to verify that it is doing the right thing, but there were some errors, so I filed more issues.

@revans2
Copy link
Collaborator Author

revans2 commented Mar 12, 2024

build

@revans2
Copy link
Collaborator Author

revans2 commented Mar 13, 2024

build

@revans2
Copy link
Collaborator Author

revans2 commented Mar 13, 2024

build

jlowe
jlowe previously approved these changes Mar 14, 2024
Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Minor comment, overall lgtm.

def deepTransformView(cv: ColumnView, dt: Option[DataType] = None)
def deepTransformView(cv: ColumnView, dt: Option[DataType] = None,
nestedMismatchHandler: Option[(ColumnView, DataType) =>
(Option[ColumnView], ArrayBuffer[AutoCloseable])] = None)
Copy link
Member

Choose a reason for hiding this comment

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

Does the handler need to return a mutable ArrayBuffer? I think the handler could return an immutable Seq given how it's being used, and that seems more flexible and less error-prone than forcing an ArrayBuffer here.

@revans2
Copy link
Collaborator Author

revans2 commented Mar 14, 2024

build

@revans2
Copy link
Collaborator Author

revans2 commented Mar 14, 2024

@jlowe please take another look

Copy link
Member

@jlowe jlowe left a comment

Choose a reason for hiding this comment

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

Looks like the ArrayBuffer -> Seq change only happened halfway, suggested some updates.

@revans2
Copy link
Collaborator Author

revans2 commented Mar 15, 2024

build

@revans2
Copy link
Collaborator Author

revans2 commented Mar 15, 2024

@jlowe please take another look

@revans2 revans2 merged commit a56da0c into NVIDIA:branch-24.04 Mar 15, 2024
43 checks passed
@revans2 revans2 deleted the from_json_normal_whitespace branch March 15, 2024 17:01
@sameerz sameerz added the bug Something isn't working label Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants