Uh oh!
There was an error while loading. Please reload this page.
ARROW-15271: [R] Refactor do_exec_plan to return a RecordBatchReader - #13170
ARROW-15271: [R] Refactor do_exec_plan to return a RecordBatchReader#13170nealrichardson wants to merge 5 commits into
Conversation
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } 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 🎉
ursabot
commented
May 19, 2022
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. |
ursabot
commented
May 19, 2022
['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#5198ARROW-6364#5201ARROW-6323#5169ARROW-6278#5141ARROW-6360#5329ARROW-6533#5450ARROW-6348#5223ARROW-6337#5399ARROW-10850#9128ARROW-10624#9092ARROW-10386#8549ARROW-6994#23308ARROW-12774#10320ARROW-12670#10287ARROW-16828#13484ARROW-14989#13482ARROW-16977#13514ARROW-13404#10999ARROW-16887#13601ARROW-15906#13206ARROW-15280#13171ARROW-16144#13183ARROW-16511#13105ARROW-16085#13088ARROW-16715#13555ARROW-16268#13550ARROW-16700#13518ARROW-16807#13583ARROW-16871#13517ARROW-16415#13190ARROW-14821#12154ARROW-16439#13174ARROW-16394#13118ARROW-16516#13163ARROW-16395#13627ARROW-14848#12589ARROW-16407#13196ARROW-16653#13506ARROW-14575#13160ARROW-15271#13170ARROW-16703#13650ARROW-16444#13397ARROW-15016#13541ARROW-16776#13563ARROW-15622#13090ARROW-18131#14484ARROW-18305#14581ARROW-18285#14615 * Closes: #33631 Authored-by: SHIMA Tatsuya <ts1s1andn@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
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.