Repository navigation
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: |
thatstatsguy
changed the title
thatstatsguy
changed the title
|
|
kou
changed the title
thatstatsguy
changed the title
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
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_chunksizedocumentation 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_chunksizeparameter 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: 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]>
…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.