westonpace commented on code in PR #34575:
URL: https://github.com/apache/arrow/pull/34575#discussion_r1138101813
##########
cpp/src/arrow/compute/api.h:
##########
@@ -38,24 +38,16 @@
/// @{
/// @}
-#include "arrow/compute/exec/expression.h" // IWYU pragma: export
-
-/// \defgroup execnode-options Concrete option classes for ExecNode options
-/// @{
-/// @}
-
-#include "arrow/compute/exec/options.h" // IWYU pragma: export
+#include "arrow/compute/expression.h" // IWYU pragma: export
/// \defgroup execnode-row Utilities for working with data in a row-major
format
/// @{
/// @}
#include "arrow/compute/row/grouper.h" // IWYU pragma: export
-/// \defgroup execnode-components Components associated with ExecNode
+/// \defgroup execnode-components Components associated with ExecBatch
/// @{
/// @}
-#include "arrow/compute/exec.h" // IWYU pragma: export
-#include "arrow/compute/exec/exec_plan.h" // IWYU pragma: export
-#include "arrow/compute/exec/groupby.h" // IWYU pragma: export
+#include "arrow/compute/exec.h" // IWYU pragma: export
Review Comment:
This could probably be split someday (no need to do it in this PR).
`ExecContext` / `ExecSpan` / `ExecResult` / `ExecValue` / `CallFunction` /
`GetFunctionExecutor` belong in compute. `ExecBatch` belongs in `exec`.
`SelectionVector` could probably be removed at this point and added back in
later if we ever decide to do something with it.
##########
cpp/src/arrow/compute/type_fwd.h:
##########
@@ -50,14 +50,6 @@ struct KernelState;
struct Declaration;
Review Comment:
I think `Declaration` is part of `exec` too.
##########
cpp/src/arrow/testing/generator.cc:
##########
@@ -373,19 +362,15 @@ class GTestDataGeneratorImpl : public GTestDataGenerator {
EXPECT_OK_AND_ASSIGN(auto batches, target_->ExecBatches(rows_per_batch,
num_batches));
return batches;
}
- ::arrow::compute::Declaration SourceNode(int64_t rows_per_batch,
- int num_batches) override {
- EXPECT_OK_AND_ASSIGN(auto source_node,
- target_->SourceNode(rows_per_batch, num_batches));
- return source_node;
- }
std::shared_ptr<::arrow::Table> Table(int64_t rows_per_chunk, int
num_chunks) override {
EXPECT_OK_AND_ASSIGN(auto table, target_->Table(rows_per_chunk,
num_chunks));
return table;
}
std::shared_ptr<::arrow::Schema> Schema() override { return
target_->Schema(); }
+ std::shared_ptr<DataGenerator> Target() { return target_; }
+
Review Comment:
Is this needed anywhere?
##########
cpp/src/arrow/compute/exec/plan_test.cc:
##########
@@ -1016,9 +1019,14 @@ TEST(ExecPlanExecution, ProjectMaintainsOrder) {
constexpr int kRandomSeed = 42;
constexpr int64_t kRowsPerBatch = 1;
constexpr int kNumBatches = 16;
- auto source_node = gen::Gen({{"x", gen::Step()}})
- ->FailOnError()
- ->SourceNode(kRowsPerBatch, kNumBatches);
+
+ auto generator = gen::Gen({{"x", gen::Step()}})->FailOnError();
Review Comment:
I am planning on starting to use these utilities in more places. However,
we can introduce `exec::gen::Gen` (or something like that) which wraps the
generator and adds exec-specific utilities. So I think this is tolerable for
now.
--
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]
