westonpace commented on issue #33638:
URL: https://github.com/apache/arrow/issues/33638#issuecomment-1381111412
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++.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
