westonpace commented on code in PR #13751:
URL: https://github.com/apache/arrow/pull/13751#discussion_r933654764
##########
cpp/src/arrow/compute/light_array.h:
##########
@@ -40,9 +40,35 @@ namespace compute {
/// allows us to take advantage of these resources without coupling the logic
with
/// the execution engine.
struct LightContext {
+ static constexpr int kLogMinibatchSizeMax = 10;
+ static constexpr int kMinibatchSizeMax = 1 << kLogMinibatchSizeMax;
+
bool has_avx2() const { return (hardware_flags &
arrow::internal::CpuInfo::AVX2) > 0; }
int64_t hardware_flags;
util::TempVectorStack* stack;
+ MemoryPool* memory_pool;
+
+ // A deleter for a light context that owns its TempVectorStack
+ struct OwningLightContextDeleter {
+ void operator()(LightContext* ctx) const {
+ delete ctx->stack;
+ delete ctx;
+ };
+ };
+
+ // Creates a new light context with an owned temp vector stack. Only to be
+ // used at the highest level (e.g. in ExecPlan, unit tests or benchmarks)
+ // Nodes should get their light context from the plan
Review Comment:
We have too many context objects at the moment :laughing: . `LightContext`
was my poor attempt at https://xkcd.com/927/
However, since it holds a non-owning pointer to `TempVectorStack` it can't
just be easily created for unit tests. So this was just a quick-hack for unit
tests. The unique_ptr with a custom deleter is just a way of doing...
```
struct LightContextThatAlsoOwnsTempVectorStack {
LightContext ctx;
TempVectorStack stack;
}
```
...without creating a new class since I was afraid of creating "yet another
context variant". However, I think I might as well do that explicitly and
create a new class. It will be less confusing to new readers.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
