westonpace commented on code in PR #13782: URL: https://github.com/apache/arrow/pull/13782#discussion_r974180407
########## cpp/src/arrow/compute/exec/expression.h: ########## @@ -277,6 +279,53 @@ ARROW_EXPORT Expression or_(Expression lhs, Expression rhs); ARROW_EXPORT Expression or_(const std::vector<Expression>&); ARROW_EXPORT Expression not_(Expression operand); +/// Modify an Expression with pre-order and post-order visitation. +/// `pre` will be invoked on each Expression. `pre` will visit Calls before their +/// arguments, `post_call` will visit Calls (and no other Expressions) after their +/// arguments. Visitors should return the Identical expression to indicate no change; this +/// will prevent unnecessary construction in the common case where a modification is not +/// possible/necessary/... +/// +/// If an argument was modified, `post_call` visits a reconstructed Call with the modified +/// arguments but also receives a pointer to the unmodified Expression as a second +/// argument. If no arguments were modified the unmodified Expression* will be nullptr. +template <typename PreVisit, typename PostVisitCall> +Result<Expression> Modify(Expression expr, const PreVisit& pre, Review Comment: If I didn't then I ended up needing `expression_internal.h` in `partition.cc`. I'm not entirely sure why but including `expression_internal.h` in two different .cc files in the same module caused duplicate symbol errors. Not on `Modify` (which should be fine because it is templated) but on things like `KnownFieldValues` and `CallNotNull`. Adding `Modify` didn't require any additional includes in `expression.h` and so it doesn't seem too heavyweight an addition but I could be missing some reason we didn't want it exposed. -- 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]
