-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite #12628
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
Conversation
cyb70289
left a comment
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.
LGTM
cpp/src/arrow/flight/flight_test.cc
Outdated
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.
Are these definitions meant to be repeated for each backend? If so, should there be a convenience macro so that none of them is forgotten?
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.
I added convenience macros. GTest's value-parameterized tests would work better for this but I couldn't get the Windows build to work out (GTest thought that the same test fixture had been defined multiple times and would error on startup)
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 we test for a specific status code here (IOError perhaps)?
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.
Changed to test for IOError.
pitrou
left a comment
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.
LGTM except for a couple nits
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.
What happens if counter % 2 == 0? Should we assert that there's no app metadata?
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 we use checked_cast here?
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 we have a CheckBuffersOnDevice overload for record batches?
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.
checked_cast as well?
|
Benchmark runs are scheduled for baseline = 331fc44 and contender = 6a2c432. 6a2c432 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Expand the common Flight suite with some edge cases discovered while testing UCX (ARROW-15706). Also, factor the Flight client's handling of non-host memory types out of the gRPC transport and into the common code.