You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/02/13 19:59:14 UTC
[PR] Alamb/documentation [datafusion]
alamb opened a new pull request, #20346:
URL: https://github.com/apache/datafusion/pull/20346
## Which issue does this PR close?
- Related to https://github.com/apache/datafusion/pull/20180
## Rationale for this change
I gave feedback to @devanshu0987 https://github.com/apache/datafusion/pull/20180/changes#r2800720037 that it was not a good idea to check for function names in optimizer rules, but then I realized that the rationale for this is not written down anywhere.
## What changes are included in this PR?
Document why checking for function names in optimizer rules is not good and offer alternatives
## Are these changes tested?
By CI
## Are there any user-facing changes?
Just docs, no functional changes
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments on OptimizerRule about function name matching [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #20346:
URL: https://github.com/apache/datafusion/pull/20346
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments on OptimizerRule about function name matching [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20346:
URL: https://github.com/apache/datafusion/pull/20346#discussion_r2847432128
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -88,8 +88,35 @@ 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`] and [`AggregateUDFImpl`]. Specifically, the
Review Comment:
This is a great call -- added in 05f8ca177
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments on OptimizerRule about function name matching [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #20346:
URL: https://github.com/apache/datafusion/pull/20346#discussion_r2839836816
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -88,8 +88,35 @@ 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`] and [`AggregateUDFImpl`]. Specifically, the
Review Comment:
I remember there are still several cases that rely on function name checking, where it is not possible to implement the optimization using methods on `ScalarUDFImpl`.
We have a tracking issue, should we link it here? https://github.com/apache/datafusion/issues/18643
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Update comments on OptimizerRule about function name matching [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org