R package has pragmas that are supressing diagnostics

[paleolimbot]

Describe the bug, including details regarding any error messages, version, and platform.

In compute-exec.cpp, we have:

arrow/r/src/compute-exec.cpp

Lines 44 to 51 in 3146588

	// TODO(weston) using gc_context() in this way is deprecated. Once ordering has
	// been added we can probably entirely remove all reference to ExecPlan from R
	// in favor of DeclarationToXyz
	#pragma GCC diagnostic push
	#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
	auto plan = ValueOrStop(
	compute::ExecPlan::Make(use_threads ? &threaded_context : gc_context()));
	#pragma GCC diagnostic pop

...which is not allowed on CRAN. Ideally we need to find a way to not use any of these pragmas in the R code by fixing it to not use deprecated behaviour. A workaround may be to remove the deprecation by patching a C++ file when we do the vendor step (but hopefully we don't have to do this).

Component(s)

R

[westonpace]

So the problem stems from the case when use_threads is false. ExecPlan::Make(ExecContext*) changed to ExecPlan::Make(ExecContext) but the behavior subtly changed.

In the previous version you were allowed to supply a null executor. In the later iteration you are not.

So the deprecated method actually does a bit of a workaround:

Result<std::shared_ptr<ExecPlan>> ExecPlan::Make(
    QueryOptions opts, ExecContext* ctx,
    std::shared_ptr<const KeyValueMetadata> metadata) {
  if (ctx->executor() == nullptr) {
    ARROW_ASSIGN_OR_RAISE(std::shared_ptr<ThreadPool> tpool, ThreadPool::Make(1));
    ExecContext actual_ctx(ctx->memory_pool(), tpool.get(), ctx->func_registry());
    return std::shared_ptr<ExecPlan>(
        new ExecPlanImpl{opts, actual_ctx, std::move(metadata), std::move(tpool)});
  }
  return ExecPlan::Make(opts, *ctx, std::move(metadata));
}

If there is a null executor it creates a thread pool with one thread to use for the duration of the plan. This is kind of what the old nullptr behavior was (though not exactly). We can almost pull the workaround into R (and then R can use the new non-deprecated call) except the "create an exec plan that keeps a thread pool alive" constructor is really meant to be an internal hack and shouldn't be exposed (and if we did expose it we'd mark it deprecated).

Long term the fix will be to remove all references to ExecPlan. However, we can't do that until "order by" and "top k" nodes don't have to be sink nodes.

I'd be OK with just removing the deprecation warning from ExecPlan::Make. This might be the only tractable solution for 11.0.0. I had primarily introduced it to make sure I caught and fixed all instances of the old behavior within C++.

[paleolimbot]

Thanks for the explanation!

I'd be OK with just removing the deprecation warning from ExecPlan::Make

If you feel comfortable with that solution, that would be my preference for 11.0.0 (I'm worried that in the quest to find a workaround I will break something since the ExecPlan lifecycle + threading fills my heart with fear).