[FEA] Make cudf::ast::operation own its subexpressions

[vyasr]

Is your feature request related to a problem? Please describe.
Currently users are responsible for keeping all subexpressions of an expression alive. In particular, operations do not own their operands, which are stored as a std::vector<std::reference_wrapper<expression const>> const. This approach is cumbersome because it forces client code to keep track of all previously created expressions to avoid their premature destruction.

Describe the solution you'd like
Instead of storing a vector of references to its operands, an operation should instead store a vector of unique pointers to copies of the operands. While this change would create duplicate expressions, they are cheap to carry around, and this change would simplify the management of expression lifetimes in all client code.

Describe alternatives you've considered
We could use std::shared_ptr instead of using std::unique_ptr, which would allow subexpression reuse. Reuse isn't that important here though because expressions are relatively small and only use host memory. The primary goal is to simplify the management of expression objects, which is simpler with unique_ptr than shared_ptr.

We could also retain the status quo, which is not unworkable, just more complex and less intuitive than necessary. The fact that higher-level clients built on top of libcudf likely to build different ownerships models into their wrappers of expression APIs indicates that the libcudf approach is not the friendliest API for programmatic expression construction.

Additional context
This change would not change that aliteral only stores a reference to the underlying scalar. Creating a scalar involves a device memory allocation, an expensive operation that we should not hide from a user. Additionally, it is reasonably straightforward for client code to wrap a literal in some owning container that handles scalar allocation if needed, whereas maintaining all subexpressions of potentially arbitrarily complex expressions is a significantly more onerous task.

[vyasr]

@jlowe I believe that you were interested in something like this at one point as well. We had higher priorities and didn't consider addressing it at the time, but I think this would help simplify some of the JNI wrappers right?

[jlowe]

I believe that you were interested in something like this at one point as well.

Yes, it would simplify things a bit since the JNI code explicitly tracks all the subexpressions and deletes them when the parent expression is finally deleted. The Java code does a full translation from a Java tree to a native tree in an explicit compile() step, so nothing in the Java layer should need to change. It should only be a change in the JNI native code layer that explicitly tracks the AST sub-expressions. Now that it's coded and working, not a high priority from my perspective. 😄

[github-actions]

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

[github-actions]

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

[vyasr]

We decided in #17156 to go a different direction. Rather than having operations owning subexpressions, we introduced a new tree class that manages all expressions that it owns. That approach was chosen instead of owning subexpressions to avoid the recursive copying that would be otherwise introduced by having operations own their subexpressions if you wanted to support assignment or similar operations where an expression could get added to multiple operations, in which case each operation would have to copy the subexpression.

@lamarrr please let me know if I've missed anything in my summary or if there's any reason to reopen this issue, but I'm going to close this as resolved by #17156 for now.