neilconway commented on code in PR #20346:
URL: https://github.com/apache/datafusion/pull/20346#discussion_r2805926843
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -88,8 +88,32 @@ pub trait OptimizerRule: Debug {
true
}
- /// Try to rewrite `plan` to an optimized form, returning
`Transformed::yes`
- /// if the plan was rewritten and `Transformed::no` if it was not.
+ /// Try to rewrite `plan` to an optimized form, returning
[`Transformed::yes`]
+ /// if the plan was rewritten and [`Transformed::no`] if it was not.
+ ///
+ /// # Notes for implementations:
+ ///
+ /// # Return the same plan if no changes were made
+ ///
+ /// If there are no suitable transformations for the input plan,
+ /// the optimizer should simply return it unmodified.
+ ///
+ /// The optimizer will call `rewrite` several times until a fixed point is
+ /// reached, so it is important that `rewrite` return [`Transformed::no`]
if
+ /// the output is the same.
+ ///
+ /// ## Matching on functions
+ ///
+ /// The rule should avoid function specific transformations, and instead
use
+ /// methods on [`ScalarUDFImpl`], [`AggregateUDFImpl`]. Specifically the
+ /// rule should not check function names as functions can be overridden,
and
+ /// may not have the same semantics as the functions provided with
+ /// DataFusion.
+ ///
+ /// For example if a rule rewrites a function based on the check
+ /// `func.name() == 'sum'`, if rewrite the plan incorrectly if the
Review Comment:
Grammar / typo
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -88,8 +88,32 @@ pub trait OptimizerRule: Debug {
true
}
- /// Try to rewrite `plan` to an optimized form, returning
`Transformed::yes`
- /// if the plan was rewritten and `Transformed::no` if it was not.
+ /// Try to rewrite `plan` to an optimized form, returning
[`Transformed::yes`]
+ /// if the plan was rewritten and [`Transformed::no`] if it was not.
+ ///
+ /// # Notes for implementations:
+ ///
+ /// # Return the same plan if no changes were made
Review Comment:
Two hash marks?
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -88,8 +88,32 @@ pub trait OptimizerRule: Debug {
true
}
- /// Try to rewrite `plan` to an optimized form, returning
`Transformed::yes`
- /// if the plan was rewritten and `Transformed::no` if it was not.
+ /// Try to rewrite `plan` to an optimized form, returning
[`Transformed::yes`]
+ /// if the plan was rewritten and [`Transformed::no`] if it was not.
+ ///
+ /// # Notes for implementations:
+ ///
+ /// # Return the same plan if no changes were made
+ ///
+ /// If there are no suitable transformations for the input plan,
+ /// the optimizer should simply return it unmodified.
+ ///
+ /// The optimizer will call `rewrite` several times until a fixed point is
+ /// reached, so it is important that `rewrite` return [`Transformed::no`]
if
+ /// the output is the same.
+ ///
+ /// ## Matching on functions
+ ///
+ /// The rule should avoid function specific transformations, and instead
use
Review Comment:
function-specific
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
