You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/03/01 17:50:19 UTC
[GitHub] [arrow] nealrichardson opened a new pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
nealrichardson opened a new pull request #12533:
URL: https://github.com/apache/arrow/pull/12533
This allows us to use the query engine without `ARROW_DATASET=ON`, and it's nice to just keep around the object that was provided instead of transforming it, which makes printing query objects a little more intuitive. There may be other optimizations or cleanups we could do as well.
Other changes: added head/tail methods for RecordBatchReader and fixed an edge case (`head(x, 0)` works but was previously rejected as invalid input).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] removed a comment on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] removed a comment on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1055699835
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1056083333
Revision: 06e4436cc0c2d7d565c3ec8d2856909f853a195f
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1708](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1708)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-1708-azure-test-r-minimal-build)|
|test-r-offline-minimal|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-1708-azure-test-r-offline-minimal)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818136658
##########
File path: r/R/dplyr.R
##########
@@ -151,8 +151,14 @@ dim.arrow_dplyr_query <- function(x) {
rows <- NA_integer_
} else if (isTRUE(x$filtered_rows)) {
rows <- x$.data$num_rows
- } else {
+ } else if (query_on_dataset(x)) {
+ # TODO: do this with an ExecPlan instead of Scanner?
Review comment:
```suggestion
# TODO: do this with an ExecPlan instead of Scanner (after ARROW-12311)?
# See also https://github.com/apache/arrow/pull/12533/files#r818129459
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818243975
##########
File path: r/src/recordbatchreader.cpp
##########
@@ -51,6 +51,20 @@ std::shared_ptr<arrow::Table> Table__from_RecordBatchReader(
return table;
}
+// [[arrow::export]]
+std::shared_ptr<arrow::Table> RecordBatchReader__Head(
+ const std::shared_ptr<arrow::RecordBatchReader>& reader, int64_t num_rows) {
+ std::vector<std::shared_ptr<arrow::RecordBatch>> batches;
+ std::shared_ptr<arrow::RecordBatch> this_batch;
+ while (num_rows > 0) {
+ this_batch = ValueOrStop(reader->Next());
+ if (this_batch == nullptr) break;
+ batches.push_back(this_batch->Slice(0, num_rows));
+ num_rows -= this_batch->num_rows();
+ }
+ return ValueOrStop(arrow::Table::FromRecordBatches(reader->schema(), batches));
+}
Review comment:
How so? The tests don't show that; is there an additional test you'd like to see?
FWIW this is adapted from the Scanner::Head method [here](https://github.com/apache/arrow/blob/master/cpp/src/arrow/dataset/scanner.cc#L465-L479)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1059281689
Benchmark runs are scheduled for baseline = f5a0cafe159eb1fe8000d8db6bc3b683ee83bbf8 and contender = 1b796ec3f9caeb5e86e3348ba940bef8d95915c5. 1b796ec3f9caeb5e86e3348ba940bef8d95915c5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/1b1de544e9294e8d997f180e7199e28b...27a7955e596b4abd8dafb260a88a5404/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/84611f515798445b9913e299b4a59fd4...3ff49d42d10c4b5a95919b02a8de4c45/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3a5f74133f554cf3acc7e45fc35c7b2f...1b8baa0103a0416a99a4d4800e94b604/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8a0bac0022084f28a9bf3541eabeab27...b73be9fa55c14342a456d8555f1ad923/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818123668
##########
File path: r/src/compute-exec.cpp
##########
@@ -277,4 +277,18 @@ std::shared_ptr<compute::ExecNode> ExecNode_ReadFromRecordBatchReader(
return MakeExecNodeOrStop("source", plan.get(), {}, options);
}
+// [[arrow::export]]
+std::shared_ptr<compute::ExecNode> ExecNode_TableSourceNode(
+ const std::shared_ptr<compute::ExecPlan>& plan,
+ const std::shared_ptr<arrow::Table>& table) {
+ arrow::compute::TableSourceNodeOptions options{
+ /*table=*/table,
+ // TODO: make batch_size configurable
+ /*batch_size=*/1048576
Review comment:
So I was doing some related testing of the table source node yesterday and I think we are going to change the name of this to `max_batch_size` because it doesn't actually concatenate small batches, it only splits up large ones. I'm not mentioning this to delay this PR, proceed as is and we will adjust the R linkage in the PR that does the rename.
Mostly, I'm just looking to point out that batch size will still largely be controlled by the customer in how they create their table (e.g. if they create a table with many small batches then we will feed many small batches into the exec plan) and this configuration is really only a failsafe to use when the customer has tables with really large batches to make sure we get some parallelism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818170225
##########
File path: r/tests/testthat/test-record-batch-reader.R
##########
@@ -139,3 +139,21 @@ test_that("reader with 0 batches", {
expect_r6_class(tab, "Table")
expect_identical(dim(tab), c(0L, 1L))
})
+
+test_that("reader head method edge cases", {
+ batch <- record_batch(
+ x = 1:10,
+ y = letters[1:10]
+ )
+ sink <- BufferOutputStream$create()
+ writer <- RecordBatchStreamWriter$create(sink, batch$schema)
+ writer$write(batch)
+ writer$write(batch)
+ writer$close()
+ buf <- sink$finish()
+
+ reader <- RecordBatchStreamReader$create(buf)
+ expect_error(head(reader, -1)) # Not (yet) supported
+ expect_equal(head(reader, 0), Table$create(x = integer(0), y = character(0)))
+ expect_equal(head(reader, 100), Table$create(batch, batch))
Review comment:
If I'm reading the head method correctly, it reads from the RecordBatchReader, but pushes the records back on so that they can be read again if necessary. Should we add a test that confirms this?
Something like:
```
expect_equal(head(reader, 10), Table$create(batch))
expect_equal(reader$read_table(), Table$create(batch, batch))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1056082447
@github-actions crossbow submit test-r-*minimal*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818129459
##########
File path: r/R/dplyr.R
##########
@@ -151,8 +151,14 @@ dim.arrow_dplyr_query <- function(x) {
rows <- NA_integer_
} else if (isTRUE(x$filtered_rows)) {
rows <- x$.data$num_rows
- } else {
+ } else if (query_on_dataset(x)) {
+ # TODO: do this with an ExecPlan instead of Scanner?
Review comment:
At the moment at least, scanner's count rows implementation has inaccessible custom logic that makes it more efficient than a count rows done with an exec plan. So don't do this before ARROW-12311 is addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818106094
##########
File path: r/R/arrow-package.R
##########
@@ -128,7 +128,7 @@ arrow_with_dataset <- function() {
is_32bit <- .Machine$sizeof.pointer < 8
is_old_r <- getRversion() < "4.0.0"
is_windows <- tolower(Sys.info()[["sysname"]]) == "windows"
- if (is_32bit && is_old_r && is_windows) {
+ if (on_old_windows()) {
Review comment:
Oh yep good catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #12533:
URL: https://github.com/apache/arrow/pull/12533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1055699807
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1055706965
@github-actions crossbow submit test-r-*minimal*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818106331
##########
File path: r/R/arrow-package.R
##########
@@ -128,7 +128,7 @@ arrow_with_dataset <- function() {
is_32bit <- .Machine$sizeof.pointer < 8
is_old_r <- getRversion() < "4.0.0"
is_windows <- tolower(Sys.info()[["sysname"]]) == "windows"
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wjones127 commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
wjones127 commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818082451
##########
File path: r/R/arrow-package.R
##########
@@ -128,7 +128,7 @@ arrow_with_dataset <- function() {
is_32bit <- .Machine$sizeof.pointer < 8
is_old_r <- getRversion() < "4.0.0"
is_windows <- tolower(Sys.info()[["sysname"]]) == "windows"
- if (is_32bit && is_old_r && is_windows) {
+ if (on_old_windows()) {
Review comment:
If we're moving this into a function, can we delete the variables above?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1059281689
Benchmark runs are scheduled for baseline = f5a0cafe159eb1fe8000d8db6bc3b683ee83bbf8 and contender = 1b796ec3f9caeb5e86e3348ba940bef8d95915c5. 1b796ec3f9caeb5e86e3348ba940bef8d95915c5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/1b1de544e9294e8d997f180e7199e28b...27a7955e596b4abd8dafb260a88a5404/)
[Finished :arrow_down:10.47% :arrow_up:8.29%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/84611f515798445b9913e299b4a59fd4...3ff49d42d10c4b5a95919b02a8de4c45/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3a5f74133f554cf3acc7e45fc35c7b2f...1b8baa0103a0416a99a4d4800e94b604/)
[Finished :arrow_down:0.38% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8a0bac0022084f28a9bf3541eabeab27...b73be9fa55c14342a456d8555f1ad923/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1059281689
Benchmark runs are scheduled for baseline = f5a0cafe159eb1fe8000d8db6bc3b683ee83bbf8 and contender = 1b796ec3f9caeb5e86e3348ba940bef8d95915c5. 1b796ec3f9caeb5e86e3348ba940bef8d95915c5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/1b1de544e9294e8d997f180e7199e28b...27a7955e596b4abd8dafb260a88a5404/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/84611f515798445b9913e299b4a59fd4...3ff49d42d10c4b5a95919b02a8de4c45/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3a5f74133f554cf3acc7e45fc35c7b2f...1b8baa0103a0416a99a4d4800e94b604/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8a0bac0022084f28a9bf3541eabeab27...b73be9fa55c14342a456d8555f1ad923/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1059281689
Benchmark runs are scheduled for baseline = f5a0cafe159eb1fe8000d8db6bc3b683ee83bbf8 and contender = 1b796ec3f9caeb5e86e3348ba940bef8d95915c5. 1b796ec3f9caeb5e86e3348ba940bef8d95915c5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/1b1de544e9294e8d997f180e7199e28b...27a7955e596b4abd8dafb260a88a5404/)
[Finished :arrow_down:10.47% :arrow_up:8.29%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/84611f515798445b9913e299b4a59fd4...3ff49d42d10c4b5a95919b02a8de4c45/)
[Failed] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3a5f74133f554cf3acc7e45fc35c7b2f...1b8baa0103a0416a99a4d4800e94b604/)
[Finished :arrow_down:0.38% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/8a0bac0022084f28a9bf3541eabeab27...b73be9fa55c14342a456d8555f1ad923/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r817009732
##########
File path: r/src/compute-exec.cpp
##########
@@ -277,4 +277,18 @@ std::shared_ptr<compute::ExecNode> ExecNode_ReadFromRecordBatchReader(
return MakeExecNodeOrStop("source", plan.get(), {}, options);
}
+// [[arrow::export]]
+std::shared_ptr<compute::ExecNode> ExecNode_TableSourceNode(
+ const std::shared_ptr<compute::ExecPlan>& plan,
+ const std::shared_ptr<arrow::Table>& table) {
+ arrow::compute::TableSourceNodeOptions options{
+ /*table=*/table,
+ // TODO: make batch_size configurable
+ /*batch_size=*/1048576
Review comment:
@westonpace is this a reasonable default? I heard 1M rows somewhere so went with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#issuecomment-1055707907
Revision: a72bfd32b928f152a153a6ceadb1d4bc40d49a78
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1699](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1699)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-1699-azure-test-r-minimal-build)|
|test-r-offline-minimal|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-1699-azure-test-r-offline-minimal)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r817310102
##########
File path: r/src/compute-exec.cpp
##########
@@ -277,4 +277,18 @@ std::shared_ptr<compute::ExecNode> ExecNode_ReadFromRecordBatchReader(
return MakeExecNodeOrStop("source", plan.get(), {}, options);
}
+// [[arrow::export]]
+std::shared_ptr<compute::ExecNode> ExecNode_TableSourceNode(
+ const std::shared_ptr<compute::ExecPlan>& plan,
+ const std::shared_ptr<arrow::Table>& table) {
+ arrow::compute::TableSourceNodeOptions options{
+ /*table=*/table,
+ // TODO: make batch_size configurable
+ /*batch_size=*/1048576
Review comment:
Yes. I wouldn't worry about making it configurable at the moment either. If our default is non-ideal then we should be figuring out a more-ideal default. At the moment I don't know of any case where a user will have more information than us in picking a reasonable default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818599359
##########
File path: r/src/recordbatchreader.cpp
##########
@@ -51,6 +51,20 @@ std::shared_ptr<arrow::Table> Table__from_RecordBatchReader(
return table;
}
+// [[arrow::export]]
+std::shared_ptr<arrow::Table> RecordBatchReader__Head(
+ const std::shared_ptr<arrow::RecordBatchReader>& reader, int64_t num_rows) {
+ std::vector<std::shared_ptr<arrow::RecordBatch>> batches;
+ std::shared_ptr<arrow::RecordBatch> this_batch;
+ while (num_rows > 0) {
+ this_batch = ValueOrStop(reader->Next());
+ if (this_batch == nullptr) break;
+ batches.push_back(this_batch->Slice(0, num_rows));
+ num_rows -= this_batch->num_rows();
+ }
+ return ValueOrStop(arrow::Table::FromRecordBatches(reader->schema(), batches));
+}
Review comment:
Ah sorry, I missed the `->Slice()` line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818222944
##########
File path: r/src/recordbatchreader.cpp
##########
@@ -51,6 +51,20 @@ std::shared_ptr<arrow::Table> Table__from_RecordBatchReader(
return table;
}
+// [[arrow::export]]
+std::shared_ptr<arrow::Table> RecordBatchReader__Head(
+ const std::shared_ptr<arrow::RecordBatchReader>& reader, int64_t num_rows) {
+ std::vector<std::shared_ptr<arrow::RecordBatch>> batches;
+ std::shared_ptr<arrow::RecordBatch> this_batch;
+ while (num_rows > 0) {
+ this_batch = ValueOrStop(reader->Next());
+ if (this_batch == nullptr) break;
+ batches.push_back(this_batch->Slice(0, num_rows));
+ num_rows -= this_batch->num_rows();
+ }
+ return ValueOrStop(arrow::Table::FromRecordBatches(reader->schema(), batches));
+}
Review comment:
My reading of this is that it may give more rows than `num_rows`...is that the intention? I get that it's awkward because the record batch reader is mutable (as Jon noted below).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #12533: ARROW-15817: [R] Use TableSourceNode instead of InMemoryDataset
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #12533:
URL: https://github.com/apache/arrow/pull/12533#discussion_r818245512
##########
File path: r/tests/testthat/test-record-batch-reader.R
##########
@@ -139,3 +139,21 @@ test_that("reader with 0 batches", {
expect_r6_class(tab, "Table")
expect_identical(dim(tab), c(0L, 1L))
})
+
+test_that("reader head method edge cases", {
+ batch <- record_batch(
+ x = 1:10,
+ y = letters[1:10]
+ )
+ sink <- BufferOutputStream$create()
+ writer <- RecordBatchStreamWriter$create(sink, batch$schema)
+ writer$write(batch)
+ writer$write(batch)
+ writer$close()
+ buf <- sink$finish()
+
+ reader <- RecordBatchStreamReader$create(buf)
+ expect_error(head(reader, -1)) # Not (yet) supported
+ expect_equal(head(reader, 0), Table$create(x = integer(0), y = character(0)))
+ expect_equal(head(reader, 100), Table$create(batch, batch))
Review comment:
Actually I believe the opposite is true: the method is consuming the reader. RecordBatchReader (and Scanner) are one-shot. (I think you may be misreading the C++ function: `reader` is the reader and we collect into `batches` via `push_back`, there is no pushing back into `reader`.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org