jdye64 commented on issue #5583:
URL:
https://github.com/apache/arrow-datafusion/issues/5583#issuecomment-1468193771
Not an expert on this but it seems like what we would really want here is a
"mutable reference for self" aka `&mut self` for the
`OptimizerRule::try_optimize` signature. I think using just a `self` in the
signature would sort of invalidate the Rust paradigm of only using that
signature when the intent is for the struct to be "consumed" and no longer
present after the function has finished execution. Much like how "into_*"
functions are meant to be used and why they have that `self` in the signature.
Also changing that signature to a `&mut self` should cause less of an impact
on existing optimizers both in DataFusion and that others might have in their
codebase. Using this approach it would make sense to expose functions that "act
upon" the `MyRule::my_set` field but don't necessarily update the entire field
itself. For example using a function like `set_insert(&mut self, expr: &Expr)`
instead of `set_myset(mut self, value: HashSet<Expr>)`. Something like this ....
```
impl MyRule {
pub fn new() -> Self {
Self { my_set: HashSet::new() }
}
// Don't use this, left for reference.
pub fn set_myset(mut self, value: HashSet<Expr>) {
self.my_set = value;
}
// Instead using functions like this to access or modify the field in
the mutable self reference.
pub fn set_insert(&mut self, expr: &Expr) {
self.my_set.insert(expr.clone());
}
}
impl OptimizerRule for MyRule {
fn name(&self) -> &str {
"my_rule"
}
fn apply_order(&self) -> Option<ApplyOrder> {
Some(ApplyOrder::TopDown)
}
// Notice the change from `&self` to `&mut self` here
fn try_optimize(
&mut self,
plan: &LogicalPlan,
_config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
// This way each `try_optimize` invocation will receive a reference
to the same mutable self and can continuously "insert" elements into it
self.set_insert(&col("test"));
}
}
```
--
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]
