GH-36819: [R] Use RunWithCapturedR for reading Parquet files

[paleolimbot]

Rationale for this change

When we first added RunWithCapturedR to support reading files from R connections, none of the Parquet tests seemed to call R from another thread. Because RunWithCapturedR comes with some complexity, I didn't add it anywhere it wasn't strictly needed. A recent StackOverflow post exposed that reading very large parquet files do use multiple threads and thus need RunWithCapturedR.

What changes are included in this PR?

The two most common calls to read a parquet in which a user might trigger this failure are now wrapped in RunWithCapturedR.

Are these changes tested?

The changes are tested in the current suite.

Are there any user-facing changes?

No.

• Closes: [R] Reading large Parquet files from a seekable connection fails #36819

[github-actions]

⚠️ GitHub issue #36819 has been automatically assigned in GitHub to PR creator.

[nealrichardson]

I'm not familiar enough with parquet-cpp to know which methods and under what conditions Parquet file reading is multithreaded--I'm actually surprised it's not all multithreaded. @pitrou @jorisvandenbossche what do you think?

Naively, if ReadTable needs this treatment, I'd expect that ReadRowGroup and ReadColumn would too.

[paleolimbot]

@ursabot please benchmark

[ursabot]

Benchmark runs are scheduled for commit 775db90. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

[paleolimbot]

I believe the fact that the parquet reader issues reads on the calling (or any non-IO) thread is considered a bug (#30496). Good catch on issuing all read calls in the same way!

[conbench-apache-arrow]

Thanks for your patience. Conbench analyzed the 6 benchmarking runs that have been run so far on PR commit 775db90.

There were 3 benchmark results indicating a performance regression:

• Pull Request Run on ursa-thinkcentre-m75q at 2023-08-28 19:09:13Z
  • BM_ReadInt64Column (C++) with params=<Repetition::OPTIONAL, Compression::ZSTD>/65536/1024, source=cpp-micro, suite=parquet-column-io-benchmark
  • BM_ReadInt64Column (C++) with params=<Repetition::REQUIRED, Compression::ZSTD>/65536/1024, source=cpp-micro, suite=parquet-column-io-benchmark
• and 1 more (see the report linked below)

The full Conbench report has more details.

[jorisvandenbossche]

AFAIK all of the FileReader::ReadTable, ReadRowGroup, etc methods can read columns in parallel depending on a setting on the reader object (FileReader::set_use_threads, which in pyarrow is exposed as a use_threads keyword in the single file Parquet reader).

This for the plain Parquet reader, not the Parquet scanning through dataset, which I think has its own logic of threading.

[thisisnic]

Thanks @paleolimbot !

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit b5d36e9.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.