ARROW-15271: [R] Refactor do_exec_plan to return a RecordBatchReader #13170
Conversation
|
|
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
This is awesome! I took a look through for anything that seemed asmiss and didn't find anything.
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
This looks good, I think the change to forcing the map_batches() lambda return a RecordBatch simplifies the API nicely, so I'm in favor of that.
I just had one question on use of ADQ vs RBR.
nealrichardson
force-pushed
the
exec-to-rbr
branch
from
ef747fb to
e12c633
Compare
| } else { | ||
| duckdb::duckdb_fetch_record_batch(res) | ||
| } | ||
| duckdb::duckdb_fetch_record_batch(res) |
There was a problem hiding this comment.
@jonkeane FYI. See also #11730 (comment) for historical context
There was a problem hiding this comment.
I'm glad we can get this cleaned up 🎉
|
Benchmark runs are scheduled for baseline = 663dc32 and contender = dc39f83. dc39f83 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…Hub issue numbers (#34260) Rewrite the Jira issue numbers to the GitHub issue numbers, so that the GitHub issue numbers are automatically linked to the issues by pkgdown's auto-linking feature. Issue numbers have been rewritten based on the following correspondence. Also, the pkgdown settings have been changed and updated to link to GitHub. I generated the Changelog page using the `pkgdown::build_news()` function and verified that the links work correctly. --- ARROW-6338 #5198 ARROW-6364 #5201 ARROW-6323 #5169 ARROW-6278 #5141 ARROW-6360 #5329 ARROW-6533 #5450 ARROW-6348 #5223 ARROW-6337 #5399 ARROW-10850 #9128 ARROW-10624 #9092 ARROW-10386 #8549 ARROW-6994 #23308 ARROW-12774 #10320 ARROW-12670 #10287 ARROW-16828 #13484 ARROW-14989 #13482 ARROW-16977 #13514 ARROW-13404 #10999 ARROW-16887 #13601 ARROW-15906 #13206 ARROW-15280 #13171 ARROW-16144 #13183 ARROW-16511 #13105 ARROW-16085 #13088 ARROW-16715 #13555 ARROW-16268 #13550 ARROW-16700 #13518 ARROW-16807 #13583 ARROW-16871 #13517 ARROW-16415 #13190 ARROW-14821 #12154 ARROW-16439 #13174 ARROW-16394 #13118 ARROW-16516 #13163 ARROW-16395 #13627 ARROW-14848 #12589 ARROW-16407 #13196 ARROW-16653 #13506 ARROW-14575 #13160 ARROW-15271 #13170 ARROW-16703 #13650 ARROW-16444 #13397 ARROW-15016 #13541 ARROW-16776 #13563 ARROW-15622 #13090 ARROW-18131 #14484 ARROW-18305 #14581 ARROW-18285 #14615 * Closes: #33631 Authored-by: SHIMA Tatsuya <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
5 participants
Ticket title is misleading: this PR actually removes do_exec_plan(). plan$Run() now always returns a RBR; the two cases where Tables are used to post-process ExecPlan results are encapsulated in Run() now.
There is one catch that still needs addressing, but I'll make another jira for it: you can provide schema metadata to the WriteNode but not the other SinkNodes, so anything that preserves R metadata needs to handle that separately because Run() will drop it.
This seems to be a limitation of the C++ library.(edit: I see where I can inject this incompute::MakeGeneratorReaderin the thing that consumes the sink node, I had made a note about that on the JIRA previously. I still will take this up in ARROW-16607.)One other change here: map_batches() now returns a RBR and requires that the function it maps returns something that is coercible to a RecordBatch.