ARROW-16977: [R] Update dataset row counting so no integer overflow on large datasets

[thisisnic]

Follow up to #13482 after this one was missed.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-16977

[github-actions]

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

[nealrichardson]

Thanks! Two questions:

• I don't see a test of the r_vec_size() behavior in this PR or the other one. What is our assurance that it behaves as we want?
• I noticed that this function previously returned int64_t, so I wondered whether cpp11 just automatically handled that in the way we wanted. [Turns out someone 🤦 already found out that this doesn't behave nicely], and that we need to handle it ourselves like you have done here. Unfortunately, we have about 20 other functions that return int64_t that would be subject to the same overflow problem. (I searched ^int64_t in r/src/ to find them.) Would you mind fixing them all in this PR too?

[nealrichardson]

Also, it looks like you need to install the package/run the codegen script so that arrowExports.cpp gets updated.

[thisisnic]

I don't see a test of the r_vec_size() behavior in this PR or the other one. What is our assurance that it behaves as we want?

I added some in an earlier version of the previous PR, but they took ages to run locally (though were successful), and on CI just crashed it entirely. I removed them, but couldn't think of an alternative. Do you have any suggestions?

...Would you mind fixing them all in this PR too?

Done.

[nealrichardson]

I don't see a test of the r_vec_size() behavior in this PR or the other one. What is our assurance that it behaves as we want?

I added some in an earlier version of the previous PR, but they took ages to run locally (though were successful), and on CI just crashed it entirely. I removed them, but couldn't think of an alternative. Do you have any suggestions?

You could tack onto one of the tests that is guarded by ARROW_LARGE_MEMORY_TESTS. Those create buffers that are >2GB and require Large types, which means their size exceeds MAX_INT32 bytes. So check that the buffer size doesn't overflow and is double? I think buffer size was one of the other cases that returned int64_t that you would have find/replaced here.

[nealrichardson]

These large memory tests are slow, so I don't think you need two versions, I think you can keep just the Table version.

If you want to test more places r_vec_size is used, you can get $nbytes() on the arrays/chunkedarrays, as well as length(). You can also get array$data() and check its length (that one might not be covered yet...) and get $buffers from that, and check them too.

[thisisnic]

I've removed the RecordBatch test, added in tests for a few more fields, as well as updating a couple of utility functions that these changes broke. The one thing I didn't do is getting array$data() and actually, I'm wondering if overflow is possible with that? From my understanding, an array's maximum size would be the maximum integer value, or am I getting mixed up there?

[nealrichardson]

LargeString/Binary at least have a buffer size that can go up to MAX_INT64, that's why they exist.

[thisisnic]

@nealrichardson Would you mind expanding on that a bit? There's something I'm missing here or misunderstanding.

[nealrichardson]

On the format, see https://github.com/apache/arrow/blob/master/format/Schema.fbs#L155-L171 and https://arrow.apache.org/docs/format/Columnar.html.

In R:

# Here's a simple integer array
> a <- Array$create(1:5)
> a$data()
ArrayData
> a$data()$buffers
[[1]]
NULL

[[2]]
Buffer
# the first buffer is for the null bitmask, but it's empty because there are no NAs

> a$data()$buffers[[2]]
Buffer
> a$data()$buffers[[2]]$size
[1] 20
> a$data()$buffers[[2]]$capacity
[1] 20

# here's a string array
> strings <- Array$create(letters)
# note it has 3 buffers, one for the data and one for the offsets
> strings$data()$buffers
[[1]]
NULL

[[2]]
Buffer

[[3]]
Buffer

> strings$data()$buffers[[2]]$size
[1] 108
> strings$data()$buffers[[3]]$size
[1] 26

# let's make a LargeString array using the helper from the test suite
> make_big_string <- function() {
+   # This creates a character vector that would exceed the capacity of BinaryArray
+   rep(purrr::map_chr(2047:2050, ~ paste(sample(letters, ., replace = TRUE), collapse = "")), 2^18)
+ }
> big <- Array$create(make_big_string())
> big$type
LargeUtf8
large_string
> big$data()$buffers
[[1]]
NULL

[[2]]
Buffer

[[3]]
Buffer

> big$data()$buffers[[2]]$size
[1] 8388616
# the data buffer is > MAX_INT32 so it overflows (on master)
> big$data()$buffers[[3]]$size
[1] -2146959360

[thisisnic]

Thank you for that! Worked out what it was - I'd completely forgotten R's issues with 64 bit integers for a moment there.

[ursabot]

Benchmark runs are scheduled for baseline = 5d86e9f and contender = 87d1889. 87d1889 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.73% ⬆️0.49%] test-mac-arm
[Failed ⬇️1.27% ⬆️3.17%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 87d18890 ec2-t3-xlarge-us-east-2
[Failed] 87d18890 test-mac-arm
[Failed] 87d18890 ursa-i9-9960x
[Finished] 87d18890 ursa-thinkcentre-m75q
[Finished] 5d86e9fc ec2-t3-xlarge-us-east-2
[Failed] 5d86e9fc test-mac-arm
[Failed] 5d86e9fc ursa-i9-9960x
[Finished] 5d86e9fc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. 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