Skip to content

ARROW-12859: [C++] Add ScalarFromJSON for testing #10386

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.

Sign up for GitHub

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

Jump to bottom
Closed
lidavidm wants to merge 2 commits into from
Closed

ARROW-12859: [C++] Add ScalarFromJSON for testing #10386

lidavidm wants to merge 2 commits into from

Conversation

@lidavidm
Copy link
Member

@lidavidm lidavidm commented May 24, 2021 •
edited by bkietz
Loading 

ScalarFromJSON(int64(), "null") == Datum(Int64Scalar())
ScalarFromJSON(int64(), "5")    == Datum(Int64Scalar(5))

@lidavidm lidavidm mentioned this pull request May 24, 2021
Closed
@github-actions
Copy link

github-actions bot commented May 24, 2021

https://issues.apache.org/jira/browse/ARROW-12859

@bkietz bkietz changed the title ARROW-12859: [C++] Add DatumFromJSON for testing ARROW-12859: [C++] Add ScalarFromJSON for testing May 25, 2021
bkietz
bkietz reviewed May 25, 2021
View reviewed changes
cpp/src/arrow/testing/gtest_util.cc Outdated
Copy link
Member

@bkietz bkietz May 25, 2021

Choose a reason for hiding this comment

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

This approach might be worthwhile for consistency's sake, but we could also use Scalar::CastTo

@pitrou
Copy link
Member

pitrou commented Jun 1, 2021

Is there anything left to do here?

@lidavidm
Copy link
Member Author

lidavidm commented Jun 1, 2021

Is there anything left to do here?

I don't think so unless we really want to use Scalar::CastTo here.

@pitrou
Copy link
Member

pitrou commented Jun 1, 2021

Can you explain the Scalar::CastTo suggestion? I think I'm missing something.

Also, I notice this isn't actually tested? Perhaps add a test alongside those for ArrayFromJSON. And ensure that a non-1 length array would return an error?

@lidavidm
Copy link
Member Author

lidavidm commented Jun 1, 2021

For CastTo: I think it was actually the Cast kernel (string->type cast).

I added some basic tests. I don't think there's a good way to hit the DCHECK because that would imply a JSON value converter appended two array values for a single JSON value.

pitrou
pitrou approved these changes Jun 2, 2021
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1

@pitrou pitrou closed this in f3344dc Jun 2, 2021
@asfimport asfimport mentioned this pull request Jun 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkietz bkietz bkietz approved these changes

@pitrou pitrou pitrou approved these changes

+1 more reviewer

@nirandaperera nirandaperera nirandaperera approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lidavidm @pitrou @bkietz @nirandaperera