-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
ARROW-16690: [R][FlightRPC] Additional max_chunksize parameter in do_put method #13267
ARROW-16690: [R][FlightRPC] Additional max_chunksize parameter in do_put method #13267
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename pull request title in the following format?
or
See also: |
|
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.
Thank you for this! Just a few changes before we can merge:
- There's a trailing whitespace warning from the linter, which although minor, is something that needs fixing or else we'll get it for all our subsequent PRs
- It needs another
devtools::document()
run to add themax_chunksize
documentation item into the .Rd file - It needs a test to make sure it works! You can double the existing block here: https://github.com/apache/arrow/blob/master/r/tests/testthat/test-python-flight.R#L31-L39 but with passing a
max_chunksize
parameter to make sure it works when passed.
Let me know if you need any help and I'm happy to help get it across the finish line!
@paleolimbot thanks for such a detailed review and prompts on how to resolve it - you were very kind :) I've updated the linter, unit tests and rerun the devtools::document(). Assuming the unit test passes, the only issue remaining is the second comment on the implementation |
Awesome! The test looks great. I think ignoring |
@paleolimbot thanks for your patience! All updated, assuming there are no build issues we should be good to go! |
Benchmark runs are scheduled for baseline = 5f84335 and contender = 6d8624b. 6d8624b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
The "R / AMD64 Windows R 4.2 RTools 42" CI job is failed by this commit: https://github.com/apache/arrow/runs/7957718636?check_suite_focus=true#step:12:70
Could you remove trailing spaces from |
After #13267 there is some trailing whitespace left over in one of the files. Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
After apache#13267 there is some trailing whitespace left over in one of the files. Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…put method (apache#13267) **Summary** An additional parameter in Flight do_put to specify chunk size in R. **Problem** Currently, all data is sent through in a single message. It's a likely scenario that users will want the ability to control the batch sizes without building a custom do_put method. **Solution** Additional (optional) parameter to specify chunk size. Lead-authored-by: Christopher.Dunderdale <[email protected]> Co-authored-by: Christopher Dunderdale <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>
After apache#13267 there is some trailing whitespace left over in one of the files. Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Summary
An additional parameter in Flight do_put to specify chunk size in R.
Problem
Currently, all data is sent through in a single message. It's a likely scenario that users will want the ability to control the batch sizes without building a custom do_put method.
Solution
Additional (optional) parameter to specify chunk size.