[GitHub] [arrow] westonpace commented on issue #33638: R package has pragmas that are supressing diagnostics

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org