pitrou commented on code in PR #14663:
URL: https://github.com/apache/arrow/pull/14663#discussion_r1039834731
##########
cpp/src/arrow/compute/exec/expression_test.cc:
##########
@@ -927,6 +927,26 @@ TEST(Expression, FoldConstantsBoolean) {
ExpectFoldsTo(or_(whatever, whatever), whatever);
}
+struct {
+ void operator()(Expression expr, Expression expected,
+ const Schema& schema = *kBoringSchema) {
+ ASSERT_OK_AND_ASSIGN(expr, expr.Bind(schema));
+ ASSERT_OK_AND_ASSIGN(expected, expected.Bind(schema));
+
+ ASSERT_OK_AND_ASSIGN(auto without_named_refs, RemoveNamedRefs(expr));
+
+ EXPECT_EQ(without_named_refs, expected);
+ }
+} ExpectRemovesRefsTo;
Review Comment:
Am I missing something that makes this function better/more useful than a
simple function?
##########
cpp/src/arrow/compute/exec/expression.cc:
##########
@@ -1191,6 +1191,26 @@ Result<Expression> SimplifyWithGuarantee(Expression expr,
return expr;
}
+Result<Expression> RemoveNamedRefs(Expression src) {
+ if (!src.IsBound()) {
+ return Status::Invalid("RemoveNamedRefs called on unbound expression");
+ }
+ return ModifyExpression(
+ std::move(src),
+ [](Expression expr) {
Review Comment:
Nit: add parameter names for these callbacks?
```suggestion
/*pre=*/ [](Expression expr) {
```
##########
cpp/src/arrow/compute/exec/options.h:
##########
@@ -214,9 +214,11 @@ struct ARROW_EXPORT BackpressureOptions {
class ARROW_EXPORT SinkNodeOptions : public ExecNodeOptions {
public:
explicit SinkNodeOptions(std::function<Future<std::optional<ExecBatch>>()>*
generator,
Review Comment:
Do we want to ensure backwards compatibility and still provide the former
3-argument constructor?
##########
cpp/src/arrow/util/thread_pool.h:
##########
@@ -497,5 +497,30 @@ typename Fut::SyncType
RunSynchronously(FnOnce<Fut(Executor*)> get_future,
}
}
+/// \brief Potentially iterate an async generator serially (if use_threads is
false)
+/// \see IterateGenerator
+///
+/// If `use_threads` is true, the global CPU executor will be used. Each call
to
+/// the iterator will simply wait until the next item is available. Tasks
may run in
+/// the background between calls.
+///
+/// If `use_threads` is false, the calling thread only will be used. Each
call to
+/// the iterator will use the calling thread to do enough work to generate
one item.
+/// Tasks will be left in a queue until the next call and no work will be
done between
+/// calls.
+template <typename T>
+Iterator<T> IterateSynchronously(
Review Comment:
Can you add basic tests for this?
##########
cpp/src/arrow/compute/exec/expression.cc:
##########
@@ -1191,6 +1191,26 @@ Result<Expression> SimplifyWithGuarantee(Expression expr,
return expr;
}
+Result<Expression> RemoveNamedRefs(Expression src) {
+ if (!src.IsBound()) {
+ return Status::Invalid("RemoveNamedRefs called on unbound expression");
+ }
+ return ModifyExpression(
+ std::move(src),
+ [](Expression expr) {
+ const Expression::Parameter* param = expr.parameter();
+ if (param && !param->ref.IsFieldPath()) {
+ FieldPath ref_as_path(
+ std::vector<int>(param->indices.begin(), param->indices.end()));
+ return Expression(
+ Expression::Parameter{std::move(ref_as_path), param->type,
param->indices});
+ }
+
+ return expr;
+ },
+ [](Expression expr, ...) { return expr; });
Review Comment:
```suggestion
/*post_call=*/ [](Expression expr, ...) { return expr; });
```
--
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]
