GH-42240: [R] Fix crash in ParquetFileWriter$WriteTable and add WriteBatch

[amoeba]

Rationale for this change

See #42240.

What changes are included in this PR?

• Fixes a crash in ParquetFileWriter$WriteTable by asserting the class of what's passed in and stopping if it's not a Table
• Since I was already there, added WriteBatch to match pyarrow.parquet.ParquetWriter.write_batch which is just a convenience
• Adds a test for the behavior of trying to write to a closed sink
• Bumps the minimum Arrow C++ test version we test the R package with on CI from 13 to 15
• Removes one ARROW_VERSION_MAJOR >= 15 guard

Are these changes tested?

Yes.

Are there any user-facing changes?

New method on ParquetFileWriter (WriteBatch).

• GitHub Issue: [R] Calling ParquetFileWriter$WriteTable with a non-Table crashes #42240

[github-actions]

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

[jonkeane]

Thanks for jumping on this! I have one comment, feel free to take it or leave it.

[amoeba]

I pushed up a change addressing that comment and noticed I missed documenting the new class method so I added another commit for that. Letting CI run and then I'll merge.

[amoeba]

One of the tests this PR adds, in conjunction with the minimum supported C++ version check, caught a segfault in Arrow C++ that wasn't fixed until Arrow 15. I think we need to put a closed-check in the R package somewhere to catch this. I'll look into that and include a note that the check can be removed once we bump the minimum version to or above 15.

[amoeba]

It wasn't easy to expose the writer's closed_ state to the R package for Arrow C++ 13/14 so I opted just to skip the test on Arrow C++ <15, see 22a706b. @jonkeane can you take a quick look?

[jonkeane]

It wasn't easy to expose the writer's closed_ state to the R package for Arrow C++ 13/14 so I opted just to skip the test on Arrow C++ <15, see 22a706b. @jonkeane can you take a quick look?

Does this mean we should bump our minimum libarrow to 15 then? If we can't support it, that's the way to go IMO. The backwards compatibility with libarrow is nice, but as far as I know it's not used anywhere specifically. It was added with the hopes that we might use system-library libarrow on CRAN among other places, but that hasn't happened.

[amoeba]

Hrm, maybe. I think the only thing to do other than that would be to duplicate the open/closed state tracking on the R side until we bump the minimum version for some other reason. I think it may be relevant that this segfault is just in the manual invocation of the ParquetFileWriter and I'm not sure if that impacts the decision-making process here.

[jonkeane]

Do we know of any actual uses of the backwards compatibility in the wild?

[amoeba]

I'm not aware of any.

[jonkeane]

I'm not aware of any.

In that case I think we should just require >15 and that's ok (that might also let us delete some of the ifdefs too).

[amoeba]

Okay @jonkeane, I've bumped the minimum version from 13 to 15 and updated the PR body. There was only one ARROW_VERSION_MAJOR guard to be removed which I removed.

[jonkeane]

Thanks! Sorry I should have linked this before, but we also need to change

arrow/r/tools/check-versions.R

Line 27 in 89fd566

minimum_cpp_version <- package_version("13.0.0")

too yeah?

[amoeba]

My fault for missing that. I changed that and added a NEWS.md entry while I was at it.

[amoeba]

I think CI is happy now that I pushed c2d39d0.

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.